Re: [Qemu-devel] [PATCH] intel_iommu: handle invalid ce for shadow sync

[Peter Xu]

On Mon, Oct 01, 2018 at 01:36:50PM +0200, Auger Eric wrote:
> Hi Peter,
> On 9/13/18 9:55 AM, Peter Xu wrote:
> > 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, 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>
> > Tested-by: Pei Zhang <address@hidden>
> > CC: Pei Zhang <address@hidden>
> > CC: Alex Williamson <address@hidden>
> > CC: Jason Wang <address@hidden>
> > CC: Maxime Coquelin <address@hidden>
> > CC: Michael S. Tsirkin <address@hidden>
> > CC: QEMU Stable <address@hidden>
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1627272
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  hw/i386/intel_iommu.c | 54 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 33 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 3dfada19a6..2509520d6f 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)
> >  {
> Comment above is outdated:
> /* If context entry is NULL, we'll try to fetch it on our own. */

Indeed.

> > @@ -1047,39 +1049,49 @@ static int 
> > vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as,
> >          .notify_unmap = true,
> >          .aw = s->aw_bits,
> >          .as = vtd_as,
> > +        .domain_id = VTD_CONTEXT_ENTRY_DID(ce->hi),
> >      };
> > -    VTDContextEntry ce_cache;
> > +
> > +    return vtd_page_walk(ce, addr, addr + size, &info);
> > +}
> Maybe change would gain in clarity if split into 2 patches, code
> reorganization and fix on the side.

Sure.  Thanks for reviewing!

-- 
Peter Xu