Re: [Qemu-devel] [PATCH v3] intel_iommu: better handling of dmar state switch

[Auger Eric]

Hi Peter,

On 9/7/18 4:46 AM, Peter Xu wrote:
> 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.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1625173
> CC: QEMU Stable <address@hidden>
> Reported-by: Cong Li <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> --
> 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
> ---
>  hw/i386/intel_iommu.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 3dfada19a6..59dc155911 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)
>  {
> @@ -1428,7 +1430,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
> @@ -1719,6 +1721,7 @@ 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_address_space_refresh_all(s)
>  }
>  
>  /* Set Interrupt Remap Table Pointer */
> @@ -1751,7 +1754,7 @@ 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_address_space_refresh_all(s);

>  }
>  
>  /* Handle Interrupt Remap Enable/Disable */
> @@ -3051,6 +3054,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);
What about internal context and iotlb caches? Shouldn't they be also
invalidated at the same time we invalidate external caches?

Thanks

Eric
> +    vtd_switch_address_space_all(s);
> +}
> +
>  static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private)
>  {
>      memory_region_notify_one((IOMMUNotifier *)private, entry);
> @@ -3226,11 +3235,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)
>