1. Patchew
  2. QEMU
  3. [PATCH v6 00/19] Add a host IOMMU device abstraction to check with vIOMMU
  4. View patch

[PATCH v6 11/19] backends/iommufd: Implement HostIOMMUDeviceClass::get_cap() handler

Zhenzhong Duan posted 19 patches 5 months, 3 weeks ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Yi Liu <yi.l.liu@intel.com>, Eric Auger <eric.auger@redhat.com>, Zhenzhong Duan <zhenzhong.duan@intel.com>, "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>
There is a newer version of this series
[PATCH v6 11/19] backends/iommufd: Implement HostIOMMUDeviceClass::get_cap() handler
Posted by Zhenzhong Duan 5 months, 3 weeks ago 
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);
+        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,
     }
 };
-- 
2.34.1
Re: [PATCH v6 11/19] backends/iommufd: Implement HostIOMMUDeviceClass::get_cap() handler
Posted by Eric Auger 5 months, 3 weeks ago 

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,
>      }
>  };
Re: [PATCH v6 11/19] backends/iommufd: Implement HostIOMMUDeviceClass::get_cap() handler
Posted by Cédric Le Goater 5 months, 3 weeks ago 
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,
>>       }
>>   };
>
RE: [PATCH v6 11/19] backends/iommufd: Implement HostIOMMUDeviceClass::get_cap() handler
Posted by Duan, Zhenzhong 5 months, 3 weeks ago 
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
Re: [PATCH v6 11/19] backends/iommufd: Implement HostIOMMUDeviceClass::get_cap() handler
Posted by Eric Auger 5 months, 3 weeks ago 

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
>
RE: [PATCH v6 11/19] backends/iommufd: Implement HostIOMMUDeviceClass::get_cap() handler
Posted by Duan, Zhenzhong 5 months, 3 weeks ago 


>-----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;
Re: [PATCH v6 11/19] backends/iommufd: Implement HostIOMMUDeviceClass::get_cap() handler
Posted by Eric Auger 5 months, 3 weeks ago 

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;

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.