| Message ID | 20240603061023.269738-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 6/3/24 08:10, Zhenzhong Duan wrote: > Suggested-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > backends/iommufd.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/backends/iommufd.c b/backends/iommufd.c > index c7e969d6f7..f2f7a762a0 100644 > --- a/backends/iommufd.c > +++ b/backends/iommufd.c > @@ -230,6 +230,28 @@ bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, > return true; > } > > +static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp) > +{ > + HostIOMMUDeviceCaps *caps = &hiod->caps; > + > + switch (cap) { > + case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE: > + return caps->type; > + case HOST_IOMMU_DEVICE_CAP_AW_BITS: > + return caps->aw_bits; > + default: > + error_setg(errp, "Not support get cap %x", cap); can't you add details about the faulting HostIOMMUDevice by tracing the devid for instance? I would rephrase the error message into No support for capability 0x%x Eric > + return -EINVAL; > + } > +} > + > +static void hiod_iommufd_class_init(ObjectClass *oc, void *data) > +{ > + HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); > + > + hioc->get_cap = hiod_iommufd_get_cap; > +}; > + > static const TypeInfo types[] = { > { > .name = TYPE_IOMMUFD_BACKEND, > @@ -246,6 +268,7 @@ static const TypeInfo types[] = { > }, { > .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, > .parent = TYPE_HOST_IOMMU_DEVICE, > + .class_init = hiod_iommufd_class_init, > .abstract = true, > } > };
On 6/3/24 13:32, Eric Auger wrote: > > > On 6/3/24 08:10, Zhenzhong Duan wrote: >> Suggested-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> backends/iommufd.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/backends/iommufd.c b/backends/iommufd.c >> index c7e969d6f7..f2f7a762a0 100644 >> --- a/backends/iommufd.c >> +++ b/backends/iommufd.c >> @@ -230,6 +230,28 @@ bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, >> return true; >> } >> >> +static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp) >> +{ >> + HostIOMMUDeviceCaps *caps = &hiod->caps; >> + >> + switch (cap) { >> + case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE: >> + return caps->type; >> + case HOST_IOMMU_DEVICE_CAP_AW_BITS: >> + return caps->aw_bits; >> + default: >> + error_setg(errp, "Not support get cap %x", cap); > can't you add details about the faulting HostIOMMUDevice by tracing the > devid for instance? yes. > I would rephrase the error message into No support for capability 0x%x I was going to propose "Unsupported capability ..." Thanks, C. > > Eric >> + return -EINVAL; >> + } >> +} >> + >> +static void hiod_iommufd_class_init(ObjectClass *oc, void *data) >> +{ >> + HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); >> + >> + hioc->get_cap = hiod_iommufd_get_cap; >> +}; >> + >> static const TypeInfo types[] = { >> { >> .name = TYPE_IOMMUFD_BACKEND, >> @@ -246,6 +268,7 @@ static const TypeInfo types[] = { >> }, { >> .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, >> .parent = TYPE_HOST_IOMMU_DEVICE, >> + .class_init = hiod_iommufd_class_init, >> .abstract = true, >> } >> }; >
Hi Cédric, Eric, >-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Subject: Re: [PATCH v6 11/19] backends/iommufd: Implement >HostIOMMUDeviceClass::get_cap() handler > >On 6/3/24 13:32, Eric Auger wrote: >> >> >> On 6/3/24 08:10, Zhenzhong Duan wrote: >>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> backends/iommufd.c | 23 +++++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >>> >>> diff --git a/backends/iommufd.c b/backends/iommufd.c >>> index c7e969d6f7..f2f7a762a0 100644 >>> --- a/backends/iommufd.c >>> +++ b/backends/iommufd.c >>> @@ -230,6 +230,28 @@ bool >iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, >>> return true; >>> } >>> >>> +static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, >Error **errp) >>> +{ >>> + HostIOMMUDeviceCaps *caps = &hiod->caps; >>> + >>> + switch (cap) { >>> + case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE: >>> + return caps->type; >>> + case HOST_IOMMU_DEVICE_CAP_AW_BITS: >>> + return caps->aw_bits; >>> + default: >>> + error_setg(errp, "Not support get cap %x", cap); >> can't you add details about the faulting HostIOMMUDevice by tracing the >> devid for instance? > >yes. devid isn't added to make this series simpler. It's added in nesting series, https://github.com/yiliu1765/qemu/commit/5333b1a0ae03b3c5119b46a1af786d199f103889 Do you want to add devid in this series for tracing purpose or adding trace in nesting series is fine for you? > >> I would rephrase the error message into No support for capability 0x%x > >I was going to propose "Unsupported capability ..." Sounds better, will do. Thanks Zhenzhong
On 6/4/24 05:23, Duan, Zhenzhong wrote: > Hi Cédric, Eric, > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Subject: Re: [PATCH v6 11/19] backends/iommufd: Implement >> HostIOMMUDeviceClass::get_cap() handler >> >> On 6/3/24 13:32, Eric Auger wrote: >>> >>> On 6/3/24 08:10, Zhenzhong Duan wrote: >>>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> backends/iommufd.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/backends/iommufd.c b/backends/iommufd.c >>>> index c7e969d6f7..f2f7a762a0 100644 >>>> --- a/backends/iommufd.c >>>> +++ b/backends/iommufd.c >>>> @@ -230,6 +230,28 @@ bool >> iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, >>>> return true; >>>> } >>>> >>>> +static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, >> Error **errp) >>>> +{ >>>> + HostIOMMUDeviceCaps *caps = &hiod->caps; >>>> + >>>> + switch (cap) { >>>> + case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE: >>>> + return caps->type; >>>> + case HOST_IOMMU_DEVICE_CAP_AW_BITS: >>>> + return caps->aw_bits; >>>> + default: >>>> + error_setg(errp, "Not support get cap %x", cap); >>> can't you add details about the faulting HostIOMMUDevice by tracing the >>> devid for instance? >> yes. > devid isn't added to make this series simpler. > It's added in nesting series, https://github.com/yiliu1765/qemu/commit/5333b1a0ae03b3c5119b46a1af786d199f103889 > > Do you want to add devid in this series for tracing purpose or adding trace in nesting series is fine for you? what would be nice is to get a common way to identify a HostIOMMUDevice, can't we use the name of the VFIO/VDPA device? devid does not exist on legacy container. At least a kind of wrapper may be relevant to extract the name. Eric > >>> I would rephrase the error message into No support for capability 0x%x >> I was going to propose "Unsupported capability ..." > Sounds better, will do. > > Thanks > Zhenzhong >
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH v6 11/19] backends/iommufd: Implement >HostIOMMUDeviceClass::get_cap() handler > > > >On 6/4/24 05:23, Duan, Zhenzhong wrote: >> Hi Cédric, Eric, >> >>> -----Original Message----- >>> From: Cédric Le Goater <clg@redhat.com> >>> Subject: Re: [PATCH v6 11/19] backends/iommufd: Implement >>> HostIOMMUDeviceClass::get_cap() handler >>> >>> On 6/3/24 13:32, Eric Auger wrote: >>>> >>>> On 6/3/24 08:10, Zhenzhong Duan wrote: >>>>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>> --- >>>>> backends/iommufd.c | 23 +++++++++++++++++++++++ >>>>> 1 file changed, 23 insertions(+) >>>>> >>>>> diff --git a/backends/iommufd.c b/backends/iommufd.c >>>>> index c7e969d6f7..f2f7a762a0 100644 >>>>> --- a/backends/iommufd.c >>>>> +++ b/backends/iommufd.c >>>>> @@ -230,6 +230,28 @@ bool >>> iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t >devid, >>>>> return true; >>>>> } >>>>> >>>>> +static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, >>> Error **errp) >>>>> +{ >>>>> + HostIOMMUDeviceCaps *caps = &hiod->caps; >>>>> + >>>>> + switch (cap) { >>>>> + case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE: >>>>> + return caps->type; >>>>> + case HOST_IOMMU_DEVICE_CAP_AW_BITS: >>>>> + return caps->aw_bits; >>>>> + default: >>>>> + error_setg(errp, "Not support get cap %x", cap); >>>> can't you add details about the faulting HostIOMMUDevice by tracing >the >>>> devid for instance? >>> yes. >> devid isn't added to make this series simpler. >> It's added in nesting series, >https://github.com/yiliu1765/qemu/commit/5333b1a0ae03b3c5119b46a1 >af786d199f103889 >> >> Do you want to add devid in this series for tracing purpose or adding trace >in nesting series is fine for you? > >what would be nice is to get a common way to identify a HostIOMMUDevice, >can't we use the name of the VFIO/VDPA device? devid does not exist on >legacy container. At least a kind of wrapper may be relevant to extract >the name. Getting name directly is not easy, we can save a copy in .realize(), like below: --- a/include/sysemu/host_iommu_device.h +++ b/include/sysemu/host_iommu_device.h @@ -33,6 +33,7 @@ OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass, HOST_IOMMU_DEVICE) struct HostIOMMUDevice { Object parent_obj; + char *name; HostIOMMUDeviceCaps caps; }; diff --git a/backends/iommufd.c b/backends/iommufd.c index f2f7a762a0..84fefbc9ee 100644 --- a/backends/iommufd.c +++ b/backends/iommufd.c @@ -240,7 +240,7 @@ static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp) case HOST_IOMMU_DEVICE_CAP_AW_BITS: return caps->aw_bits; default: - error_setg(errp, "Not support get cap %x", cap); + error_setg(errp, "%s: unsupported capability %x", hiod->name, cap); return -EINVAL; } } diff --git a/hw/vfio/container.c b/hw/vfio/container.c index a830426647..e78538efec 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -1152,6 +1152,7 @@ static bool hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque, } else { hiod->caps.aw_bits = 0xff; } + hiod->name = g_strdup(vdev->name); return true; } @@ -1165,7 +1166,7 @@ static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap, case HOST_IOMMU_DEVICE_CAP_AW_BITS: return caps->aw_bits; default: - error_setg(errp, "Not support get cap %x", cap); + error_setg(errp, "%s: unsupported capability %x", hiod->name, cap); return -EINVAL; } } diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 8fd8d52bc2..2df3aed47f 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -637,6 +637,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, return false; } + hiod->name = g_strdup(vdev->name); caps->type = type;
On 6/4/24 10:46, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Subject: Re: [PATCH v6 11/19] backends/iommufd: Implement >> HostIOMMUDeviceClass::get_cap() handler >> >> >> >> On 6/4/24 05:23, Duan, Zhenzhong wrote: >>> Hi Cédric, Eric, >>> >>>> -----Original Message----- >>>> From: Cédric Le Goater <clg@redhat.com> >>>> Subject: Re: [PATCH v6 11/19] backends/iommufd: Implement >>>> HostIOMMUDeviceClass::get_cap() handler >>>> >>>> On 6/3/24 13:32, Eric Auger wrote: >>>>> On 6/3/24 08:10, Zhenzhong Duan wrote: >>>>>> Suggested-by: Cédric Le Goater <clg@redhat.com> >>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>>> --- >>>>>> backends/iommufd.c | 23 +++++++++++++++++++++++ >>>>>> 1 file changed, 23 insertions(+) >>>>>> >>>>>> diff --git a/backends/iommufd.c b/backends/iommufd.c >>>>>> index c7e969d6f7..f2f7a762a0 100644 >>>>>> --- a/backends/iommufd.c >>>>>> +++ b/backends/iommufd.c >>>>>> @@ -230,6 +230,28 @@ bool >>>> iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t >> devid, >>>>>> return true; >>>>>> } >>>>>> >>>>>> +static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, >>>> Error **errp) >>>>>> +{ >>>>>> + HostIOMMUDeviceCaps *caps = &hiod->caps; >>>>>> + >>>>>> + switch (cap) { >>>>>> + case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE: >>>>>> + return caps->type; >>>>>> + case HOST_IOMMU_DEVICE_CAP_AW_BITS: >>>>>> + return caps->aw_bits; >>>>>> + default: >>>>>> + error_setg(errp, "Not support get cap %x", cap); >>>>> can't you add details about the faulting HostIOMMUDevice by tracing >> the >>>>> devid for instance? >>>> yes. >>> devid isn't added to make this series simpler. >>> It's added in nesting series, >> https://github.com/yiliu1765/qemu/commit/5333b1a0ae03b3c5119b46a1 >> af786d199f103889 >>> Do you want to add devid in this series for tracing purpose or adding trace >> in nesting series is fine for you? >> >> what would be nice is to get a common way to identify a HostIOMMUDevice, >> can't we use the name of the VFIO/VDPA device? devid does not exist on >> legacy container. At least a kind of wrapper may be relevant to extract >> the name. > Getting name directly is not easy, we can save a copy in .realize(), like below: sounds good + dealloc Eric > > --- a/include/sysemu/host_iommu_device.h > +++ b/include/sysemu/host_iommu_device.h > @@ -33,6 +33,7 @@ OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass, HOST_IOMMU_DEVICE) > struct HostIOMMUDevice { > Object parent_obj; > > + char *name; > HostIOMMUDeviceCaps caps; > }; > > diff --git a/backends/iommufd.c b/backends/iommufd.c > index f2f7a762a0..84fefbc9ee 100644 > --- a/backends/iommufd.c > +++ b/backends/iommufd.c > @@ -240,7 +240,7 @@ static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp) > case HOST_IOMMU_DEVICE_CAP_AW_BITS: > return caps->aw_bits; > default: > - error_setg(errp, "Not support get cap %x", cap); > + error_setg(errp, "%s: unsupported capability %x", hiod->name, cap); > return -EINVAL; > } > } > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index a830426647..e78538efec 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -1152,6 +1152,7 @@ static bool hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque, > } else { > hiod->caps.aw_bits = 0xff; > } > + hiod->name = g_strdup(vdev->name); > > return true; > } > @@ -1165,7 +1166,7 @@ static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap, > case HOST_IOMMU_DEVICE_CAP_AW_BITS: > return caps->aw_bits; > default: > - error_setg(errp, "Not support get cap %x", cap); > + error_setg(errp, "%s: unsupported capability %x", hiod->name, cap); > return -EINVAL; > } > } > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 8fd8d52bc2..2df3aed47f 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -637,6 +637,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, > return false; > } > > + hiod->name = g_strdup(vdev->name); > caps->type = type;
diff --git a/backends/iommufd.c b/backends/iommufd.c index c7e969d6f7..f2f7a762a0 100644 --- a/backends/iommufd.c +++ b/backends/iommufd.c @@ -230,6 +230,28 @@ bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid, return true; } +static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp) +{ + HostIOMMUDeviceCaps *caps = &hiod->caps; + + switch (cap) { + case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE: + return caps->type; + case HOST_IOMMU_DEVICE_CAP_AW_BITS: + return caps->aw_bits; + default: + error_setg(errp, "Not support get cap %x", cap); + return -EINVAL; + } +} + +static void hiod_iommufd_class_init(ObjectClass *oc, void *data) +{ + HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc); + + hioc->get_cap = hiod_iommufd_get_cap; +}; + static const TypeInfo types[] = { { .name = TYPE_IOMMUFD_BACKEND, @@ -246,6 +268,7 @@ static const TypeInfo types[] = { }, { .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, .parent = TYPE_HOST_IOMMU_DEVICE, + .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 | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)