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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.au...@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 <c...@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 <c...@redhat.com>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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;