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

[Peter Xu <peterx@redhat.com>]

On Thu, Sep 13, 2018 at 10:16:20AM +0200, Maxime Coquelin wrote:
> Hi Peter,
> 
> On 09/13/2018 09: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<pezhang@redhat.com>
> > Tested-by: Pei Zhang<pezhang@redhat.com>
> > CC: Pei Zhang<pezhang@redhat.com>
> > CC: Alex Williamson<alex.williamson@redhat.com>
> > CC: Jason Wang<jasowang@redhat.com>
> > CC: Maxime Coquelin<maxime.coquelin@redhat.com>
> > CC: Michael S. Tsirkin<mst@redhat.com>
> > CC: QEMU Stable<qemu-stable@nongnu.org>
> > Fixes:https://bugzilla.redhat.com/show_bug.cgi?id=1627272
> 
> It seems like a regression as it wasn't reported earlier, isn't it?
> If it is, do you know what is the faulty commit?

I think it should be 63b88968f1 ("intel-iommu: rework the page walk
logic", 2018-05-23).  The old code would possibly unmap all (using the
old replay logic) before we introduce the shadow page sync helpers and
I must have overlooked on that point.

> 
> > Signed-off-by: Peter Xu<peterx@redhat.com>
> > ---
> >   hw/i386/intel_iommu.c | 54 ++++++++++++++++++++++++++-----------------
> >   1 file changed, 33 insertions(+), 21 deletions(-)
> 
> Other than that, the patch looks good to me.
> FWIW:
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,

-- 
Peter Xu