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

[Alexey Kardashevskiy]

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 <address@hidden>
> 
> 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.


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


-- 
Alexey

signature.asc
Description: OpenPGP digital signature