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: 3059 bytes --]

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.

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.

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