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

[Paolo Bonzini]

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