[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/2] intel_iommu: handle invalid ce for shado
From: |
Auger Eric |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/2] intel_iommu: handle invalid ce for shadow sync |
Date: |
Tue, 9 Oct 2018 09:55:13 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
Hi Peter,
On 10/9/18 9:45 AM, Peter Xu wrote:
> We 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.
>
> Reported-by: Pei Zhang <address@hidden>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1627272
> Signed-off-by: Peter Xu <address@hidden>
> ---
> hw/i386/intel_iommu.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index d6b4f8705d..6072f9a4e0 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_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;
> }
>
>
nit: in the future, you could consider making vtd_sync_shadow_page_table
a void function as the returned value never is used.
Besides
Reviewed-by: Eric Auger <address@hidden>
Thanks
Eric