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 <a...@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. > > 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