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

[Alex Williamson]

On Wed, 29 Mar 2017 12:41:01 +1100
Alexey Kardashevskiy <address@hidden> wrote:

> On 29/03/17 04:48, 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;  
> 
> 
> Where do I put this enum definition? include/exec/memory.h? It does not
> have any mention of any platform yet...

I would assume memory.h, yes.  It seems like the enum is just an
abstraction, what does "get fd" mean generically to an IOMMU
MemoryRegion?  How can anyone else ever implement that callback if the
initial user is assuming that the returned fd is a specific, yet
unspecified type.  If the API is "give me an fd for this type of thing"
then the IOMMU driver can either provide it or indicate that type is not
supported.  There's really no platform knowledge at the memory API
level, it's just a type of thing that means something to the driver
providing the MemoryRegionIOMMUOps and the caller.
 
> I could pass char* instead of IOMMUFdType (and pass there something like
> TYPE_SPAPR_TCE_TABLE), would it be any better?

Gack, an enum seems so much cleaner than requiring a strcmp.  Thanks,

Alex

> > 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)  
> >   
> 
>