[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice
From: |
Cédric Le Goater |
Subject: |
Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice |
Date: |
Wed, 27 Mar 2024 11:25:53 +0100 |
User-agent: |
Mozilla Thunderbird |
Hello Zhenzhong,
On 3/19/24 12:58, Duan, Zhenzhong wrote:
Hi Cédric,
-----Original Message-----
From: Cédric Le Goater <clg@redhat.com>
Sent: Tuesday, March 19, 2024 4:17 PM
To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
devel@nongnu.org
Cc: alex.williamson@redhat.com; eric.auger@redhat.com;
peterx@redhat.com; jasowang@redhat.com; mst@redhat.com;
jgg@nvidia.com; nicolinc@nvidia.com; joao.m.martins@oracle.com; Tian,
Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Sun, Yi Y
<yi.y.sun@intel.com>; Peng, Chao P <chao.p.peng@intel.com>
Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
HostIOMMUDevice
Hello Zhenzhong,
On 2/28/24 04:58, Zhenzhong Duan wrote:
HostIOMMUDevice will be inherited by two sub classes,
legacy and iommufd currently.
Introduce a helper function host_iommu_base_device_init to initialize it.
Suggested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
include/sysemu/host_iommu_device.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 include/sysemu/host_iommu_device.h
diff --git a/include/sysemu/host_iommu_device.h
b/include/sysemu/host_iommu_device.h
new file mode 100644
index 0000000000..fe80ab25fb
--- /dev/null
+++ b/include/sysemu/host_iommu_device.h
@@ -0,0 +1,22 @@
+#ifndef HOST_IOMMU_DEVICE_H
+#define HOST_IOMMU_DEVICE_H
+
+typedef enum HostIOMMUDevice_Type {
+ HID_LEGACY,
+ HID_IOMMUFD,
+ HID_MAX,
+} HostIOMMUDevice_Type;
+
+typedef struct HostIOMMUDevice {
+ HostIOMMUDevice_Type type;
A type field is not a good sign and that's where QOM is useful.
Yes, agree.
I didn't choose QOM because in iommufd-cdev series, VFIOContainer chooses not
using QOM model.
See the discussion: https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
I thought HostIOMMUDevice need to follow same rule.
But after further digging into this, I think it may be ok to use QOM model as
long as we don't expose
HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE interface. Your
thoughts?
yes. Can we change a bit this series to use QOM ? something like :
typedef struct HostIOMMUDevice {
Object parent;
} HostIOMMUDevice;
#define TYPE_HOST_IOMMU "host.iommu"
OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass, HOST_IOMMU)
struct HostIOMMUClass {
ObjectClass parent_class;
int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);
int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);
};
Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and TYPE_HOST_IOMMU_LEGACY.
Each class implementing the handlers or not (legacy mode).
The class handlers are introduced for the intel-iommu helper vtd_check_hdev()
in order to avoid using iommufd routines directly. HostIOMMUDevice is supposed
to abstract the Host IOMMU device, so we need to abstract also all the
interfaces to this object.
The .host_iommu_device_create() handler could be merged in .attach_device()
possibly. Anyhow, please use now object_new() and object_unref() instead.
host_iommu_base_device_init() is useless IMHO.
Is vtd_check_hdev() the only use of this field ?
Currently yes. virtio-iommu may have similar usage.
If so, can we simplify with a QOM interface in any way ?
QOM interface is a set of callbacks, guess you mean QOM class,
saying HostIOMMUDevice class, IOMMULegacyDevice class and IOMMUFDDevice class?
See above proposal. it should work fine.
Also, I think it is better to use a IOMMUFDBackend* parameter for
iommufd_device_get_info() to be consistent with the other routines.
Then It would interesting to see how this applies to Eric's series.
Thanks,
C.