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
signature.asc
Description: PGP signature
