| Message ID | 20240507092043.1172717-20-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a host IOMMU device abstraction to check with vIOMMU | expand |
Hi Zhenzhong, On 07/05/2024 11:20, Zhenzhong Duan wrote: Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. If check fails, host device (either VFIO or VDPA device) is not compatible with current vIOMMU config and should not be passed to guest. Only aw_bits is checked for now, we don't care other capabilities before scalable modern mode is introduced. Signed-off-by: Yi Liu <yi.l.liu@intel.com><mailto:yi.l.liu@intel.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com><mailto:zhenzhong.duan@intel.com> --- hw/i386/intel_iommu.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 747c988bc4..146fde23fc 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/host_iommu_device.h" #include "hw/i386/apic_internal.h" #include "kvm/kvm_i386.h" #include "migration/vmstate.h" @@ -3819,6 +3820,25 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, return vtd_dev_as; } +static bool vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev, + Error **errp) +{ + HostIOMMUDevice *hiod = vtd_hdev->dev; Why not passing the hiod pointer as parameter directly? Maybe you have something in mind for a future patch? It would allow us to allocate the VTDHostIOMMUDevice later in vtd_dev_set_iommu_device. + int ret; + + /* Common checks */ + ret = host_iommu_device_get_cap(hiod, HOST_IOMMU_DEVICE_CAP_AW_BITS, errp); + if (ret < 0) { + return false; + } + if (s->aw_bits > ret) { + error_setg(errp, "aw-bits %d > host aw-bits %d", s->aw_bits, ret); + return false; + } + + return true; +} + static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, HostIOMMUDevice *hiod, Error **errp) { @@ -3848,6 +3868,12 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, vtd_hdev->iommu_state = s; vtd_hdev->dev = hiod; + if (!vtd_check_hdev(s, vtd_hdev, errp)) { + g_free(vtd_hdev); + vtd_iommu_unlock(s); + return false; + } + new_key = g_malloc(sizeof(*new_key)); new_key->bus = bus; new_key->devfn = devfn; -- 2.34.1
Hi Clement, See inline. From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> Sent: Tuesday, May 7, 2024 7:40 PM To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-devel@nongnu.org Cc: alex.williamson@redhat.com; clg@redhat.com; eric.auger@redhat.com; mst@redhat.com; peterx@redhat.com; jasowang@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>; Peng, Chao P <chao.p.peng@intel.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net> Subject: Re: [PATCH v4 19/19] intel_iommu: Check compatibility with host IOMMU capabilities Hi Zhenzhong, On 07/05/2024 11:20, Zhenzhong Duan wrote: Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. If check fails, host device (either VFIO or VDPA device) is not compatible with current vIOMMU config and should not be passed to guest. Only aw_bits is checked for now, we don't care other capabilities before scalable modern mode is introduced. Signed-off-by: Yi Liu <yi.l.liu@intel.com><mailto:yi.l.liu@intel.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com><mailto:zhenzhong.duan@intel.com> --- hw/i386/intel_iommu.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 747c988bc4..146fde23fc 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/host_iommu_device.h" #include "hw/i386/apic_internal.h" #include "kvm/kvm_i386.h" #include "migration/vmstate.h" @@ -3819,6 +3820,25 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, return vtd_dev_as; } +static bool vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev, + Error **errp) +{ + HostIOMMUDevice *hiod = vtd_hdev->dev; Why not passing the hiod pointer as parameter directly? Maybe you have something in mind for a future patch? [Zhenzhong] Yes, I have below change in commit https://github.com/yiliu1765/qemu/commit/7a8219b040b44efe7a828e130bdf5ccc2dddb4d0 ret = host_iommu_device_get_cap(hiod, HOST_IOMMU_DEVICE_CAP_ERRATA, errp); if (ret < 0) { return false; } vtd_hdev->errata = ret; Thanks Zhenzhong It would allow us to allocate the VTDHostIOMMUDevice later in vtd_dev_set_iommu_device. + int ret; + + /* Common checks */ + ret = host_iommu_device_get_cap(hiod, HOST_IOMMU_DEVICE_CAP_AW_BITS, errp); + if (ret < 0) { + return false; + } + if (s->aw_bits > ret) { + error_setg(errp, "aw-bits %d > host aw-bits %d", s->aw_bits, ret); + return false; + } + + return true; +} + static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, HostIOMMUDevice *hiod, Error **errp) { @@ -3848,6 +3868,12 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, vtd_hdev->iommu_state = s; vtd_hdev->dev = hiod; + if (!vtd_check_hdev(s, vtd_hdev, errp)) { + g_free(vtd_hdev); + vtd_iommu_unlock(s); + return false; + } + new_key = g_malloc(sizeof(*new_key)); new_key->bus = bus; new_key->devfn = devfn; -- 2.34.1
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 747c988bc4..146fde23fc 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/host_iommu_device.h" #include "hw/i386/apic_internal.h" #include "kvm/kvm_i386.h" #include "migration/vmstate.h" @@ -3819,6 +3820,25 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, return vtd_dev_as; } +static bool vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev, + Error **errp) +{ + HostIOMMUDevice *hiod = vtd_hdev->dev; + int ret; + + /* Common checks */ + ret = host_iommu_device_get_cap(hiod, HOST_IOMMU_DEVICE_CAP_AW_BITS, errp); + if (ret < 0) { + return false; + } + if (s->aw_bits > ret) { + error_setg(errp, "aw-bits %d > host aw-bits %d", s->aw_bits, ret); + return false; + } + + return true; +} + static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, HostIOMMUDevice *hiod, Error **errp) { @@ -3848,6 +3868,12 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, vtd_hdev->iommu_state = s; vtd_hdev->dev = hiod; + if (!vtd_check_hdev(s, vtd_hdev, errp)) { + g_free(vtd_hdev); + vtd_iommu_unlock(s); + return false; + } + new_key = g_malloc(sizeof(*new_key)); new_key->bus = bus; new_key->devfn = devfn;