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

[Eric Auger <eric.auger@redhat.com>]

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;