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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4504 bytes --]

On Fri, Jun 09, 2017 at 12:28:35AM +1000, Alexey Kardashevskiy wrote:
> On 08/06/17 17:42, David Gibson wrote:
> > On Wed, Jun 07, 2017 at 06:32:43PM +1000, Alexey Kardashevskiy 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.
> >>
> >> This defines memory_region_init_iommu_type() to allow creating
> >> IOMMUMemoryRegion subclasses. In order to support custom QOM type,
> >> this splits memory_region_init() to object_initialize() +
> >> memory_region_do_init.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > 
> > So.. this seems like an only halfway QOMification.  The main init
> > function still takes an ops structure, whereas the QOMish way to do
> > this would be to have the base IOMMUMemoryRegion be an abstract class,
> > and have the IOMMUOps pointers as part of the IOMMUMemoryRegionClass.
> 
> This would mean getting the class pointer from an IOMMUMR object every time
> IOMMUOps::translate() is called and Paolo pointed out earlier that we do
> not want that in the fast path.

So, doing that via the standard macro wrappers would be slow, because
it does type verification.  However, looking up the class function is
fundamentally:

	((IOMMUMemoryRegionClass *)(((Object *)mr)->class))->translate

i.e. two pointers followed, which is just the same as
	mr->ops->translate

So putting the IOMMU ops into the class structure doesn't preclude a
fast path.

> > Maybe you can persuade me this is a useful interim step?
> > 
> > [snip]
> >> -void memory_region_init_iommu(MemoryRegion *mr,
> >> -                              Object *owner,
> >> -                              const MemoryRegionIOMMUOps *ops,
> >> -                              const char *name,
> >> -                              uint64_t size)
> >> +void memory_region_init_iommu_type(const char *mrtypename,
> >> +                                   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;
> >> +    size_t instance_size = object_type_get_instance_size(mrtypename);
> >> +
> >> +    object_initialize(iommu_mr, instance_size, mrtypename);
> > 
> > This looks exceedingly dangerous.  AIUI, the entire purpose of the
> > size parameter to object_initialize() (which can certainly get the
> > instance size from the type, just as you do) is to verify that the
> > buffer you're initializing actually has space for the object type
> > you're putting there.
> > 
> > By looking up the instance size yourself and passing it to
> > object_initialize(), you're disabling that check.
> > 
> > If someone allocates an array of plain IOMMUMemoryRegion structures,
> > then uses this to initialize a derived IOMMU MR type with more fields,
> > the user will get no warning that something is wrong before the memory
> > corruption starts.
> 
> Hm. How can I fix it then for a generic case? Pass the actual amount of
> bytes occupied by *iommu_mr?

I think you need to make the signature similar to
object_initialize().  So instead of taking an IOMMUMemoryRegion *, it
will take a void * and a size_t, which it passes directly to
object_initialize().   A typical caller will then do
	memory_region_init_iommu_type(TYPE_WHATEVER,
				      &mystruct->mr,
				      sizeof(mystruct->mr),
				     ...)

And object_initialize() will verify that the size works for TYPE_WHATEVER.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]