Re: [PATCH v4 07/12] vfio/{iommufd,container}: Initialize HostIOMMUDeviceCaps during attach_device()

[Joao Martins]

On 17/07/2024 03:05, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Subject: [PATCH v4 07/12] vfio/{iommufd,container}: Initialize
>> HostIOMMUDeviceCaps during attach_device()
>>
>> Fetch IOMMU hw raw caps behind the device and thus move the
>> HostIOMMUDevice::realize() to be done during the attach of the device. It
>> allows it to cache the information obtained from IOMMU_GET_HW_INFO
>> from
>> iommufd early on. However, while legacy HostIOMMUDevice caps
>> always return true and doesn't have dependency on other things, the
>> IOMMUFD
>> backend requires the iommufd FD to be connected and having a devid to be
>> able to query capabilities. Hence when exactly is HostIOMMUDevice
>> initialized inside backend ::attach_device() implementation is backend
>> specific.
>>
>> This is in preparation to fetch parse hw capabilities and understand if
>> dirty tracking is supported by device backing IOMMU without necessarily
>> duplicating the amount of calls we do to IOMMU_GET_HW_INFO.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> include/sysemu/host_iommu_device.h |  1 +
>> hw/vfio/common.c                   | 16 ++++++----------
>> hw/vfio/container.c                |  6 ++++++
>> hw/vfio/iommufd.c                  |  7 +++++++
>> 4 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>> b/include/sysemu/host_iommu_device.h
>> index 20e77cf54568..b1e5f4b8ac3e 100644
>> --- a/include/sysemu/host_iommu_device.h
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -24,6 +24,7 @@
>>  */
>> typedef struct HostIOMMUDeviceCaps {
>>     uint32_t type;
>> +    uint64_t hw_caps;
>> } HostIOMMUDeviceCaps;
>>
>> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index b0beed44116e..cc14f0e3fe24 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice
>> *vbasedev,
>> {
>>     const VFIOIOMMUClass *ops =
>>
>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>> -    HostIOMMUDevice *hiod;
>> +    HostIOMMUDevice *hiod = NULL;
> 
> No need to NULL it?
> 
/me nods

>>
>>     if (vbasedev->iommufd) {
>>         ops =
>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF
>> D));
>> @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name,
>> VFIODevice *vbasedev,
>>
>>     assert(ops);
>>
>> -    if (!ops->attach_device(name, vbasedev, as, errp)) {
>> -        return false;
>> -    }
>>
>> -    if (vbasedev->mdev) {
>> -        return true;
>> +    if (!vbasedev->mdev) {
>> +        hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>> +        vbasedev->hiod = hiod;
>>     }
>>
>> -    hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>> -    if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>> errp)) {
>> +    if (!ops->attach_device(name, vbasedev, as, errp)) {
>>         object_unref(hiod);
>> -        ops->detach_device(vbasedev);
>> +        vbasedev->hiod = NULL;
>>         return false;
>>     }
>> -    vbasedev->hiod = hiod;
>>
>>     return true;
>> }
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index c27f448ba26e..29da261bbf3e 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char
>> *name, VFIODevice *vbasedev,
>>                                       AddressSpace *as, Error **errp)
>> {
>>     int groupid = vfio_device_groupid(vbasedev, errp);
>> +    HostIOMMUDevice *hiod = vbasedev->hiod;
> 
> Hiod is used only once in this func, may be use vbasedev->hiod directly?
> 

The problem is more of how the line below (...)
> 
>>     VFIODevice *vbasedev_iter;
>>     VFIOGroup *group;
>>     VFIOContainerBase *bcontainer;
>> @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char
>> *name, VFIODevice *vbasedev,
>>
>>     trace_vfio_attach_device(vbasedev->name, groupid);
>>
>> +    if (hiod &&
>> +        !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>> errp)) {
>> +        return false;
>> +    }
>> +

(...) would look like like really long. And I would end up deref-ing 3 times.

But with the helper function that Cedric suggests might easy to accomodate your
comment.

>>     group = vfio_get_group(groupid, as, errp);
>>     if (!group) {
>>         return false;
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 873c919e319c..d34dc88231ec 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name,
>> VFIODevice *vbasedev,
>>     Error *err = NULL;
>>     const VFIOIOMMUClass *iommufd_vioc =
>>
>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF
>> D));
>> +    HostIOMMUDevice *hiod = vbasedev->hiod;
> 
> Same here.
> 
>>
>>     if (vbasedev->fd < 0) {
>>         devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp);
>> @@ -401,6 +402,11 @@ static bool iommufd_cdev_attach(const char
>> *name, VFIODevice *vbasedev,
>>
>>     space = vfio_get_address_space(as);
>>
>> +    if (hiod &&
>> +        !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>> errp)) {
>> +        return false;
>> +    }
>> +
>>     /* try to attach to an existing container in this space */
>>     QLIST_FOREACH(bcontainer, &space->containers, next) {
>>         container = container_of(bcontainer, VFIOIOMMUFDContainer,
>> bcontainer);
>> @@ -722,6 +728,7 @@ static bool
>> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>
>>     hiod->name = g_strdup(vdev->name);
>>     caps->type = type;
>> +    caps->hw_caps = hw_caps;
>>
>>     return true;
>> }
>> --
>> 2.17.2
>