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

[Cédric Le Goater]

Hello Zhenzhong,

On 3/28/24 04:06, Duan, Zhenzhong wrote:
> Hi Cédric,
>
> > -----Original Message-----
> > From: Cédric Le Goater <clg@redhat.com>
> > Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
> > HostIOMMUDevice
> >
> > 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).
>
> Understood, thanks for your guide.
>
> > 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.
>
> I'd like to have a minimal adjustment to class handers. Just let me know if you 
> have strong
> preference.
>
> Cap/ecap is intel_iommu specific, I'd like to make it a bit generic also for 
> arm smmu usage,
> and merge get_type and get_cap into one function as they both calls 
> ioctl(IOMMU_GET_HW_INFO),
> something like:
> get_info(HostIOMMUDevice *hiod, enum iommu_hw_info_type *type, void **data, 
> void **len,  Error **errp);

OK. Let's see how it goes. Having more users of this new object Host
IOMMU device is important to get a better feeling of the interface.
As of today, it doesn't have not much value. The iommufd object could
be QOM linked to the vIOMMU when available and we could get the bind
devid in some other ways I suppose. Anyhow, please keep it simple and
let's explore.

Thanks,

C.



> and let iommu emulater to extract content of *data. For intel_iommu, it's:
>
> struct iommu_hw_info_vtd {
>          __u32 flags;
>          __u32 __reserved;
>          __aligned_u64 cap_reg;
>          __aligned_u64 ecap_reg;
> };
>
> > 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.
>
> Good idea, will do.
>
> > > > 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.
>
> Sure, then I'd like to also rename it to iommufd_backend_get_device_info().
>
> Thanks
> Zhenzhong
>
> > Then It would interesting to see how this applies to Eric's series.
> >
> > Thanks,
> >
> > C.