Re: [Qemu-devel] [PATCH qemu v9 1/2] memory/iommu: QOM'fy IOMMU MemoryRegion

[Greg Kurz]

On Wed, 12 Jul 2017 12:22:17 +0200
Cornelia Huck <address@hidden> wrote:

> On Tue, 11 Jul 2017 13:56:19 +1000
> Alexey Kardashevskiy <address@hidden> wrote:
> 
> > This defines new QOM object - IOMMUMemoryRegion - with MemoryRegion
> > as a parent.
> > 
> > This moves IOMMU-related fields from MR to IOMMU MR. However to avoid
> > dymanic QOM casting in fast path (address_space_translate, etc),
> > this adds an @is_iommu boolean flag to MR and provides new helper to
> > do simple cast to IOMMU MR - memory_region_get_iommu. The flag
> > is set in the instance init callback. This defines
> > memory_region_is_iommu as memory_region_get_iommu()!=NULL.
> > 
> > This switches MemoryRegion to IOMMUMemoryRegion in most places except
> > the ones where MemoryRegion may be an alias.
> > 
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > Reviewed-by: David Gibson <address@hidden>
> > ---
> > Changes:
> > v9:
> > * added rb:David
> > 
> > v8:
> > * moved is_iommu flag closer to the beginning of the MemoryRegion struct
> > * removed memory_region_init_iommu_type()
> > 
> > v7:
> > * rebased on top of the current upstream
> > 
> > v6:
> > * s/\<iommumr\>/iommu_mr/g
> > 
> > v5:
> > * fixed sparc64, first time in many years did run "./configure" without
> > --target-list :-D Sorry for the noise though :(
> > 
> > v4:
> > * fixed alpha, mips64el and sparc
> > 
> > v3:
> > * rebased on sha1 81b2d5ceb0
> > 
> > v2:
> > * added mr->is_iommu
> > * updated i386/x86_64/s390/sun
> > ---
> >  hw/s390x/s390-pci-bus.h       |   2 +-
> >  include/exec/memory.h         |  55 ++++++++++++++--------
> >  include/hw/i386/intel_iommu.h |   2 +-
> >  include/hw/mips/mips.h        |   2 +-
> >  include/hw/ppc/spapr.h        |   3 +-
> >  include/hw/vfio/vfio-common.h |   2 +-
> >  include/qemu/typedefs.h       |   1 +
> >  exec.c                        |  12 ++---
> >  hw/alpha/typhoon.c            |   8 ++--
> >  hw/dma/rc4030.c               |   8 ++--
> >  hw/i386/amd_iommu.c           |   9 ++--
> >  hw/i386/intel_iommu.c         |  17 +++----
> >  hw/mips/mips_jazz.c           |   2 +-
> >  hw/pci-host/apb.c             |   6 +--
> >  hw/ppc/spapr_iommu.c          |  16 ++++---
> >  hw/s390x/s390-pci-bus.c       |   6 +--
> >  hw/s390x/s390-pci-inst.c      |   8 ++--
> >  hw/vfio/common.c              |  12 +++--
> >  hw/vfio/spapr.c               |   3 +-
> >  memory.c                      | 105 
> > ++++++++++++++++++++++++++++--------------
> >  20 files changed, 170 insertions(+), 109 deletions(-)
> >   
> 
> > @@ -1491,17 +1506,22 @@ void memory_region_init_rom_device(MemoryRegion *mr,
> >      mr->ram_block = qemu_ram_alloc(size, mr, errp);
> >  }
> >  
> > -void memory_region_init_iommu(MemoryRegion *mr,
> > +void memory_region_init_iommu(IOMMUMemoryRegion *iommu_mr,
> >                                Object *owner,
> >                                const MemoryRegionIOMMUOps *ops,
> >                                const char *name,
> >                                uint64_t size)
> >  {
> > -    memory_region_init(mr, owner, name, size);
> > -    mr->iommu_ops = ops,
> > +    struct MemoryRegion *mr;
> > +
> > +    object_initialize(iommu_mr, sizeof(*iommu_mr), 
> > TYPE_IOMMU_MEMORY_REGION);
> > +    mr = MEMORY_REGION(iommu_mr);
> > +    memory_region_do_init(mr, owner, name, size);
> > +    iommu_mr = IOMMU_MEMORY_REGION(mr);
> > +    iommu_mr->iommu_ops = ops,  
> 
> I'd finish with a semicolon instead.
> 

Heh, this nit has been there since:

commit 30951157441aed950ad8ca326500b4986d431c7a
Author: Avi Kivity <address@hidden>
Date:   Tue Oct 30 13:47:46 2012 +0200

    memory: iommu support

> Should this require ops != NULL? There are a number of places where
> ->iommu_ops is dereferenced unconditionally.  
> 
> >      mr->terminates = true;  /* then re-forwards */
> > -    QLIST_INIT(&mr->iommu_notify);
> > -    mr->iommu_notify_flags = IOMMU_NOTIFIER_NONE;
> > +    QLIST_INIT(&iommu_mr->iommu_notify);
> > +    iommu_mr->iommu_notify_flags = IOMMU_NOTIFIER_NONE;
> >  }
> >  
> >  static void memory_region_finalize(Object *obj)  
> 
> (...)
> 
> > -uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr)
> > +uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr)
> >  {
> > -    assert(memory_region_is_iommu(mr));
> > -    if (mr->iommu_ops && mr->iommu_ops->get_min_page_size) {
> > -        return mr->iommu_ops->get_min_page_size(mr);
> > +    if (iommu_mr->iommu_ops && iommu_mr->iommu_ops->get_min_page_size) {  
> 
> I think this is the only place that actually checks for iommu_ops.
> 
> > +        return iommu_mr->iommu_ops->get_min_page_size(iommu_mr);
> >      }
> >      return TARGET_PAGE_SIZE;
> >  }  
> 
> The s390 conversions look fine.
>

pgpnBbplxEDoo.pgp
Description: OpenPGP digital signature