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.
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.
>
> - const MemoryRegionIOMMUOps *iommu_ops;
>
> const MemoryRegionOps *ops;
> void *opaque;
> @@ -243,6 +246,13 @@ struct MemoryRegion {
> const char *name;
> unsigned ioeventfd_nb;
> MemoryRegionIoeventfd *ioeventfds;
> + bool is_iommu;
Thanks,
Paolo
