Re: [PATCH v5 06/13] vfio/{iommufd,container}: Remove caps::aw_bits

[Joao Martins <joao.m.martins@oracle.com>]

On 22/07/2024 06:22, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Subject: [PATCH v5 06/13] vfio/{iommufd,container}: Remove caps::aw_bits
>>
>> Remove caps::aw_bits which requires the bcontainer::iova_ranges being
>> initialized after device is actually attached. Instead defer that to
>> .get_cap() and call vfio_device_get_aw_bits() directly.
>>
>> This is in preparation for HostIOMMUDevice::realize() being called early
>> during attach_device().
>>
>> Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Reviewed-by: Cédric Le Goater <clg@redhat.com
>> ---
>> include/sysemu/host_iommu_device.h | 3 ---
>> backends/iommufd.c                 | 3 ++-
>> hw/vfio/container.c                | 5 +----
>> hw/vfio/iommufd.c                  | 1 -
>> 4 files changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>> b/include/sysemu/host_iommu_device.h
>> index ee6c813c8b22..cdeeccec7671 100644
>> --- a/include/sysemu/host_iommu_device.h
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -19,12 +19,9 @@
>>  * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
>>  *
>>  * @type: host platform IOMMU type.
>> - *
>> - * @aw_bits: host IOMMU address width. 0xff if no limitation.
>>  */
>> typedef struct HostIOMMUDeviceCaps {
>>     uint32_t type;
>> -    uint8_t aw_bits;
>> } HostIOMMUDeviceCaps;
>>
>> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index a94d3b90c05c..58032e588f49 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -18,6 +18,7 @@
>> #include "qemu/error-report.h"
>> #include "monitor/monitor.h"
>> #include "trace.h"
>> +#include "hw/vfio/vfio-common.h"
>> #include <sys/ioctl.h>
>> #include <linux/iommufd.h>
>>
>> @@ -270,7 +271,7 @@ static int
>> hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp)
>>     case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE:
>>         return caps->type;
>>     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>> -        return caps->aw_bits;
>> +        return vfio_device_get_aw_bits(hiod->agent);
> 
> I just realized there is an open here. hiod->agent is not necessarily VFIO device, can be VDPA device.
> May need a bit more work on this.
> 

Broadly speaking I agree, that this needs some sort of IOMMUDevice structure
with a agent type that it needs to abstract from instead of an opaque object.

But feels unrelated to this patch exactly, as the existing code was already
making assumptions that ::opaque is a VFIODevice.

> Thanks
> Zhenzhong
> 
>>     default:
>>         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 88ede913d6f7..c27f448ba26e 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -1144,7 +1144,6 @@ static bool
>> hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>     VFIODevice *vdev = opaque;
>>
>>     hiod->name = g_strdup(vdev->name);
>> -    hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev);
>>     hiod->agent = opaque;
>>
>>     return true;
>> @@ -1153,11 +1152,9 @@ static bool
>> hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>> static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap,
>>                                     Error **errp)
>> {
>> -    HostIOMMUDeviceCaps *caps = &hiod->caps;
>> -
>>     switch (cap) {
>>     case HOST_IOMMU_DEVICE_CAP_AW_BITS:
>> -        return caps->aw_bits;
>> +        return vfio_device_get_aw_bits(hiod->agent);
>>     default:
>>         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 545f4a404125..028533bc39b9 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -724,7 +724,6 @@ static bool
>> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>
>>     hiod->name = g_strdup(vdev->name);
>>     caps->type = type;
>> -    caps->aw_bits = vfio_device_get_aw_bits(vdev);
>>
>>     return true;
>> }
>> --
>> 2.17.2
>