On 22/07/2024 06:22, Duan, Zhenzhong wrote:
>
>
>> -----Original Message-----
>> From: Joao Martins <joao.m.mart...@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.d...@intel.com>
>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>> Reviewed-by: Cédric Le Goater <c...@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
>
