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

[Duan, Zhenzhong]

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

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


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