| Message ID | 20240228035900.1085727-2-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a host IOMMU device abstraction | expand |
Hi Zhenzhong, On 2/28/24 04:58, Zhenzhong Duan wrote: > HostIOMMUDevice will be inherited by two sub classes, > legacy and iommufd currently. As this patch introduces the object, you describe what the object is meant for and used for. Maybe reuse text from the cover letter Thanks Eric > > 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; > + size_t size; > +} HostIOMMUDevice; > + > +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev, > + HostIOMMUDevice_Type type, > + size_t size) > +{ > + dev->type = type; > + dev->size = size; > +} > +#endif
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct >HostIOMMUDevice > >Hi Zhenzhong, >On 2/28/24 04:58, Zhenzhong Duan wrote: >> HostIOMMUDevice will be inherited by two sub classes, >> legacy and iommufd currently. >As this patch introduces the object, you describe what the object is >meant for and used for. Maybe reuse text from the cover letter Sure, will do. Thanks Zhenzhong > >Thanks > >Eric >> >> 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; >> + size_t size; >> +} HostIOMMUDevice; >> + >> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev, >> + HostIOMMUDevice_Type type, >> + size_t size) >> +{ >> + dev->type = type; >> + dev->size = size; >> +} >> +#endif
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. Is vtd_check_hdev() the only use of this field ? If so, can we simplify with a QOM interface in any way ? Thanks, C. > + size_t size; > +} HostIOMMUDevice; > + > +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev, > + HostIOMMUDevice_Type type, > + size_t size) > +{ > + dev->type = type; > + dev->size = size; > +} > +#endif
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? > >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? Thanks Zhenzhong > >Thanks, > >C. > > > > >> + size_t size; >> +} HostIOMMUDevice; >> + >> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev, >> + HostIOMMUDevice_Type type, >> + size_t size) >> +{ >> + dev->type = type; >> + dev->size = size; >> +} >> +#endif
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.
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); 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. > >
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. >> >> >
>-----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/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. Got it, thanks Cédric! BRs. Zhenzhong
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; + size_t size; +} HostIOMMUDevice; + +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev, + HostIOMMUDevice_Type type, + size_t size) +{ + dev->type = type; + dev->size = size; +} +#endif
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