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

On Thu, Jun 08, 2017 at 03:59:27PM +0200, Paolo Bonzini wrote:
> 
> 
> On 08/06/2017 09:42, David Gibson wrote:
> > 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?
> 
> Well, it's definitely better than nothing, and MR already relies heavily
> on the ops pattern.

Hm, fair point.

> The only changes I'd make are:
> 
> 1) remove this unnecessary hunk
> 
> >      snprintf(tmp, sizeof(tmp), "tce-iommu-%x", tcet->liobn);
> > -    memory_region_init_iommu(&tcet->iommu, tcetobj, &spapr_iommu_ops, tmp, 0);
> > +    memory_region_init_iommu_type(TYPE_IOMMU_MEMORY_REGION,
> > +                                  &tcet->iommu, tcetobj, &spapr_iommu_ops,
> > +                                  tmp, 0);
> 
> thus delaying the introduction of memory_region_init_iommu_type to the
> next step.
> 
> 2) Leave is_iommu early in the "should fit in a cache line" part, for
> example after dirty_log_mask, and since we have room move "bool
> ram_device;" there too.

I really think the unsafe use of object_initialize() needs to be fixed
as well.

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