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

[Paolo Bonzini]

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.

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