Re: [Qemu-devel] [PATCH qemu v17 06/12] memory: Add reporting of supported page sizes

[David Gibson]

On Mon, Jun 06, 2016 at 03:31:04PM +0200, Paolo Bonzini wrote:
> 
> 
> On 02/06/2016 05:35, David Gibson wrote:
> > On Wed, Jun 01, 2016 at 06:57:37PM +1000, Alexey Kardashevskiy wrote:
> >> > Every IOMMU has some granularity which MemoryRegionIOMMUOps::translate
> >> > uses when translating, however this information is not available outside
> >> > the translate context for various checks.
> >> > 
> >> > This adds a get_page_sizes callback to MemoryRegionIOMMUOps and
> >> > a wrapper for it so IOMMU users (such as VFIO) can know the actual
> >> > page size(s) used by an IOMMU.
> >> > 
> >> > As IOMMU MR represents a guest IOMMU, this uses TARGET_PAGE_SIZE
> >> > as fallback.
> >> > 
> >> > This removes vfio_container_granularity() and uses new helper in
> >> > memory_region_iommu_replay() when replaying IOMMU mappings on added
> >> > IOMMU memory region.
> >> > 
> >> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> >> > Reviewed-by: David Gibson <address@hidden>
> > Paolo,
> > 
> > Looks like you were left off the CC for this one.
> > 
> > I think this is ready to go - do you want to merge, comment or ack and
> > we'll take it either through my tree or Alex's?
> 
> It's okay for you to merge, but the callback should be called
> "get_page_size" or "get_replay_granularity".  The plural is weird.

Hm, no, it really could return multiple page sizes if the logical
(guest side) IOMMU supports them.  That might be useful at some point
in the future.  For now, it's sufficient for the replay to use the
smallest pagesize.

> 
> Thanks,
> 
> Paolo
> 
> 
> >> > ---
> >> > Changes:
> >> > v16:
> >> > * used memory_region_iommu_get_page_sizes() instead of
> >> > mr->iommu_ops->get_page_sizes() in memory_region_iommu_replay()
> >> > 
> >> > v15:
> >> > * s/qemu_real_host_page_size/TARGET_PAGE_SIZE/ in 
> >> > memory_region_iommu_get_page_sizes
> >> > 
> >> > v14:
> >> > * removed vfio_container_granularity(), changed 
> >> > memory_region_iommu_replay()
> >> > 
> >> > v4:
> >> > * s/1<<TARGET_PAGE_BITS/qemu_real_host_page_size/
> >> > ---
> >> >  hw/ppc/spapr_iommu.c  |  8 ++++++++
> >> >  hw/vfio/common.c      |  6 ------
> >> >  include/exec/memory.h | 18 ++++++++++++++----
> >> >  memory.c              | 16 +++++++++++++---
> >> >  4 files changed, 35 insertions(+), 13 deletions(-)
> >> > 
> >> > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> >> > index a3cc572..90a45c0 100644
> >> > --- a/hw/ppc/spapr_iommu.c
> >> > +++ b/hw/ppc/spapr_iommu.c
> >> > @@ -149,6 +149,13 @@ static void spapr_tce_table_pre_save(void *opaque)
> >> >                                 tcet->bus_offset, tcet->page_shift);
> >> >  }
> >> >  
> >> > +static uint64_t spapr_tce_get_page_sizes(MemoryRegion *iommu)
> >> > +{
> >> > +    sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
> >> > +
> >> > +    return 1ULL << tcet->page_shift;
> >> > +}
> >> > +
> >> >  static int spapr_tce_table_post_load(void *opaque, int version_id)
> >> >  {
> >> >      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> >> > @@ -228,6 +235,7 @@ static const VMStateDescription 
> >> > vmstate_spapr_tce_table = {
> >> >  
> >> >  static MemoryRegionIOMMUOps spapr_iommu_ops = {
> >> >      .translate = spapr_tce_translate_iommu,
> >> > +    .get_page_sizes = spapr_tce_get_page_sizes,
> >> >  };
> >> >  
> >> >  static int spapr_tce_table_realize(DeviceState *dev)
> >> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> > index e51ed3a..f1a12b0 100644
> >> > --- a/hw/vfio/common.c
> >> > +++ b/hw/vfio/common.c
> >> > @@ -322,11 +322,6 @@ out:
> >> >      rcu_read_unlock();
> >> >  }
> >> >  
> >> > -static hwaddr vfio_container_granularity(VFIOContainer *container)
> >> > -{
> >> > -    return (hwaddr)1 << ctz64(container->iova_pgsizes);
> >> > -}
> >> > -
> >> >  static void vfio_listener_region_add(MemoryListener *listener,
> >> >                                       MemoryRegionSection *section)
> >> >  {
> >> > @@ -394,7 +389,6 @@ static void vfio_listener_region_add(MemoryListener 
> >> > *listener,
> >> >  
> >> >          memory_region_register_iommu_notifier(giommu->iommu, 
> >> > &giommu->n);
> >> >          memory_region_iommu_replay(giommu->iommu, &giommu->n,
> >> > -                                   
> >> > vfio_container_granularity(container),
> >> >                                     false);
> >> >  
> >> >          return;
> >> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> >> > index f649697..bd9625f 100644
> >> > --- a/include/exec/memory.h
> >> > +++ b/include/exec/memory.h
> >> > @@ -149,6 +149,8 @@ typedef struct MemoryRegionIOMMUOps 
> >> > MemoryRegionIOMMUOps;
> >> >  struct MemoryRegionIOMMUOps {
> >> >      /* Return a TLB entry that contains a given address. */
> >> >      IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool 
> >> > is_write);
> >> > +    /* Returns supported page sizes */
> >> > +    uint64_t (*get_page_sizes)(MemoryRegion *iommu);
> >> >  };
> >> >  
> >> >  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> >> > @@ -571,6 +573,15 @@ static inline bool 
> >> > memory_region_is_iommu(MemoryRegion *mr)
> >> >  
> >> >  
> >> >  /**
> >> > + * memory_region_iommu_get_page_sizes: get supported page sizes in an 
> >> > iommu
> >> > + *
> >> > + * Returns %bitmap of supported page sizes for an iommu.
> >> > + *
> >> > + * @mr: the memory region being queried
> >> > + */
> >> > +uint64_t memory_region_iommu_get_page_sizes(MemoryRegion *mr);
> >> > +
> >> > +/**
> >> >   * memory_region_notify_iommu: notify a change in an IOMMU translation 
> >> > entry.
> >> >   *
> >> >   * @mr: the memory region that was changed
> >> > @@ -594,16 +605,15 @@ void 
> >> > memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n);
> >> >  
> >> >  /**
> >> >   * memory_region_iommu_replay: replay existing IOMMU translations to
> >> > - * a notifier
> >> > + * a notifier with the minimum page granularity returned by
> >> > + * mr->iommu_ops->get_page_sizes().
> >> >   *
> >> >   * @mr: the memory region to observe
> >> >   * @n: the notifier to which to replay iommu mappings
> >> > - * @granularity: Minimum page granularity to replay notifications for
> >> >   * @is_write: Whether to treat the replay as a translate "write"
> >> >   *     through the iommu
> >> >   */
> >> > -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n,
> >> > -                                hwaddr granularity, bool is_write);
> >> > +void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool 
> >> > is_write);
> >> >  
> >> >  /**
> >> >   * memory_region_unregister_iommu_notifier: unregister a notifier for
> >> > diff --git a/memory.c b/memory.c
> >> > index 4e3cda8..761ae92 100644
> >> > --- a/memory.c
> >> > +++ b/memory.c
> >> > @@ -1500,12 +1500,22 @@ void 
> >> > memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n)
> >> >      notifier_list_add(&mr->iommu_notify, n);
> >> >  }
> >> >  
> >> > -void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n,
> >> > -                                hwaddr granularity, bool is_write)
> >> > +uint64_t memory_region_iommu_get_page_sizes(MemoryRegion *mr)
> >> >  {
> >> > -    hwaddr addr;
> >> > +    assert(memory_region_is_iommu(mr));
> >> > +    if (mr->iommu_ops && mr->iommu_ops->get_page_sizes) {
> >> > +        return mr->iommu_ops->get_page_sizes(mr);
> >> > +    }
> >> > +    return TARGET_PAGE_SIZE;
> >> > +}
> >> > +
> >> > +void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, bool 
> >> > is_write)
> >> > +{
> >> > +    hwaddr addr, granularity;
> >> >      IOMMUTLBEntry iotlb;
> >> >  
> >> > +    granularity = (hwaddr)1 << 
> >> > ctz64(memory_region_iommu_get_page_sizes(mr));
> >> > +
> >> >      for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
> 

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