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

[Eric Auger]

On 7/17/24 14:33, Joao Martins wrote:
> On 17/07/2024 13:19, Eric Auger wrote:
>> Hi Joao,
>>
>> On 7/12/24 13:46, Joao Martins wrote:
>>> Fetch IOMMU hw raw caps behind the device and thus move the
>> what does mean "Fetch IOMMU hw raw caps behind the device'"
> Fetching the out_capabilities field from GET_HW_INFO which essentially tell us
> if the IOMMU behind the device supports dirty tracking.
that's much clearer than the 1st sentence
>
>>> 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
>> what do you mean by " It allows it to cache the information obtained
>> from IOMMU_GET_HW_INFO from iommufd early on"
> /me nods
?
>
>>> iommufd early on. However, while legacy HostIOMMUDevice caps
>> what does mean "legacy HostIOMMUDevice caps always return true"?
> That means that it can't fail, and the data in there is synthetic:
>
>     VFIODevice *vdev = opaque;
>
>     hiod->name = g_strdup(vdev->name);
>     hiod->agent = opaque;
>
>     return true;
>
> The IOMMUFD one might fail if GET_HW_INFO fails.
so you talk about hiod_legacy_vfio_realize() and not "

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
>> fetch parse?
>>> dirty tracking is supported by device backing IOMMU without necessarily
>>> duplicating the amount of calls we do to IOMMU_GET_HW_INFO.
>> But we move code from generic place to BE specific place?
>>
> No because in IOMMUFD needs the backend connected, while the legacy backend
> doesn't. Otherwise this patch wouldn't be needed to be backend specific.
>
>> Sorry I feel really hard to understand the commit msg in general
>>
> How about this:
>
>     Fetch IOMMU hw raw caps behind the device and thus move the
You need to tell what the patch does and why.

"Fetch IOMMU hw raw caps behind the device" sentence does not clearly fit in 
any.

>     HostIOMMUDevice::realize() to be done during the attach of the device.
>
>     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.
>
>     Note that the HostIOMMUDevice data with legacy backend is synthetic
>     and doesn't need any information from the (type1-iommu) backend. While the
>     IOMMUFD backend requires the iommufd FD to be connected and having a devid
>     to be able to query device capabilities seeded in HostIOMMUDevice. This
>     means that HostIOMMUDevice initialization (i.e. ::realized() is invoked) 
> is
>     container backend specific.
>
>
>
>
>> Eric
>>
>>
>>> 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;
>> please also update the doc comment
> OK
>
>>>  } 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;
>>>  
>>>      if (vbasedev->iommufd) {
>>>          ops = 
>>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
>>> @@ -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;
>>>      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;
>>> +    }
>>> +
>>>      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_IOMMUFD));
>>> +    HostIOMMUDevice *hiod = vbasedev->hiod;
>>>  
>>>      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;
>>>  }