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 <pezh...@redhat.com> > > Tested-by: Pei Zhang <pezh...@redhat.com> > > CC: Pei Zhang <pezh...@redhat.com> > > CC: Alex Williamson <alex.william...@redhat.com> > > CC: Jason Wang <jasow...@redhat.com> > > CC: Maxime Coquelin <maxime.coque...@redhat.com> > > CC: Michael S. Tsirkin <m...@redhat.com> > > CC: QEMU Stable <qemu-sta...@nongnu.org> > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1627272 > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > 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
