Re: [Qemu-devel] [RFC PATCH qemu 1/3] memory: Add get_fd() hook for IOMMU MR

[Alexey Kardashevskiy]

On 29/03/17 14:35, David Gibson wrote:
> On Tue, Mar 28, 2017 at 11:48:29AM -0600, Alex Williamson wrote:
>> On Tue, 28 Mar 2017 20:05:28 +1100
>> Alexey Kardashevskiy <address@hidden> wrote:
>>
>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>> ---
>>>  include/exec/memory.h | 2 ++
>>>  hw/ppc/spapr_iommu.c  | 8 ++++++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index e39256ad03..925c10b35b 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -174,6 +174,8 @@ struct MemoryRegionIOMMUOps {
>>>      void (*notify_flag_changed)(MemoryRegion *iommu,
>>>                                  IOMMUNotifierFlag old_flags,
>>>                                  IOMMUNotifierFlag new_flags);
>>> +    /* Returns a kernel fd for IOMMU */
>>> +    int (*get_fd)(MemoryRegion *iommu);
>>
>> What if we used this as a prototype:
>>
>> int (*get_fd)(IOMMUFdType type, MemoryRegion *iommu);
>>
>> And then we defined:
>>
>> typedef enum {
>>     SPAPR_IOMMU_TABLE_FD = 0,
>> } IOMMUFdType;
> 
> Are we expecting any new types of fd?  Maybe it would be simpler just
> to name this spapr_tce_fd() or something more specific, and only
> generalize if we really need it for another fd type.


So far we managed to keep VFIO and sPAPR-IOMMU relatively separate - they
do not include each others headers and interact via memory regions and
kernel uapi interface. The only direct connection between VFIO and sPAPR at
all is vfio_eeh_as_ok/vfio_eeh_as_op which is rather workaround. I like
this separation tbh.



> 
>>
>> Such that you're actually asking the IOMMUOps for a specific type of FD
>> and it either has it or not, so the caller doesn't need to assume what
>> it is they get back.
>>
>> Furthermore, add:
>>
>> int memory_region_iommu_get_fd(IOMMUFdType type, MemoryRegion *mr)
>> {
>>     assert(memory_region_is_iommu(mr));
>>
>>     if (mr->iommu_ops && mr->iommu_ops->get_fd) {
>>         return mr->iommu_ops->get_fd(type, mr);
>>     }
>>
>>     return -1;
>> }
>>
>>>  };
>>>
>>
>> This should be two patches, patch 1 above, patch 2 below
>>   
>>>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>>> index 9e30e148d6..b61c8f053e 100644
>>> --- a/hw/ppc/spapr_iommu.c
>>> +++ b/hw/ppc/spapr_iommu.c
>>> @@ -170,6 +170,13 @@ static void spapr_tce_notify_flag_changed(MemoryRegion 
>>> *iommu,
>>>      }
>>>  }
>>>  
>>> +static int spapr_tce_get_fd(MemoryRegion *iommu)
>>> +{
>>> +    sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
>>> +
>>> +    return tcet->fd;
>>
>>
>> This would then be:
>>
>>     return type == SPAPR_IOMMU_TABLE_FD ? tcet->fd : -1;
>>
>>> +}
>>> +
>>>  static int spapr_tce_table_post_load(void *opaque, int version_id)
>>>  {
>>>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
>>> @@ -251,6 +258,7 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = {
>>>      .translate = spapr_tce_translate_iommu,
>>>      .get_min_page_size = spapr_tce_get_min_page_size,
>>>      .notify_flag_changed = spapr_tce_notify_flag_changed,
>>> +    .get_fd = spapr_tce_get_fd,
>>>  };
>>>  
>>>  static int spapr_tce_table_realize(DeviceState *dev)
>>
> 


-- 
Alexey

signature.asc
Description: OpenPGP digital signature