|
| From: | Cédric Le Goater |
| Subject: | Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap |
| Date: | Thu, 18 Apr 2024 08:42:34 +0200 |
| User-agent: | Mozilla Thunderbird |
Hello Zhenzhong On 4/17/24 11:24, Duan, Zhenzhong wrote:
-----Original Message----- From: Cédric Le Goater <clg@redhat.com> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap On 4/17/24 06:21, Duan, Zhenzhong wrote:-----Original Message----- From: Cédric Le Goater <clg@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 <clg@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.liu@intel.com> If check fails, the host side device(either vfio or vdpa device) shouldnotbe passed to guest. Implementation details for different backends will be in followingpatches.Signed-off-by: Yi Liu <yi.l.liu@intel.com> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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 ofvtd_check_legacy_hdevand vtd_check_iommufd_hdev are different especially after nestingsupport introduced.vtd_check_iommufd_hdev() has much wider check over cap/ecap bitsbesides aw_bits. I think it is important to fully separate the vIOMMU model from thehost IOMMU backing device.
This comment is true for the structures also.
Could we introduce a new HostIOMMUDeviceClass handler .check_hdev() handler, which would call .get_host_iommu_info() ?
This means that HIOD_LEGACY_INFO and HIOD_IOMMUFD_INFO should be
a common structure 'HostIOMMUDeviceInfo' holding all attributes
for the different backends. Each .get_host_iommu_info() implementation
would translate the specific host iommu device data presentation
into the common 'HostIOMMUDeviceInfo', this is true for host_aw_bits.
'type' could be handled the same way, with a 'HostIOMMUDeviceInfo'
type attribute and host iommu device type definitions, or as you
suggested with a QOM interface. This is more complex however. In
this case, I would suggest to implement a .compatible() handler to
compare the host iommu device type with the vIOMMU type.
The resulting check_hdev routine would look something like :
static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev,
Error **errp)
{
HostIOMMUDevice *hiod = vtd_hdev->dev;
HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
HostIOMMUDevice info;
int host_aw_bits, ret;
ret = hiodc->get_host_iommu_info(hiod, &info, sizeof(info), errp);
if (ret) {
return ret;
}
ret = hiodc->is_compatible(hiod, VIOMMU_INTERFACE(s));
if (ret) {
return ret;
}
if (s->aw_bits > info.aw_bits) {
error_setg(errp, "aw-bits %d > host aw-bits %d",
s->aw_bits, info.aw_bits);
return -EINVAL;
}
}
and the HostIOMMUDeviceClass::is_compatible() handler would call a
vIOMMUInterface::compatible() handler simply returning
IOMMU_HW_INFO_TYPE_INTEL_VTD. How does that sound ?
Including the type in HostIOMMUDeviceInfo is much simpler to start with.
Thanks,
C.
Understood, besides the new .check_hdev() handler, I think we also need anew interfaceclass TYPE_IOMMU_CHECK_HDEV which has two handlerscheck_[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]Sure. HostIOMMUDevice | .get_host_iommu_info() | .check_hdev() | .------------------------------. | | HIODLegacy HIODIOMMUFD | | .iommufd .--------------. | .devid | | | [.ioas_id] HIODLegacyVFIO [HIODLegacyVDPA] | [.attach_hwpt()] | .vdev | [.vdev] | [.detach_hwpt()] | .----------------------. | | HIODIOMMUFDVFIO [HIODIOMMUFDVDPA] | .vdev | [.vdev] HostIOMMUDevice only declare .check_hdev(), but HIODLegacy and HIODIOMMUFD will implement .check_hdev(). E.g., hiod_legacy_check_hdev() and hiod_iommufd_check_hdev(). int hiod_legacy_check_hdev(HostIOMMUDevice *hiod, IOMMUCheckHDev *viommu, Error **errp) { IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_GET_CLASS(viommu); return chdc->check_legacy_hdev(viommu, hiod, errp); } int hiod_iommufd_check_hdev(HostIOMMUDevice *hiod, IOMMUCheckHDev *viommu, Error **errp) { IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_GET_CLASS(viommu); return chdc->check_iommufd_hdev(viommu, hiod, errp); } And we implement interface TYPE_IOMMU_CHECK_HDEV in intel-iommu module. Certainly, we can also implement the same in other vIOMMUs we want. See below pseudo change: diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 68380d50ca..173c702b9f 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -5521,12 +5521,9 @@ static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev, Error **errp) { HostIOMMUDevice *hiod = vtd_hdev->dev; + HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod); - if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) { - return vtd_check_iommufd_hdev(s, vtd_hdev, errp); - } - - return vtd_check_legacy_hdev(s, hiod, errp); + return hiodc->check_hdev(IOMMU_CHECK_HDEV(s), hiod, errp); } static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, @@ -6076,6 +6073,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); X86IOMMUClass *x86_class = X86_IOMMU_DEVICE_CLASS(klass); + IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_CLASS(klass); dc->reset = vtd_reset; dc->vmsd = &vtd_vmstate; @@ -6087,6 +6085,8 @@ static void vtd_class_init(ObjectClass *klass, void *data) dc->user_creatable = true; set_bit(DEVICE_CATEGORY_MISC, dc->categories); dc->desc = "Intel IOMMU (VT-d) DMA Remapping device"; + chdc->check_legacy_hdev = vtd_check_legacy_hdev; + chdc->check_iommufd_hdev = vtd_check_iommufd_hdev; } static const TypeInfo vtd_info = { @@ -6094,6 +6094,10 @@ static const TypeInfo vtd_info = { .parent = TYPE_X86_IOMMU_DEVICE, .instance_size = sizeof(IntelIOMMUState), .class_init = vtd_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_IOMMU_CHECK_HDEV }, + { } + } }; Thanks Zhenzhong
| [Prev in Thread] | Current Thread | [Next in Thread] |