From 467b5e3127faf168fba6ce61a602f82d34a699c7 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Thu, 8 Nov 2018 06:29:36 +0000 Subject: [PATCH 08/35] intel_iommu: better handling of dmar state switch RH-Author: Peter Xu Message-id: <20181108062938.21143-6-peterx@redhat.com> Patchwork-id: 82964 O-Subject: [RHEL-8 qemu-kvm PATCH 5/7] intel_iommu: better handling of dmar state switch Bugzilla: 1625173 RH-Acked-by: Auger Eric RH-Acked-by: Michael S. Tsirkin RH-Acked-by: Laurent Vivier Bugzilla: 1625173 QEMU is not handling the global DMAR switch well, especially when from "on" to "off". Let's first take the example of system reset. Assuming that a guest has IOMMU enabled. When it reboots, we will drop all the existing DMAR mappings to handle the system reset, however we'll still keep the existing memory layouts which has the IOMMU memory region enabled. So after the reboot and before the kernel reloads again, there will be no mapping at all for the host device. That's problematic since any software (for example, SeaBIOS) that runs earlier than the kernel after the reboot will assume the IOMMU is disabled, so any DMA from the software will fail. For example, a guest that boots on an assigned NVMe device might fail to find the boot device after a system reboot/reset and we'll be able to observe SeaBIOS errors if we capture the debugging log: WARNING - Timeout at nvme_wait:144! Meanwhile, we should see DMAR errors on the host of that NVMe device. It's the DMA fault that caused a NVMe driver timeout. The correct fix should be that we do proper switching of device DMA address spaces when system resets, which will setup correct memory regions and notify the backend of the devices. This might not affect much on non-assigned devices since QEMU VT-d emulation will assume a default passthrough mapping if DMAR is not enabled in the GCMD register (please refer to vtd_iommu_translate). However that's required for an assigned devices, since that'll rebuild the correct GPA to HPA mapping that is needed for any DMA operation during guest bootstrap. Besides the system reset, we have some other places that might change the global DMAR status and we'd better do the same thing there. For example, when we change the state of GCMD register, or the DMAR root pointer. Do the same refresh for all these places. For these two places we'll also need to explicitly invalidate the context entry cache and iotlb cache. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1625173 CC: QEMU Stable Reported-by: Cong Li Signed-off-by: Peter Xu -- v2: - do the same for GCMD write, or root pointer update [Alex] - test is carried out by me this time, by observing the vtd_switch_address_space tracepoint after system reboot v3: - rewrite commit message as suggested by Alex Signed-off-by: Peter Xu Reviewed-by: Eric Auger Reviewed-by: Jason Wang Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin (cherry picked from commit 2cc9ddccebcaa48b3debfc279a83761fcbb7616c) Signed-off-by: Peter Xu Signed-off-by: Danilo C. L. de Paula --- hw/i386/intel_iommu.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 48d0ba3..a6e87a9 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -37,6 +37,8 @@ #include "kvm_i386.h" #include "trace.h" +static void vtd_address_space_refresh_all(IntelIOMMUState *s); + static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val, uint64_t wmask, uint64_t w1cmask) { @@ -1436,7 +1438,7 @@ static void vtd_context_global_invalidate(IntelIOMMUState *s) vtd_reset_context_cache_locked(s); } vtd_iommu_unlock(s); - vtd_switch_address_space_all(s); + vtd_address_space_refresh_all(s); /* * From VT-d spec 6.5.2.1, a global context entry invalidation * should be followed by a IOTLB global invalidation, so we should @@ -1727,6 +1729,8 @@ static void vtd_handle_gcmd_srtp(IntelIOMMUState *s) vtd_root_table_setup(s); /* Ok - report back to driver */ vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_RTPS); + vtd_reset_caches(s); + vtd_address_space_refresh_all(s); } /* Set Interrupt Remap Table Pointer */ @@ -1759,7 +1763,8 @@ static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en) vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_TES, 0); } - vtd_switch_address_space_all(s); + vtd_reset_caches(s); + vtd_address_space_refresh_all(s); } /* Handle Interrupt Remap Enable/Disable */ @@ -3059,6 +3064,12 @@ static void vtd_address_space_unmap_all(IntelIOMMUState *s) } } +static void vtd_address_space_refresh_all(IntelIOMMUState *s) +{ + vtd_address_space_unmap_all(s); + vtd_switch_address_space_all(s); +} + static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private) { memory_region_notify_one((IOMMUNotifier *)private, entry); @@ -3231,11 +3242,7 @@ static void vtd_reset(DeviceState *dev) IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); vtd_init(s); - - /* - * When device reset, throw away all mappings and external caches - */ - vtd_address_space_unmap_all(s); + vtd_address_space_refresh_all(s); } static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) -- 1.8.3.1