On 4/17/24 06:21, Duan, Zhenzhong wrote:
-----Original Message-----
From: Cédric Le Goater <c...@redhat.com>
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
Hello,
On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
-----Original Message-----
From: Cédric Le Goater <c...@redhat.com>
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu <yi.l....@intel.com>
If check fails, the host side device(either vfio or vdpa device) should not
be passed to guest.
Implementation details for different backends will be in following
patches.
Signed-off-by: Yi Liu <yi.l....@intel.com>
Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
---
hw/i386/intel_iommu.c | 35
+++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+ return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+ return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s,
VTDHostIOMMUDevice
*vtd_hdev,
+ Error **errp)
+{
+ HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+ if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
+ return vtd_check_iommufd_hdev(s, hiod, errp);
+ }
+
+ return vtd_check_legacy_hdev(s, hiod, errp);
+}
I think we should be using the .get_host_iommu_info() class handler
instead. Can we refactor the code slightly to avoid this check on
the type ?
There is some difficulty ini avoiding this check, the behavior of
vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after nesting
support introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap bits
besides aw_bits.
I think it is important to fully separate the vIOMMU model from the
host IOMMU backing device. Could we introduce a new
HostIOMMUDeviceClass
handler .check_hdev() handler, which would call .get_host_iommu_info() ?
Understood, besides the new .check_hdev() handler, I think we also need a new
interface
class TYPE_IOMMU_CHECK_HDEV which has two handlers
check_[legacy|iommufd]_hdev(),
and different vIOMMUs have different implementation.
I am not sure to understand. Which class hierarchy would implement this
new "TYPE_IOMMU_CHECK_HDEV" interface ? vIOMMU or host iommu ?
Could you please explain with an update of your diagram :
HostIOMMUDevice
| .get_host_iommu_info()
|
|
.------------------------------------.
| | |
HIODLegacyVFIO [HIODLegacyVDPA] HIODIOMMUFD
| .vdev | [.vdev] | .iommufd
| .devid
| [.ioas_id]
| [.attach_hwpt()]
| [.detach_hwpt()]
|
.----------------------.
| |
HIODIOMMUFDVFIO [HIODIOMMUFDVDPA]
| .vdev | [.vdev]
Thanks,
C.
Then legacy and iommufd host device have different implementation of
.check_hdev()
and calls into one of the two interface handlers.
Let me know if I misunderstand any of your point.
Thanks
Zhenzhong
Thanks,
C.
That the reason I have two functions to do different thing.
See:
https://github.com/yiliu1765/qemu/blob/zhenzhong/iommufd_nesting_rfc
v2/hw/i386/intel_iommu.c#L5472
Meanwhile in vtd_check_legacy_hdev(), when legacy VFIO device attaches
to modern vIOMMU,
this is unsupported and error out early, it will not
call .get_host_iommu_info().
I mean we don't need to unconditionally call .get_host_iommu_info() in
some cases.
Thanks
Zhenzhong
