[Xen-changelog] [xen master] gnttab: make sure grant map operations don't skip their IOMMU part

[patchbot@xxxxxxx]

commit 921f1f42260c7967bf18f8a143d39511d163c421
Author:     Jan Beulich <jbeulich@xxxxxxxx>
AuthorDate: Tue Dec 3 14:13:40 2019 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Tue Dec 3 14:13:40 2019 +0100

    gnttab: make sure grant map operations don't skip their IOMMU part
    
    Two almost simultaneous mapping requests need to make sure that at the
    completion of the earlier one IOMMU mappings (established explicitly
    here in the PV case) have been put in place. Forever since the splitting
    of the grant table lock a violation of this has been possible (using
    simplified pin counts, as it doesn't matter whether we talk about read
    or write mappings here):
    
    initial state: act->pin = 0
    
    vCPU A: progress the operation past the dropping of the locks after the
            act->pin updates (act->pin = 1, old_pin = 0, act_pin = 1)
    
    vCPU B: progress the operation past the dropping of the locks after the
            act->pin updates (act->pin = 2, old_pin = 1, act_pin = 2)
    
    vCPU B: (re-)acquire both gt locks, mapkind() returns 0, but both
            iommu_legacy_map() invocations get skipped due to non-zero
            old_pin
    
    vCPU B: return to caller without IOMMU mapping
    
    vCPU A: (re-)acquire both gt locks, mapkind() returns 0,
            iommu_legacy_map() gets invoked
    
    With the locks dropped intermediately, whether to invoke
    iommu_legacy_map() must depend on only the return value of mapkind()
    and of course the kind of mapping request being processed, just like
    is already the case in unmap_common().
    
    Also fix the style of the adjacent comment, and correct a nearby one
    still referring to a prior name of what is now mapkind().
    
    Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
    Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
 xen/common/grant_table.c | 35 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 729f362ea8..5536d282b9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -948,8 +948,6 @@ map_grant_ref(
     mfn_t mfn;
     struct page_info *pg = NULL;
     int            rc = GNTST_okay;
-    u32            old_pin;
-    u32            act_pin;
     unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0;
     bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
@@ -1058,7 +1056,6 @@ map_grant_ref(
         }
     }
 
-    old_pin = act->pin;
     if ( op->flags & GNTMAP_device_map )
         act->pin += (op->flags & GNTMAP_readonly) ?
             GNTPIN_devr_inc : GNTPIN_devw_inc;
@@ -1067,7 +1064,6 @@ map_grant_ref(
             GNTPIN_hstr_inc : GNTPIN_hstw_inc;
 
     mfn = act->mfn;
-    act_pin = act->pin;
 
     cache_flags = (shah->flags & (GTF_PAT | GTF_PWT | GTF_PCD) );
 
@@ -1175,27 +1171,22 @@ map_grant_ref(
     if ( need_iommu )
     {
         unsigned int kind;
-        int err = 0;
 
         double_gt_lock(lgt, rgt);
 
-        /* We're not translated, so we know that gmfns and mfns are
-           the same things, so the IOMMU entry is always 1-to-1. */
+        /*
+         * We're not translated, so we know that dfns and mfns are
+         * the same things, so the IOMMU entry is always 1-to-1.
+         */
         kind = mapkind(lgt, rd, mfn);
-        if ( (act_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) &&
-             !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
-        {
-            if ( !(kind & MAPKIND_WRITE) )
-                err = iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0,
-                                       IOMMUF_readable | IOMMUF_writable);
-        }
-        else if ( act_pin && !old_pin )
-        {
-            if ( !kind )
-                err = iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0,
-                                       IOMMUF_readable);
-        }
-        if ( err )
+        if ( !(op->flags & GNTMAP_readonly) &&
+             !(kind & MAPKIND_WRITE) )
+            kind = IOMMUF_readable | IOMMUF_writable;
+        else if ( !kind )
+            kind = IOMMUF_readable;
+        else
+            kind = 0;
+        if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0, kind) )
         {
             double_gt_unlock(lgt, rgt);
             rc = GNTST_general_error;
@@ -1210,7 +1201,7 @@ map_grant_ref(
      * other fields so just ensure the flags field is stored last.
      *
      * However, if gnttab_need_iommu_mapping() then this would race
-     * with a concurrent mapcount() call (on an unmap, for example)
+     * with a concurrent mapkind() call (on an unmap, for example)
      * and a lock is required.
      */
     mt = &maptrack_entry(lgt, handle);
--
generated by git-patchbot for /home/xen/git/xen.git#master

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/xen-changelog