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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Subject: Re: [PATCH v5 06/13] vfio/{iommufd,container}: Remove
>caps::aw_bits
>
>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.

Currently only VFIODevice is supported, so hiod->agent can only points to a 
VFIODevice.
In future, when VDPA is supported, hiod->agent can point to some kind of 
VDPADevice structure after ::realize() initialize it.

But I'm ok to leave it to VDPA to fix this as for now hiod->agent only points 
to VFIODevice.

Thanks
Zhenzhong

>
>> 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
>>