intel_iommu: handle invalid ce for shadow sync
v3 cover intel_iommu: handle invalid ce for shadow sync
v3:
- pick r-b
- return when -VTD_FR_CONTEXT_ENTRY_P is detected (v1 is correct here, but I did wrong thing when splitting the patch in v2) [Eric]
v2:
- split patch into more, remove useless comment [Eric]
- remove one error_report_once() when rework the code [Jason]
This series fixes a QEMU crash reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1627272
Please review, thanks.
Peter Xu (2):
intel_iommu: move ce fetching out when sync shadow
intel_iommu: handle invalid ce for shadow sync
dtc | 2 +- hw/i386/intel_iommu.c | 55 +++++++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 27 deletions(-)
Signature · 1 line
2.17.1
PATCH v3 1/2 intel_iommu: move ce fetching out when sync shadow
There are two callers for vtd_sync_shadow_page_table_range(): one provided a valid context entry and one not. Move that fetching operation into the caller vtd_sync_shadow_page_table() where we need to fetch the context entry.
Meanwhile, remove the error_report_once() directly since we're already tracing all the error cases in the previous call. Instead, return error number back to caller. This will not change anything functional since callers are dropping it after all.
We do this move majorly because we want to do something more later in vtd_sync_shadow_page_table().
--- dtc | 2 +- hw/i386/intel_iommu.c | 41 +++++++++++++---------------------------- 2 files changed, 14 insertions(+), 29 deletions(-)
dtc +1 -1
@@ -1 +1 @@ -Subproject commit 88f18909db731a627456f26d779445f84e449536 +Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
hw/i386/intel_iommu.c +13 -28
59 lines not on this page · raw message
Signature · 1 line
2.17.1
PATCH v3 2/2 intel_iommu: handle invalid ce for shadow sync
In reply to Peter XuWe should handle VTD_FR_CONTEXT_ENTRY_P properly when synchronizing shadow page tables. Having invalid context entry there is perfectly valid when we move a device out of an existing domain. When that happens, instead of posting an error we invalidate the whole region.
Without this patch, QEMU will crash if we do these steps:
(1) start QEMU with VT-d IOMMU and two 10G NICs (ixgbe)
(2) bind the NICs with vfio-pci in the guest
(3) start testpmd with the NICs applied
(4) stop testpmd
(5) rebind the NIC back to ixgbe kernel driver
The patch should fix it.
--- hw/i386/intel_iommu.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
hw/i386/intel_iommu.c +18 -0
@@ -37,6 +37,8 @@#include "kvm_i386.h" #include "trace.h" +static void vtd_address_space_unmap (VTDAddressSpace *as,IOMMUNotifier *n); +static void vtd_define_quad (IntelIOMMUState *s,hwaddr addr,uint64_t val,uint64_t wmask,uint64_t w1cmask) {
@@ -1056,11 +1058,27 @@ static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as) {int ret;VTDContextEntry ce; +IOMMUNotifier *n; ret =vtd_dev_to_context_entry (vtd_as->iommu_state,pci_bus_num (vtd_as->bus), vtd_as->devfn, &ce);if (ret) { +if (ret == -VTD_FR_CONTEXT_ENTRY_P ) { +/* +* It's a valid scenario to have a context entry that is +* not present. For example, when a device is removed +* from an existing domain then the context entry will be +* zeroed by the guest before it was put into another +* domain. When this happens, instead of synchronizing +* the shadow pages we should invalidate all existing +* mappings and notify the backends. +*/ + IOMMU_NOTIFIER_FOREACH(n, &vtd_as->iommu) { +vtd_address_space_unmap (vtd_as, n); + } + ret =0 ; + }return ret; }
Signature · 1 line
2.17.1
Hi Peter,
Quoted 54 lines · Peter Xu
We should handle
VTD_FR_CONTEXT_ENTRY_Pproperly when synchronizing shadow page tables. Having invalid context entry there is perfectly valid when we move a device out of an existing domain. When that happens, instead of posting an error we invalidate the whole region.Without this patch, QEMU will crash if we do these steps:
(1) start QEMU with VT-d IOMMU and two 10G NICs (ixgbe)
(2) bind the NICs with vfio-pci in the guest
(3) start testpmd with the NICs applied
(4) stop testpmd
(5) rebind the NIC back to ixgbe kernel driverThe patch should fix it.
Reported-by Pei Zhang <[email protected]>Signed-off-by Peter Xu <[email protected]>--- hw/i386/intel_iommu.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)hw/i386/intel_iommu.c +11
-0@@ -37,6 +37,8 @@#include "kvm_i386.h" #include "trace.h" +static void vtd_address_space_unmap (VTDAddressSpace *as,IOMMUNotifier *n); +static void vtd_define_quad (IntelIOMMUState *s,hwaddr addr,uint64_t val,uint64_t wmask,uint64_t w1cmask) {@@ -1056,11 +1058,27 @@ static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as) {int ret;VTDContextEntry ce; +IOMMUNotifier *n; ret =vtd_dev_to_context_entry (vtd_as->iommu_state,pci_bus_num (vtd_as->bus), vtd_as->devfn, &ce);if (ret) { +if (ret == -VTD_FR_CONTEXT_ENTRY_P ) { +/* +* It's a valid scenario to have a context entry that is +* not present. For example, when a device is removed +* from an existing domain then the context entry will be +* zeroed by the guest before it was put into another +* domain. When this happens, instead of synchronizing +* the shadow pages we should invalidate all existing
hw/i386/intel_iommu.c static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as) · line 1084
Quoted 10 lines
hw/i386/intel_iommu.c +7
-0+* mappings and notify the backends. +*/ + IOMMU_NOTIFIER_FOREACH(n, &vtd_as->iommu) { +vtd_address_space_unmap (vtd_as, n); + } + ret =0 ; + }return ret; }
nit: in the future, you could consider making vtd_sync_shadow_page_table a void function as the returned value never is used.
Besides
Thanks
Eric
Quoted 15 lines · Peter Xu
v3:
- pick r-b
- return when -VTD_FR_CONTEXT_ENTRY_P is detected (v1 is correct here, but I did wrong thing when splitting the patch in v2) [Eric]v2:
- split patch into more, remove useless comment [Eric]
- remove oneerror_report_once()when rework the code [Jason]This series fixes a QEMU crash reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1627272
Please review, thanks.
Quoted 8 lines
Peter Xu (2):
intel_iommu: move ce fetching out when sync shadow
intel_iommu: handle invalid ce for shadow syncdtc | 2 +- hw/i386/intel_iommu.c | 55 +++++++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 27 deletions(-)
For the series:
Hi, Michael,
Just a kind reminder that this series has got enough ACKs and please consider to merge it in your next pull. Thanks!
Quoted 15 lines · Peter Xu
v3:
- pick r-b
- return when -VTD_FR_CONTEXT_ENTRY_P is detected (v1 is correct here, but I did wrong thing when splitting the patch in v2) [Eric]v2:
- split patch into more, remove useless comment [Eric]
- remove oneerror_report_once()when rework the code [Jason]This series fixes a QEMU crash reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1627272
Please review, thanks.
Quoted 11 lines
Peter Xu (2):
intel_iommu: move ce fetching out when sync shadow
intel_iommu: handle invalid ce for shadow syncdtc | 2 +- hw/i386/intel_iommu.c | 55 +++++++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 27 deletions(-)Signature · 1 line
2.17.1
Regards,
Signature · 1 line
Peter Xu