[Qemu-devel] [PATCH v2 0/2] intel_iommu: handle invalid ce for shadow sync

[Qemu-devel] [PATCH v2 0/2] intel_iommu: handle invalid ce for shadow sync

[Peter Xu]

v2:
- split patch into more, remove useless comment [Eric]
- remove one error_report_once() when rework the code [Jason]

This series fixes a QEMU crash reported here:

https://bugzilla.redhat.com/show_bug.cgi?id=1627272

Please review, thanks.

Peter Xu (2):
  intel_iommu: move ce fetching out when sync shadow
  intel_iommu: handle invalid ce for shadow sync

 dtc                   |  2 +-
 hw/i386/intel_iommu.c | 55 +++++++++++++++++++++++--------------------
 2 files changed, 30 insertions(+), 27 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/2] intel_iommu: move ce fetching out when sync shadow

[Peter Xu]

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, remove the error_report_once() directly since we're already
tracing all the error cases in the previous call.  Instead, return error
number back to caller.  This will not change anything functional since
callers are dropping it after all.

We do this move majorly because we want to do something more later in
vtd_sync_shadow_page_table().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dtc                   |  2 +-
 hw/i386/intel_iommu.c | 41 +++++++++++++----------------------------
 2 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/dtc b/dtc
index 88f18909db..e54388015a 160000
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit 88f18909db731a627456f26d779445f84e449536
+Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 3dfada19a6..d6b4f8705d 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1035,7 +1035,6 @@ static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry,
     return 0;
 }
 
-/* If context entry is NULL, we'll try to fetch it on our own. */
 static int vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as,
                                             VTDContextEntry *ce,
                                             hwaddr addr, hwaddr size)
@@ -1047,39 +1046,25 @@ 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;
-    int ret;
-
-    if (ce) {
-        /* If the caller provided context entry, use it */
-        ce_cache = *ce;
-    } else {
-        /* If the caller didn't provide ce, try to fetch */
-        ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
-                                       vtd_as->devfn, &ce_cache);
-        if (ret) {
-            /*
-             * This should not really happen, but in case it happens,
-             * we just skip the sync for this time.  After all we even
-             * don't have the root table pointer!
-             */
-            error_report_once("%s: invalid context entry for bus 0x%x"
-                              " devfn 0x%x",
-                              __func__, pci_bus_num(vtd_as->bus),
-                              vtd_as->devfn);
-            return 0;
-        }
-    }
 
-    info.domain_id = VTD_CONTEXT_ENTRY_DID(ce_cache.hi);
-
-    return vtd_page_walk(&ce_cache, addr, addr + size, &info);
+    return vtd_page_walk(ce, addr, addr + size, &info);
 }
 
 static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as)
 {
-    return vtd_sync_shadow_page_table_range(vtd_as, NULL, 0, UINT64_MAX);
+    int ret;
+    VTDContextEntry ce;
+
+    ret = vtd_dev_to_context_entry(vtd_as->iommu_state,
+                                   pci_bus_num(vtd_as->bus),
+                                   vtd_as->devfn, &ce);
+    if (ret) {
+        return ret;
+    }
+
+    return vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX);
 }
 
 /*
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/2] intel_iommu: handle invalid ce for shadow sync

[Peter Xu]

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>
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1627272
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index d6b4f8705d..b0884e87e8 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,12 +1058,28 @@ 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) {
-        return 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);
+            }
+        } else {
+            return ret;
+        }
     }
 
     return vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX);
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: handle invalid ce for shadow sync

[Peter Xu]

On Mon, Oct 08, 2018 at 02:47:11PM +0800, Peter Xu wrote:
> v2:
> - split patch into more, remove useless comment [Eric]
> - remove one error_report_once() when rework the code [Jason]
> 
> This series fixes a QEMU crash reported here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1627272
> 
> Please review, thanks.

Sorry when splitting the patch I forgot to:

CC: QEMU Stable <qemu-stable@nongnu.org>

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 1/2] intel_iommu: move ce fetching out when sync shadow

[Auger Eric]

Hi Peter,

On 10/8/18 8:47 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, remove the error_report_once() directly since we're already
> tracing all the error cases in the previous call.  Instead, return error
> number back to caller.  This will not change anything functional since
> callers are dropping it after all.
> 
> We do this move majorly because we want to do something more later in
> vtd_sync_shadow_page_table().
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  dtc                   |  2 +-
>  hw/i386/intel_iommu.c | 41 +++++++++++++----------------------------
>  2 files changed, 14 insertions(+), 29 deletions(-)
> 
> diff --git a/dtc b/dtc
> index 88f18909db..e54388015a 160000
> --- a/dtc
> +++ b/dtc
> @@ -1 +1 @@
> -Subproject commit 88f18909db731a627456f26d779445f84e449536
> +Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 3dfada19a6..d6b4f8705d 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1035,7 +1035,6 @@ static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry,
>      return 0;
>  }
>  
> -/* If context entry is NULL, we'll try to fetch it on our own. */
>  static int vtd_sync_shadow_page_table_range(VTDAddressSpace *vtd_as,
>                                              VTDContextEntry *ce,
>                                              hwaddr addr, hwaddr size)
> @@ -1047,39 +1046,25 @@ 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;
> -    int ret;
> -
> -    if (ce) {
> -        /* If the caller provided context entry, use it */
> -        ce_cache = *ce;
> -    } else {
> -        /* If the caller didn't provide ce, try to fetch */
> -        ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
> -                                       vtd_as->devfn, &ce_cache);
> -        if (ret) {
> -            /*
> -             * This should not really happen, but in case it happens,
> -             * we just skip the sync for this time.  After all we even
> -             * don't have the root table pointer!
> -             */
> -            error_report_once("%s: invalid context entry for bus 0x%x"
> -                              " devfn 0x%x",
> -                              __func__, pci_bus_num(vtd_as->bus),
> -                              vtd_as->devfn);
> -            return 0;
> -        }
> -    }
>  
> -    info.domain_id = VTD_CONTEXT_ENTRY_DID(ce_cache.hi);
> -
> -    return vtd_page_walk(&ce_cache, addr, addr + size, &info);
> +    return vtd_page_walk(ce, addr, addr + size, &info);
>  }
>  
>  static int vtd_sync_shadow_page_table(VTDAddressSpace *vtd_as)
>  {
> -    return vtd_sync_shadow_page_table_range(vtd_as, NULL, 0, UINT64_MAX);
> +    int ret;
> +    VTDContextEntry ce;
> +
> +    ret = vtd_dev_to_context_entry(vtd_as->iommu_state,
> +                                   pci_bus_num(vtd_as->bus),
> +                                   vtd_as->devfn, &ce);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    return vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX);
>  }
>  
>  /*
> 

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

[Auger Eric]

Hi Peter,

On 10/8/18 8:47 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 <pezhang@redhat.com>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1627272
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index d6b4f8705d..b0884e87e8 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,12 +1058,28 @@ 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) {
> -        return 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);
> +            }
don't you want to return here also in this case?

Thanks

Eric
> +        } else {
> +            return ret;
> +        }
>      }
>  
>      return vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX);
> 

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

[Peter Xu]

On Tue, Oct 09, 2018 at 09:27:15AM +0200, Auger Eric wrote:

[...]

> > @@ -1056,12 +1058,28 @@ 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) {
> > -        return 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);
> > +            }
> don't you want to return here also in this case?

Yes... a stupid mistake!

I'll repost.  Thank you,

-- 
Peter Xu