| Message ID | 20240429065046.3688701-12-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a host IOMMU device abstraction to check with vIOMMU | expand |
On 4/29/24 08:50, Zhenzhong Duan wrote: > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > backends/iommufd.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/backends/iommufd.c b/backends/iommufd.c > index d61209788a..28faec528e 100644 > --- a/backends/iommufd.c > +++ b/backends/iommufd.c > @@ -233,6 +233,23 @@ int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, > return ret; > } > > +static int hiod_iommufd_check_cap(HostIOMMUDevice *hiod, int cap, Error **errp) > +{ > + switch (cap) { > + case HOST_IOMMU_DEVICE_CAP_IOMMUFD: > + return 1; I don't understand this value. Thanks, C. > + default: > + return host_iommu_device_check_cap_common(hiod, cap, errp); > + } > +} > + > +static void hiod_iommufd_class_init(ObjectClass *oc, void *data) > +{ > + HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); > + > + hioc->check_cap = hiod_iommufd_check_cap; > +}; > + > static const TypeInfo types[] = { > { > .name = TYPE_IOMMUFD_BACKEND, > @@ -251,6 +268,7 @@ static const TypeInfo types[] = { > .parent = TYPE_HOST_IOMMU_DEVICE, > .instance_size = sizeof(HostIOMMUDeviceIOMMUFD), > .class_size = sizeof(HostIOMMUDeviceIOMMUFDClass), > + .class_init = hiod_iommufd_class_init, > .abstract = true, > } > };
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v3 11/19] backends/iommufd: Implement >HostIOMMUDeviceClass::check_cap() handler > >On 4/29/24 08:50, Zhenzhong Duan wrote: >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> backends/iommufd.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/backends/iommufd.c b/backends/iommufd.c >> index d61209788a..28faec528e 100644 >> --- a/backends/iommufd.c >> +++ b/backends/iommufd.c >> @@ -233,6 +233,23 @@ int >iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, >> return ret; >> } >> >> +static int hiod_iommufd_check_cap(HostIOMMUDevice *hiod, int cap, >Error **errp) >> +{ >> + switch (cap) { >> + case HOST_IOMMU_DEVICE_CAP_IOMMUFD: >> + return 1; > >I don't understand this value. 1 means this host iommu device is attached to IOMMUFD backend, or else 0 if attached to legacy backend. Strictly speaking, HOST_IOMMU_DEVICE_CAP_IOMMUFD is not a hardware capability, I'm trying to put all(sw/hw) in CAPs checking framework just like KVM<->qemu CAPs does. Thanks Zhenzhong > > >Thanks, > >C. > > >> + default: >> + return host_iommu_device_check_cap_common(hiod, cap, errp); >> + } >> +} >> + >> +static void hiod_iommufd_class_init(ObjectClass *oc, void *data) >> +{ >> + HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); >> + >> + hioc->check_cap = hiod_iommufd_check_cap; >> +}; >> + >> static const TypeInfo types[] = { >> { >> .name = TYPE_IOMMUFD_BACKEND, >> @@ -251,6 +268,7 @@ static const TypeInfo types[] = { >> .parent = TYPE_HOST_IOMMU_DEVICE, >> .instance_size = sizeof(HostIOMMUDeviceIOMMUFD), >> .class_size = sizeof(HostIOMMUDeviceIOMMUFDClass), >> + .class_init = hiod_iommufd_class_init, >> .abstract = true, >> } >> };
On 4/30/24 12:06, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Subject: Re: [PATCH v3 11/19] backends/iommufd: Implement >> HostIOMMUDeviceClass::check_cap() handler >> >> On 4/29/24 08:50, Zhenzhong Duan wrote: >>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> backends/iommufd.c | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/backends/iommufd.c b/backends/iommufd.c >>> index d61209788a..28faec528e 100644 >>> --- a/backends/iommufd.c >>> +++ b/backends/iommufd.c >>> @@ -233,6 +233,23 @@ int >> iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, >>> return ret; >>> } >>> >>> +static int hiod_iommufd_check_cap(HostIOMMUDevice *hiod, int cap, >> Error **errp) >>> +{ >>> + switch (cap) { >>> + case HOST_IOMMU_DEVICE_CAP_IOMMUFD: >>> + return 1; >> >> I don't understand this value. > > 1 means this host iommu device is attached to IOMMUFD backend, > or else 0 if attached to legacy backend. Hmm, this looks hacky to me and it is not used anywhere in the patchset. Let's reconsider when there is actually a use for it. Until then, please drop. My feeling is that a new HostIOMMUDeviceClass handler/attributed should be introduced instead. Thanks, C. > Strictly speaking, HOST_IOMMU_DEVICE_CAP_IOMMUFD is not a > hardware capability, I'm trying to put all(sw/hw) in CAPs checking > framework just like KVM<->qemu CAPs does. > > Thanks > Zhenzhong > >> >> >> Thanks, >> >> C. >> >> >>> + default: >>> + return host_iommu_device_check_cap_common(hiod, cap, errp); >>> + } >>> +} >>> + >>> +static void hiod_iommufd_class_init(ObjectClass *oc, void *data) >>> +{ >>> + HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); >>> + >>> + hioc->check_cap = hiod_iommufd_check_cap; >>> +}; >>> + >>> static const TypeInfo types[] = { >>> { >>> .name = TYPE_IOMMUFD_BACKEND, >>> @@ -251,6 +268,7 @@ static const TypeInfo types[] = { >>> .parent = TYPE_HOST_IOMMU_DEVICE, >>> .instance_size = sizeof(HostIOMMUDeviceIOMMUFD), >>> .class_size = sizeof(HostIOMMUDeviceIOMMUFDClass), >>> + .class_init = hiod_iommufd_class_init, >>> .abstract = true, >>> } >>> }; >
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v3 11/19] backends/iommufd: Implement >HostIOMMUDeviceClass::check_cap() handler > >On 4/30/24 12:06, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Cédric Le Goater <clg@redhat.com> >>> Subject: Re: [PATCH v3 11/19] backends/iommufd: Implement >>> HostIOMMUDeviceClass::check_cap() handler >>> >>> On 4/29/24 08:50, Zhenzhong Duan wrote: >>>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> backends/iommufd.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/backends/iommufd.c b/backends/iommufd.c >>>> index d61209788a..28faec528e 100644 >>>> --- a/backends/iommufd.c >>>> +++ b/backends/iommufd.c >>>> @@ -233,6 +233,23 @@ int >>> iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t >devid, >>>> return ret; >>>> } >>>> >>>> +static int hiod_iommufd_check_cap(HostIOMMUDevice *hiod, int cap, >>> Error **errp) >>>> +{ >>>> + switch (cap) { >>>> + case HOST_IOMMU_DEVICE_CAP_IOMMUFD: >>>> + return 1; >>> >>> I don't understand this value. >> >> 1 means this host iommu device is attached to IOMMUFD backend, >> or else 0 if attached to legacy backend. > >Hmm, this looks hacky to me and it is not used anywhere in the patchset. >Let's reconsider when there is actually a use for it. Until then, please >drop. My feeling is that a new HostIOMMUDeviceClass handler/attributed >should be introduced instead. Got it, will drop it in this series. Is "return 1" directly the concern on your side? If yes, what about adding a new element be_type which can be initialized in realize(), like below: --- a/include/sysemu/host_iommu_device.h +++ b/include/sysemu/host_iommu_device.h @@ -28,6 +28,9 @@ * @fs1gp: first stage(a.k.a, Stage-1) 1GB huge page support. */ typedef struct HostIOMMUDeviceCaps { +#define HOST_IOMMU_DEVICE_CAP_BACKEND_LEGACY 0 +#define HOST_IOMMU_DEVICE_CAP_BACKEND_IOMMUFD 1 + uint32_t be_type; enum iommu_hw_info_type type; uint8_t aw_bits; bool nesting; @@ -91,7 +94,7 @@ struct HostIOMMUDeviceClass { /* * Host IOMMU device capability list. */ -#define HOST_IOMMU_DEVICE_CAP_IOMMUFD 0 +#define HOST_IOMMU_DEVICE_CAP_BACKEND_TYPE 0 #define HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE 1 #define HOST_IOMMU_DEVICE_CAP_AW_BITS 2 #define HOST_IOMMU_DEVICE_CAP_NESTING 3 This looks a bit simpler than adding another handler. Or you have other concern? Thanks Zhenzhong > > >Thanks, > >C. > > > >> Strictly speaking, HOST_IOMMU_DEVICE_CAP_IOMMUFD is not a >> hardware capability, I'm trying to put all(sw/hw) in CAPs checking >> framework just like KVM<->qemu CAPs does. >> >> Thanks >> Zhenzhong >> >>> >>> >>> Thanks, >>> >>> C. >>> >>> >>>> + default: >>>> + return host_iommu_device_check_cap_common(hiod, cap, errp); >>>> + } >>>> +} >>>> + >>>> +static void hiod_iommufd_class_init(ObjectClass *oc, void *data) >>>> +{ >>>> + HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); >>>> + >>>> + hioc->check_cap = hiod_iommufd_check_cap; >>>> +}; >>>> + >>>> static const TypeInfo types[] = { >>>> { >>>> .name = TYPE_IOMMUFD_BACKEND, >>>> @@ -251,6 +268,7 @@ static const TypeInfo types[] = { >>>> .parent = TYPE_HOST_IOMMU_DEVICE, >>>> .instance_size = sizeof(HostIOMMUDeviceIOMMUFD), >>>> .class_size = sizeof(HostIOMMUDeviceIOMMUFDClass), >>>> + .class_init = hiod_iommufd_class_init, >>>> .abstract = true, >>>> } >>>> }; >>
>>>>> +static int hiod_iommufd_check_cap(HostIOMMUDevice *hiod, int cap, >>>> Error **errp) >>>>> +{ >>>>> + switch (cap) { >>>>> + case HOST_IOMMU_DEVICE_CAP_IOMMUFD: >>>>> + return 1; >>>> >>>> I don't understand this value. >>> >>> 1 means this host iommu device is attached to IOMMUFD backend, >>> or else 0 if attached to legacy backend. >> >> Hmm, this looks hacky to me and it is not used anywhere in the patchset. >> Let's reconsider when there is actually a use for it. Until then, please >> drop. My feeling is that a new HostIOMMUDeviceClass handler/attributed >> should be introduced instead. > > Got it, will drop it in this series. > > Is "return 1" directly the concern on your side? I don't know yet why the implementation would need to know if the host IOMMU device is of type IOMMUFD. If that's the case, there are alternative ways, like using OBJECT_CHECK( ..., TYPE_HOST_IOMMU_DEVICE_IOMMUFD) or a class attribute defined at build time but that's a bit the same. Let's see when the need arises. Thanks, C.
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v3 11/19] backends/iommufd: Implement >HostIOMMUDeviceClass::check_cap() handler > >>>>>> +static int hiod_iommufd_check_cap(HostIOMMUDevice *hiod, int >cap, >>>>> Error **errp) >>>>>> +{ >>>>>> + switch (cap) { >>>>>> + case HOST_IOMMU_DEVICE_CAP_IOMMUFD: >>>>>> + return 1; >>>>> >>>>> I don't understand this value. >>>> >>>> 1 means this host iommu device is attached to IOMMUFD backend, >>>> or else 0 if attached to legacy backend. >>> >>> Hmm, this looks hacky to me and it is not used anywhere in the patchset. >>> Let's reconsider when there is actually a use for it. Until then, please >>> drop. My feeling is that a new HostIOMMUDeviceClass >handler/attributed >>> should be introduced instead. >> >> Got it, will drop it in this series. >> >> Is "return 1" directly the concern on your side? > >I don't know yet why the implementation would need to know if the host >IOMMU device is of type IOMMUFD. If that's the case, there are alternative >ways, like using OBJECT_CHECK( ..., TYPE_HOST_IOMMU_DEVICE_IOMMUFD) >or >a class attribute defined at build time but that's a bit the same. Let's >see when the need arises. Got it, let's revisit it in nesting series, will drop it for now. Thanks Zhenzhong
diff --git a/backends/iommufd.c b/backends/iommufd.c index d61209788a..28faec528e 100644 --- a/backends/iommufd.c +++ b/backends/iommufd.c @@ -233,6 +233,23 @@ int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, return ret; } +static int hiod_iommufd_check_cap(HostIOMMUDevice *hiod, int cap, Error **errp) +{ + switch (cap) { + case HOST_IOMMU_DEVICE_CAP_IOMMUFD: + return 1; + default: + return host_iommu_device_check_cap_common(hiod, cap, errp); + } +} + +static void hiod_iommufd_class_init(ObjectClass *oc, void *data) +{ + HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); + + hioc->check_cap = hiod_iommufd_check_cap; +}; + static const TypeInfo types[] = { { .name = TYPE_IOMMUFD_BACKEND, @@ -251,6 +268,7 @@ static const TypeInfo types[] = { .parent = TYPE_HOST_IOMMU_DEVICE, .instance_size = sizeof(HostIOMMUDeviceIOMMUFD), .class_size = sizeof(HostIOMMUDeviceIOMMUFDClass), + .class_init = hiod_iommufd_class_init, .abstract = true, } };
Suggested-by: Cédric Le Goater <clg@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- backends/iommufd.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)