[Xen-devel] [PATCH 1/5] xen/gnttab: Reduce complexity when reading grant_entry_header_t

[Andrew Cooper <andrew.cooper3@citrix.com>]

_set_status_v{1,2}() and gnttab_prepare_for_transfer() read the shared header
by always casting to u32.  Despite grant_entry_header_t only having an
alignment of 2, this is actually safe because of the grant table ABI itself.

Switch to using an explicit uint32_t *, which removes all subsequent casting.

Furthermore, switch to using ACCESS_ONCE() for the read.  There is nothing in
the _set_status_v1() and gnttab_prepare_for_transfer() which prevents the
compiler from issuing multiple memory reads and creating a TOCTOU race around
the sanity checks, although the worst that can happen is Xen stamping a status
flag over a bad grant entry if the guest is misbehaving.

_set_status_v2() does use barrier() to try avoid multiple reads, but this is
overkill.  All that matters is that the shared header gets read in one go, and
this allows the compiler more room to optimise.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>

Dropping the barrier() in _set_status_v2() allows the optimiser to notice that
initialiser for flags/id are redundant memory reads.  This is fixed in the
following patch.
---
 xen/common/grant_table.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 2bbde5c..e5d585f 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -679,6 +679,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
                           domid_t  ldomid)
 {
     int rc = GNTST_okay;
+    uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo, prev_scombo, new_scombo;
     uint16_t mask = GTF_type_mask;
 
@@ -697,7 +698,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
     if ( mapflag )
         mask |= GTF_sub_page;
 
-    scombo.word = *(u32 *)shah;
+    scombo.word = ACCESS_ONCE(*raw_shah);
 
     /*
      * This loop attempts to set the access (reading/writing) flags
@@ -728,7 +729,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
                          "Attempt to write-pin a r/o grant entry\n");
         }
 
-        prev_scombo.word = guest_cmpxchg(rd, (u32 *)shah,
+        prev_scombo.word = guest_cmpxchg(rd, raw_shah,
                                          scombo.word, new_scombo.word);
         if ( likely(prev_scombo.word == scombo.word) )
             break;
@@ -753,17 +754,13 @@ static int _set_status_v2(const grant_entry_header_t *shah,
                           domid_t  ldomid)
 {
     int      rc    = GNTST_okay;
+    uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo;
     uint16_t flags = shah->flags;
     domid_t  id    = shah->domid;
     uint16_t mask  = GTF_type_mask;
 
-    /* we read flags and domid in a single memory access.
-       this avoids the need for another memory barrier to
-       ensure access to these fields are not reordered */
-    scombo.word = *(u32 *)shah;
-    barrier(); /* but we still need to stop the compiler from turning
-                  it back into two reads */
+    scombo.word = ACCESS_ONCE(*raw_shah);
     flags = scombo.shorts.flags;
     id = scombo.shorts.domid;
 
@@ -797,8 +794,7 @@ static int _set_status_v2(const grant_entry_header_t *shah,
        still valid */
     smp_mb();
 
-    scombo.word = *(u32 *)shah;
-    barrier();
+    scombo.word = ACCESS_ONCE(*raw_shah);
     flags = scombo.shorts.flags;
     id = scombo.shorts.domid;
 
@@ -2041,7 +2037,7 @@ gnttab_prepare_for_transfer(
     struct domain *rd, struct domain *ld, grant_ref_t ref)
 {
     struct grant_table *rgt = rd->grant_table;
-    grant_entry_header_t *sha;
+    uint32_t *raw_shah;
     union grant_combo   scombo, prev_scombo, new_scombo;
     int                 retries = 0;
 
@@ -2055,9 +2051,8 @@ gnttab_prepare_for_transfer(
         goto fail;
     }
 
-    sha = shared_entry_header(rgt, ref);
-
-    scombo.word = *(u32 *)&sha->flags;
+    raw_shah = (uint32_t *)shared_entry_header(rgt, ref);
+    scombo.word = ACCESS_ONCE(*raw_shah);
 
     for ( ; ; )
     {
@@ -2074,7 +2069,7 @@ gnttab_prepare_for_transfer(
         new_scombo = scombo;
         new_scombo.shorts.flags |= GTF_transfer_committed;
 
-        prev_scombo.word = guest_cmpxchg(rd, (u32 *)&sha->flags,
+        prev_scombo.word = guest_cmpxchg(rd, raw_shah,
                                          scombo.word, new_scombo.word);
         if ( likely(prev_scombo.word == scombo.word) )
             break;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel