Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Cédric Le Goater]

Hello Zhenzhong,

On 3/19/24 12:58, Duan, Zhenzhong wrote:
> Hi Cédric,
>
> > -----Original Message-----
> > From: Cédric Le Goater <clg@redhat.com>
> > Sent: Tuesday, March 19, 2024 4:17 PM
> > To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
> > devel@nongnu.org
> > Cc: alex.williamson@redhat.com; eric.auger@redhat.com;
> > peterx@redhat.com; jasowang@redhat.com; mst@redhat.com;
> > jgg@nvidia.com; nicolinc@nvidia.com; joao.m.martins@oracle.com; Tian,
> > Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Sun, Yi Y
> > <yi.y.sun@intel.com>; Peng, Chao P <chao.p.peng@intel.com>
> > Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
> > HostIOMMUDevice
> >
> > Hello Zhenzhong,
> >
> > On 2/28/24 04:58, Zhenzhong Duan wrote:
> > > HostIOMMUDevice will be inherited by two sub classes,
> > > legacy and iommufd currently.
> > >
> > > Introduce a helper function host_iommu_base_device_init to initialize it.
> > >
> > > Suggested-by: Eric Auger <eric.auger@redhat.com>
> > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > > ---
> > >    include/sysemu/host_iommu_device.h | 22 ++++++++++++++++++++++
> > >    1 file changed, 22 insertions(+)
> > >    create mode 100644 include/sysemu/host_iommu_device.h
> > >
> > > diff --git a/include/sysemu/host_iommu_device.h
> > b/include/sysemu/host_iommu_device.h
> > > new file mode 100644
> > > index 0000000000..fe80ab25fb
> > > --- /dev/null
> > > +++ b/include/sysemu/host_iommu_device.h
> > > @@ -0,0 +1,22 @@
> > > +#ifndef HOST_IOMMU_DEVICE_H
> > > +#define HOST_IOMMU_DEVICE_H
> > > +
> > > +typedef enum HostIOMMUDevice_Type {
> > > +    HID_LEGACY,
> > > +    HID_IOMMUFD,
> > > +    HID_MAX,
> > > +} HostIOMMUDevice_Type;
> > > +
> > > +typedef struct HostIOMMUDevice {
> > > +    HostIOMMUDevice_Type type;
> >
> > A type field is not a good sign and that's where QOM is useful.
>
> Yes, agree.
> I didn't choose QOM because in iommufd-cdev series, VFIOContainer chooses not 
> using QOM model.
> See the discussion: https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
> I thought HostIOMMUDevice need to follow same rule.
>
> But after further digging into this, I think it may be ok to use QOM model as 
> long as we don't expose
> HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE interface. Your 
> thoughts?

yes. Can we change a bit this series to use QOM ? something like :

    typedef struct HostIOMMUDevice {
        Object parent;
    } HostIOMMUDevice;
    
    #define TYPE_HOST_IOMMU "host.iommu"
    OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass, HOST_IOMMU)
    
    struct HostIOMMUClass {
        ObjectClass parent_class;
    
        int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);
        int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);
    };

Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and TYPE_HOST_IOMMU_LEGACY.
Each class implementing the handlers or not (legacy mode).

The class handlers are introduced for the intel-iommu helper vtd_check_hdev()
in order to avoid using iommufd routines directly. HostIOMMUDevice is supposed
to abstract the Host IOMMU device, so we need to abstract also all the
interfaces to this object.

The .host_iommu_device_create() handler could be merged in .attach_device()
possibly. Anyhow, please use now object_new() and object_unref() instead.
host_iommu_base_device_init() is useless IMHO.

> > Is vtd_check_hdev() the only use of this field ?
>
> Currently yes. virtio-iommu may have similar usage.
>
> > If so, can we simplify with a QOM interface in any way ?
>
> QOM interface is a set of callbacks, guess you mean QOM class,
> saying HostIOMMUDevice class, IOMMULegacyDevice class and IOMMUFDDevice class?

See above proposal. it should work fine.

Also, I think it is better to use a IOMMUFDBackend* parameter for
iommufd_device_get_info() to be consistent with the other routines.

Then It would interesting to see how this applies to Eric's series.

Thanks,

C.