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

[David Gibson]

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.

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

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature