| Message ID | 20240408081230.1030078-3-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a host IOMMU device abstraction | expand |
On 8/4/24 10:12, Zhenzhong Duan wrote: > HIODLegacyVFIO represents a host IOMMU device under VFIO legacy > container backend. > > It includes a link to VFIODevice. > > Suggested-by: Eric Auger <eric.auger@redhat.com> > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 11 +++++++++++ > hw/vfio/container.c | 11 ++++++++++- > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index b9da6c08ef..f30772f534 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -31,6 +31,7 @@ > #endif > #include "sysemu/sysemu.h" > #include "hw/vfio/vfio-container-base.h" > +#include "sysemu/host_iommu_device.h" > > #define VFIO_MSG_PREFIX "vfio %s: " > > @@ -147,6 +148,16 @@ typedef struct VFIOGroup { > bool ram_block_discard_allowed; > } VFIOGroup; > > +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-vfio" > +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) > + > +/* Abstraction of VFIO legacy host IOMMU device */ > +struct HIODLegacyVFIO { > + /*< private >*/ Please drop this comment. > + HostIOMMUDevice parent; Please name 'parent_obj'. > + VFIODevice *vdev; > +};
Hi Philippe, >-----Original Message----- >From: Philippe Mathieu-Daudé <philmd@linaro.org> >Sent: Monday, April 15, 2024 5:20 PM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >devel@nongnu.org >Cc: alex.williamson@redhat.com; clg@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>; Peng, Chao P ><chao.p.peng@intel.com> >Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device > >On 8/4/24 10:12, Zhenzhong Duan wrote: >> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy >> container backend. >> >> It includes a link to VFIODevice. >> >> Suggested-by: Eric Auger <eric.auger@redhat.com> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 11 +++++++++++ >> hw/vfio/container.c | 11 ++++++++++- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >> index b9da6c08ef..f30772f534 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -31,6 +31,7 @@ >> #endif >> #include "sysemu/sysemu.h" >> #include "hw/vfio/vfio-container-base.h" >> +#include "sysemu/host_iommu_device.h" >> >> #define VFIO_MSG_PREFIX "vfio %s: " >> >> @@ -147,6 +148,16 @@ typedef struct VFIOGroup { >> bool ram_block_discard_allowed; >> } VFIOGroup; >> >> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy- >vfio" >> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) >> + >> +/* Abstraction of VFIO legacy host IOMMU device */ >> +struct HIODLegacyVFIO { >> + /*< private >*/ > >Please drop this comment. Will do. But may I ask the rules when to use that comment and when not? I see some QOM use that comment to mark private vs. public, for example: struct AccelState { /*< private >*/ Object parent_obj; }; typedef struct AccelClass { /*< private >*/ ObjectClass parent_class; /*< public >*/ > >> + HostIOMMUDevice parent; > >Please name 'parent_obj'. Will do. Thanks Zhenzhong > >> + VFIODevice *vdev; >> +};
On 15/4/24 12:10, Duan, Zhenzhong wrote: > Hi Philippe, > >> -----Original Message----- >> From: Philippe Mathieu-Daudé <philmd@linaro.org> >> Sent: Monday, April 15, 2024 5:20 PM >> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >> devel@nongnu.org >> Cc: alex.williamson@redhat.com; clg@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>; Peng, Chao P >> <chao.p.peng@intel.com> >> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device >> >> On 8/4/24 10:12, Zhenzhong Duan wrote: >>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy >>> container backend. >>> >>> It includes a link to VFIODevice. >>> >>> Suggested-by: Eric Auger <eric.auger@redhat.com> >>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/vfio/vfio-common.h | 11 +++++++++++ >>> hw/vfio/container.c | 11 ++++++++++- >>> 2 files changed, 21 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >> common.h >>> index b9da6c08ef..f30772f534 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -31,6 +31,7 @@ >>> #endif >>> #include "sysemu/sysemu.h" >>> #include "hw/vfio/vfio-container-base.h" >>> +#include "sysemu/host_iommu_device.h" >>> >>> #define VFIO_MSG_PREFIX "vfio %s: " >>> >>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup { >>> bool ram_block_discard_allowed; >>> } VFIOGroup; >>> >>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy- >> vfio" >>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) >>> + >>> +/* Abstraction of VFIO legacy host IOMMU device */ >>> +struct HIODLegacyVFIO { >>> + /*< private >*/ >> >> Please drop this comment. > > Will do. But may I ask the rules when to use that comment and when not? Sure, see https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations > I see some QOM use that comment to mark private vs. public, for example: > > struct AccelState { > /*< private >*/ > Object parent_obj; This is old style which might be cleaned some day... > }; > > typedef struct AccelClass { > /*< private >*/ > ObjectClass parent_class; > /*< public >*/ > >> >>> + HostIOMMUDevice parent; >> >> Please name 'parent_obj'. > > Will do. Thanks, Phil. > > Thanks > Zhenzhong > >> >>> + VFIODevice *vdev; >>> +}; >
>-----Original Message----- >From: Philippe Mathieu-Daudé <philmd@linaro.org> >Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device > >On 15/4/24 12:10, Duan, Zhenzhong wrote: >> Hi Philippe, >> >>> -----Original Message----- >>> From: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Sent: Monday, April 15, 2024 5:20 PM >>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >>> devel@nongnu.org >>> Cc: alex.williamson@redhat.com; clg@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>; Peng, Chao P >>> <chao.p.peng@intel.com> >>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device >>> >>> On 8/4/24 10:12, Zhenzhong Duan wrote: >>>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy >>>> container backend. >>>> >>>> It includes a link to VFIODevice. >>>> >>>> Suggested-by: Eric Auger <eric.auger@redhat.com> >>>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> include/hw/vfio/vfio-common.h | 11 +++++++++++ >>>> hw/vfio/container.c | 11 ++++++++++- >>>> 2 files changed, 21 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >>> common.h >>>> index b9da6c08ef..f30772f534 100644 >>>> --- a/include/hw/vfio/vfio-common.h >>>> +++ b/include/hw/vfio/vfio-common.h >>>> @@ -31,6 +31,7 @@ >>>> #endif >>>> #include "sysemu/sysemu.h" >>>> #include "hw/vfio/vfio-container-base.h" >>>> +#include "sysemu/host_iommu_device.h" >>>> >>>> #define VFIO_MSG_PREFIX "vfio %s: " >>>> >>>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup { >>>> bool ram_block_discard_allowed; >>>> } VFIOGroup; >>>> >>>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "- >legacy- >>> vfio" >>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) >>>> + >>>> +/* Abstraction of VFIO legacy host IOMMU device */ >>>> +struct HIODLegacyVFIO { >>>> + /*< private >*/ >>> >>> Please drop this comment. >> >> Will do. But may I ask the rules when to use that comment and when not? > >Sure, see >https://www.qemu.org/docs/master/devel/style.html#qemu-object-model- >declarations Learned, thanks Philippe. BRs. Zhenzhong
On 4/8/24 10:12, Zhenzhong Duan wrote: > HIODLegacyVFIO represents a host IOMMU device under VFIO legacy > container backend. > > It includes a link to VFIODevice. > > Suggested-by: Eric Auger <eric.auger@redhat.com> > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 11 +++++++++++ > hw/vfio/container.c | 11 ++++++++++- > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index b9da6c08ef..f30772f534 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -31,6 +31,7 @@ > #endif > #include "sysemu/sysemu.h" > #include "hw/vfio/vfio-container-base.h" > +#include "sysemu/host_iommu_device.h" > > #define VFIO_MSG_PREFIX "vfio %s: " > > @@ -147,6 +148,16 @@ typedef struct VFIOGroup { > bool ram_block_discard_allowed; > } VFIOGroup; > > +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-vfio" I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE. > +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) > + > +/* Abstraction of VFIO legacy host IOMMU device */ > +struct HIODLegacyVFIO { same here > + /*< private >*/ > + HostIOMMUDevice parent; > + VFIODevice *vdev; It seems to me that the back pointer should be on the container instead. Looks more correct conceptually. > +}; > + > typedef struct VFIODMABuf { > QemuDmaBuf buf; > uint32_t pos_x, pos_y, pos_updates; > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index 77bdec276e..44018ef085 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -1143,12 +1143,21 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) > vioc->pci_hot_reset = vfio_legacy_pci_hot_reset; > }; > > +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data) > +{ > +}; Is it preferable to introduce routines when they are actually useful. Please drop the .class_init definition. Thanks, C. > + > static const TypeInfo types[] = { > { > .name = TYPE_VFIO_IOMMU_LEGACY, > .parent = TYPE_VFIO_IOMMU, > .class_init = vfio_iommu_legacy_class_init, > - }, > + }, { > + .name = TYPE_HIOD_LEGACY_VFIO, > + .parent = TYPE_HOST_IOMMU_DEVICE, > + .instance_size = sizeof(HIODLegacyVFIO), > + .class_init = hiod_legacy_vfio_class_init, > + } > }; > > DEFINE_TYPES(types)
Hi Cédric, >-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device > >On 4/8/24 10:12, Zhenzhong Duan wrote: >> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy >> container backend. >> >> It includes a link to VFIODevice. >> >> Suggested-by: Eric Auger <eric.auger@redhat.com> >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 11 +++++++++++ >> hw/vfio/container.c | 11 ++++++++++- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >> index b9da6c08ef..f30772f534 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -31,6 +31,7 @@ >> #endif >> #include "sysemu/sysemu.h" >> #include "hw/vfio/vfio-container-base.h" >> +#include "sysemu/host_iommu_device.h" >> >> #define VFIO_MSG_PREFIX "vfio %s: " >> >> @@ -147,6 +148,16 @@ typedef struct VFIOGroup { >> bool ram_block_discard_allowed; >> } VFIOGroup; >> >> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy- >vfio" > >I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE. Will do. > >> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) >> + >> +/* Abstraction of VFIO legacy host IOMMU device */ >> +struct HIODLegacyVFIO { > >same here Should I do the same for all the HostIOMMUDevice and HostIOMMUDeviceClass sub-structures? The reason I used 'HIOD' abbreviation is some function names become extremely long and exceed 80 characters. E.g.: @@ -1148,9 +1148,9 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) vioc->pci_hot_reset = vfio_legacy_pci_hot_reset; }; -static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod, - void *data, uint32_t len, - Error **errp) +static int host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod, + void *data, uint32_t len, + Error **errp) { VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev; /* iova_ranges is a sorted list */ @@ -1173,7 +1173,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data) { HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); - hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info; + hioc->get_host_iommu_info = host_iommu_device_legacy_vfio_get_host_iommu_info; }; I didn't find other way to make it meet the 80 chars limitation. Any suggestions on this? > >> + /*< private >*/ >> + HostIOMMUDevice parent; >> + VFIODevice *vdev; > >It seems to me that the back pointer should be on the container instead. >Looks more correct conceptually. Yes, that makes sense for legacy VFIO, as iova_ranges, pgsizes etc are all saved in bcontainer. > > >> +}; >> + >> typedef struct VFIODMABuf { >> QemuDmaBuf buf; >> uint32_t pos_x, pos_y, pos_updates; >> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >> index 77bdec276e..44018ef085 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -1143,12 +1143,21 @@ static void >vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) >> vioc->pci_hot_reset = vfio_legacy_pci_hot_reset; >> }; >> >> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data) >> +{ >> +}; > >Is it preferable to introduce routines when they are actually useful. >Please drop the .class_init definition. Sure. Thanks Zhenzhong > >Thanks, > >C. > > >> + >> static const TypeInfo types[] = { >> { >> .name = TYPE_VFIO_IOMMU_LEGACY, >> .parent = TYPE_VFIO_IOMMU, >> .class_init = vfio_iommu_legacy_class_init, >> - }, >> + }, { >> + .name = TYPE_HIOD_LEGACY_VFIO, >> + .parent = TYPE_HOST_IOMMU_DEVICE, >> + .instance_size = sizeof(HIODLegacyVFIO), >> + .class_init = hiod_legacy_vfio_class_init, >> + } >> }; >> >> DEFINE_TYPES(types)
Hello, On 4/16/24 05:41, Duan, Zhenzhong wrote: > Hi Cédric, > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device >> >> On 4/8/24 10:12, Zhenzhong Duan wrote: >>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy >>> container backend. >>> >>> It includes a link to VFIODevice. >>> >>> Suggested-by: Eric Auger <eric.auger@redhat.com> >>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/vfio/vfio-common.h | 11 +++++++++++ >>> hw/vfio/container.c | 11 ++++++++++- >>> 2 files changed, 21 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >> common.h >>> index b9da6c08ef..f30772f534 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -31,6 +31,7 @@ >>> #endif >>> #include "sysemu/sysemu.h" >>> #include "hw/vfio/vfio-container-base.h" >>> +#include "sysemu/host_iommu_device.h" >>> >>> #define VFIO_MSG_PREFIX "vfio %s: " >>> >>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup { >>> bool ram_block_discard_allowed; >>> } VFIOGroup; >>> >>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy- >> vfio" >> >> I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE. > > Will do. > >> >>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) >>> + >>> +/* Abstraction of VFIO legacy host IOMMU device */ >>> +struct HIODLegacyVFIO { >> >> same here > > Should I do the same for all the HostIOMMUDevice and HostIOMMUDeviceClass sub-structures? I would for type names. The main reason is for naming consistency, which is useful for grep and code analysis. > > The reason I used 'HIOD' abbreviation is some function names become extremely long > and exceed 80 characters. E.g.: > > @@ -1148,9 +1148,9 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) > vioc->pci_hot_reset = vfio_legacy_pci_hot_reset; > }; > > -static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod, > - void *data, uint32_t len, > - Error **errp) > +static int host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod, > + void *data, uint32_t len, > + Error **errp) > { > VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev; > /* iova_ranges is a sorted list */ > @@ -1173,7 +1173,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data) > { > HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); > > - hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info; > + hioc->get_host_iommu_info = host_iommu_device_legacy_vfio_get_host_iommu_info; > }; > > I didn't find other way to make it meet the 80 chars limitation. Any suggestions on this? Try : @@ -1177,7 +1177,8 @@ static void hiod_legacy_vfio_class_init( { HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); - hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info; + hioc->get_host_iommu_info = + host_iommu_device_legacy_vfio_get_host_iommu_info; }; static const TypeInfo types[] = { That said, I agree that 'host_iommu_device_legacy_vfio' routine prefix could be shortened to 'hiod_legacy_vfio'. Thanks, C. > >> >>> + /*< private >*/ >>> + HostIOMMUDevice parent; >>> + VFIODevice *vdev; >> >> It seems to me that the back pointer should be on the container instead. >> Looks more correct conceptually. > > Yes, that makes sense for legacy VFIO, as iova_ranges, pgsizes etc are all saved in bcontainer. > >> >> >>> +}; >>> + >>> typedef struct VFIODMABuf { >>> QemuDmaBuf buf; >>> uint32_t pos_x, pos_y, pos_updates; >>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >>> index 77bdec276e..44018ef085 100644 >>> --- a/hw/vfio/container.c >>> +++ b/hw/vfio/container.c >>> @@ -1143,12 +1143,21 @@ static void >> vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) >>> vioc->pci_hot_reset = vfio_legacy_pci_hot_reset; >>> }; >>> >>> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data) >>> +{ >>> +}; >> >> Is it preferable to introduce routines when they are actually useful. >> Please drop the .class_init definition. > > Sure. > > Thanks > Zhenzhong > >> >> Thanks, >> >> C. >> >> >>> + >>> static const TypeInfo types[] = { >>> { >>> .name = TYPE_VFIO_IOMMU_LEGACY, >>> .parent = TYPE_VFIO_IOMMU, >>> .class_init = vfio_iommu_legacy_class_init, >>> - }, >>> + }, { >>> + .name = TYPE_HIOD_LEGACY_VFIO, >>> + .parent = TYPE_HOST_IOMMU_DEVICE, >>> + .instance_size = sizeof(HIODLegacyVFIO), >>> + .class_init = hiod_legacy_vfio_class_init, >>> + } >>> }; >>> >>> DEFINE_TYPES(types) >
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device > >Hello, > >On 4/16/24 05:41, Duan, Zhenzhong wrote: >> Hi Cédric, >> >>> -----Original Message----- >>> From: Cédric Le Goater <clg@redhat.com> >>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device >>> >>> On 4/8/24 10:12, Zhenzhong Duan wrote: >>>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy >>>> container backend. >>>> >>>> It includes a link to VFIODevice. >>>> >>>> Suggested-by: Eric Auger <eric.auger@redhat.com> >>>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> include/hw/vfio/vfio-common.h | 11 +++++++++++ >>>> hw/vfio/container.c | 11 ++++++++++- >>>> 2 files changed, 21 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >>> common.h >>>> index b9da6c08ef..f30772f534 100644 >>>> --- a/include/hw/vfio/vfio-common.h >>>> +++ b/include/hw/vfio/vfio-common.h >>>> @@ -31,6 +31,7 @@ >>>> #endif >>>> #include "sysemu/sysemu.h" >>>> #include "hw/vfio/vfio-container-base.h" >>>> +#include "sysemu/host_iommu_device.h" >>>> >>>> #define VFIO_MSG_PREFIX "vfio %s: " >>>> >>>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup { >>>> bool ram_block_discard_allowed; >>>> } VFIOGroup; >>>> >>>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "- >legacy- >>> vfio" >>> >>> I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE. >> >> Will do. >> >>> >>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) >>>> + >>>> +/* Abstraction of VFIO legacy host IOMMU device */ >>>> +struct HIODLegacyVFIO { >>> >>> same here >> >> Should I do the same for all the HostIOMMUDevice and >HostIOMMUDeviceClass sub-structures? > >I would for type names. The main reason is for naming consistency, which is >useful for grep and code analysis. Got it. > >> >> The reason I used 'HIOD' abbreviation is some function names become >extremely long >> and exceed 80 characters. E.g.: >> >> @@ -1148,9 +1148,9 @@ static void >vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) >> vioc->pci_hot_reset = vfio_legacy_pci_hot_reset; >> }; >> >> -static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice >*hiod, >> - void *data, uint32_t len, >> - Error **errp) >> +static int >host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice >*hiod, >> + void *data, uint32_t len, >> + Error **errp) >> { >> VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev; >> /* iova_ranges is a sorted list */ >> @@ -1173,7 +1173,7 @@ static void >hiod_legacy_vfio_class_init(ObjectClass *oc, void *data) >> { >> HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); >> >> - hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info; >> + hioc->get_host_iommu_info = >host_iommu_device_legacy_vfio_get_host_iommu_info; >> }; >> >> I didn't find other way to make it meet the 80 chars limitation. Any >suggestions on this? > >Try : > >@@ -1177,7 +1177,8 @@ static void hiod_legacy_vfio_class_init( > { > HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); > >- hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info; >+ hioc->get_host_iommu_info = >+ host_iommu_device_legacy_vfio_get_host_iommu_info; > }; > > static const TypeInfo types[] = { > >That said, I agree that 'host_iommu_device_legacy_vfio' routine prefix >could be shortened to 'hiod_legacy_vfio'. Got it. Thanks Zhenzhong
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index b9da6c08ef..f30772f534 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -31,6 +31,7 @@ #endif #include "sysemu/sysemu.h" #include "hw/vfio/vfio-container-base.h" +#include "sysemu/host_iommu_device.h" #define VFIO_MSG_PREFIX "vfio %s: " @@ -147,6 +148,16 @@ typedef struct VFIOGroup { bool ram_block_discard_allowed; } VFIOGroup; +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-vfio" +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO) + +/* Abstraction of VFIO legacy host IOMMU device */ +struct HIODLegacyVFIO { + /*< private >*/ + HostIOMMUDevice parent; + VFIODevice *vdev; +}; + typedef struct VFIODMABuf { QemuDmaBuf buf; uint32_t pos_x, pos_y, pos_updates; diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 77bdec276e..44018ef085 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -1143,12 +1143,21 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data) vioc->pci_hot_reset = vfio_legacy_pci_hot_reset; }; +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data) +{ +}; + static const TypeInfo types[] = { { .name = TYPE_VFIO_IOMMU_LEGACY, .parent = TYPE_VFIO_IOMMU, .class_init = vfio_iommu_legacy_class_init, - }, + }, { + .name = TYPE_HIOD_LEGACY_VFIO, + .parent = TYPE_HOST_IOMMU_DEVICE, + .instance_size = sizeof(HIODLegacyVFIO), + .class_init = hiod_legacy_vfio_class_init, + } }; DEFINE_TYPES(types)
HIODLegacyVFIO represents a host IOMMU device under VFIO legacy container backend. It includes a link to VFIODevice. Suggested-by: Eric Auger <eric.auger@redhat.com> Suggested-by: Cédric Le Goater <clg@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/hw/vfio/vfio-common.h | 11 +++++++++++ hw/vfio/container.c | 11 ++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-)