[PATCH] gnttab: avoid use of raw_shah and union grant_combo in _set_status_v2()

[Jan Beulich <jbeulich@xxxxxxxx>]

The main issue here being the cast to establish raw_shah; where possible
such casts are best avoided. (The (ab)use of unions is an issue as well,
but we have uses like this all over the place.)

With shared and status entries separated, we don't really need to mirror
_set_status_v1() behavior here. The shared entry fields can be accessed
normally, merely using compiler barriers to ensure checks are done on
stable local copies. In particular, torn reads (not that we expect the
compiler to emit such, i.e. this is a purely theoretical concern here
anyway) are not a problem.

While there also avoid unnecessary use of a fixed-width type for the
"mask" local variable (which wants to match "flags" anyway).

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
I'm puzzled by gcc's codegen with uint16_t vs unsigned int for the two
local variables: Use of unsigned int in principle would allow to generate
better code, if only the compiler wouldn't (as per observations with
gcc16)
- emit redundant MOVZWL,
- needlessly clobber a register variable, thus causing use of %ebx (and
  hence saving/restoring of %rbx).
Overall there's no change in size of compiled code (when really it could
shrink). Note that no similar odd effects are observable for Arm.

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -832,26 +832,26 @@ static int _set_status_v2(const grant_en
                           domid_t  ldomid)
 {
     int      rc    = GNTST_okay;
-    const uint32_t *raw_shah = (const uint32_t *)shah;
-    union grant_combo scombo;
-    uint16_t mask  = GTF_type_mask;
+    domid_t  domid = shah->domid;
+    unsigned int flags = shah->flags, mask = GTF_type_mask;
 
-    scombo.raw = ACCESS_ONCE(*raw_shah);
-
-    /* if this is a grant mapping operation we should ensure GTF_sub_page
+    /* If this is a grant mapping operation we should ensure GTF_sub_page
        is not set */
     if ( mapflag )
         mask |= GTF_sub_page;
 
+    /* Make sure all checks below are done on stable local copies. */
+    barrier();
+
     /* If not already pinned, check the grant domid and type. */
     if ( !act->pin &&
-         ((((scombo.flags & mask) != GTF_permit_access) &&
-           (mapflag || ((scombo.flags & mask) != GTF_transitive))) ||
-          (scombo.domid != ldomid)) )
+         ((((flags & mask) != GTF_permit_access) &&
+           (mapflag || ((flags & mask) != GTF_transitive))) ||
+          (domid != ldomid)) )
     {
         gdprintk(XENLOG_WARNING,
                  "Bad flags (%x) or dom (%d); expected d%d, flags %x\n",
-                 scombo.flags, scombo.domid, ldomid, mask);
+                 flags, domid, ldomid, mask);
         rc = GNTST_general_error;
         goto done;
     }
@@ -862,7 +862,7 @@ static int _set_status_v2(const grant_en
     }
     else
     {
-        if ( unlikely(scombo.flags & GTF_readonly) )
+        if ( unlikely(flags & GTF_readonly) )
         {
             gdprintk(XENLOG_WARNING,
                      "Attempt to write-pin a r/o grant entry\n");
@@ -876,26 +876,30 @@ static int _set_status_v2(const grant_en
        still valid */
     smp_mb();
 
-    scombo.raw = ACCESS_ONCE(*raw_shah);
+    flags = shah->flags;
+    domid = shah->domid;
+
+    /* Again, all checks below need doing on stable local copies. */
+    barrier();
 
     if ( !act->pin )
     {
-        if ( (((scombo.flags & mask) != GTF_permit_access) &&
-              (mapflag || ((scombo.flags & mask) != GTF_transitive))) ||
-             (scombo.domid != ldomid) ||
-             (!readonly && (scombo.flags & GTF_readonly)) )
+        if ( (((flags & mask) != GTF_permit_access) &&
+              (mapflag || ((flags & mask) != GTF_transitive))) ||
+             (domid != ldomid) ||
+             (!readonly && (flags & GTF_readonly)) )
         {
             gnttab_clear_flags(rd, GTF_writing | GTF_reading, status);
             gdprintk(XENLOG_WARNING,
                      "Unstable flags (%x) or dom (%d); expected d%d (r/w: 
%d)\n",
-                     scombo.flags, scombo.domid, ldomid, !readonly);
+                     flags, domid, ldomid, !readonly);
             rc = GNTST_general_error;
             goto done;
         }
     }
     else
     {
-        if ( unlikely(scombo.flags & GTF_readonly) )
+        if ( unlikely(flags & GTF_readonly) )
         {
             gnttab_clear_flags(rd, GTF_writing, status);
             gdprintk(XENLOG_WARNING, "Unstable grant readonly flag\n");