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

[Eric Auger]

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'"
> 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"
> iommufd early on. However, while legacy HostIOMMUDevice caps
what does mean "legacy HostIOMMUDevice caps always return true"?
> 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?

Sorry I feel really hard to understand the commit msg in general

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