[PATCH 0/3] gnttab: pin count related adjustments & more

[PATCH 0/3] gnttab: pin count related adjustments & more

[Jan Beulich]

Getting rid of the literal number has been something I've been
hoping to see happen for perhaps over 10 years, along with
doing away with some unnecessary refusal of operations. The
other two changes are "collateral damage" from doing that
change.

1: gnttab: adjust pin count overflow checks
2: gnttab: consolidate pin-to-status syncing
3: Arm: don't hard-code grant table limits in create_domUs()

Jan

[PATCH 1/3] gnttab: adjust pin count overflow checks

[Jan Beulich]

It's at least odd to check counters which aren't going to be
incremented. And it's also not helpful to use open-coded literal numbers
in these checks.

Calculate the increment values first and derive from them the mask to
use in the checks.

Also move the pin count checks ahead of the calculation of the status
(and for copy also sha2) pointers: They're not needed in the failure
cases, and this way the compiler may also have an easier time keeping
the variables at least transiently in registers for the subsequent uses.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -323,22 +323,25 @@ shared_entry_header(struct grant_table *
 /* Active grant entry - used for shadowing GTF_permit_access grants. */
 struct active_grant_entry {
     uint32_t      pin;    /* Reference count information:             */
+                          /* Width of the individual counter fields.  */
+#define GNTPIN_cntr_width    8
+#define GNTPIN_cntr_mask     ((1U << GNTPIN_cntr_width) - 1)
                           /* Count of writable host-CPU mappings.     */
 #define GNTPIN_hstw_shift    0
 #define GNTPIN_hstw_inc      (1U << GNTPIN_hstw_shift)
-#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
+#define GNTPIN_hstw_mask     (GNTPIN_cntr_mask << GNTPIN_hstw_shift)
                           /* Count of read-only host-CPU mappings.    */
-#define GNTPIN_hstr_shift    8
+#define GNTPIN_hstr_shift    (GNTPIN_hstw_shift + GNTPIN_cntr_width)
 #define GNTPIN_hstr_inc      (1U << GNTPIN_hstr_shift)
-#define GNTPIN_hstr_mask     (0xFFU << GNTPIN_hstr_shift)
+#define GNTPIN_hstr_mask     (GNTPIN_cntr_mask << GNTPIN_hstr_shift)
                           /* Count of writable device-bus mappings.   */
-#define GNTPIN_devw_shift    16
+#define GNTPIN_devw_shift    (GNTPIN_hstr_shift + GNTPIN_cntr_width)
 #define GNTPIN_devw_inc      (1U << GNTPIN_devw_shift)
-#define GNTPIN_devw_mask     (0xFFU << GNTPIN_devw_shift)
+#define GNTPIN_devw_mask     (GNTPIN_cntr_mask << GNTPIN_devw_shift)
                           /* Count of read-only device-bus mappings.  */
-#define GNTPIN_devr_shift    24
+#define GNTPIN_devr_shift    (GNTPIN_devw_shift + GNTPIN_cntr_width)
 #define GNTPIN_devr_inc      (1U << GNTPIN_devr_shift)
-#define GNTPIN_devr_mask     (0xFFU << GNTPIN_devr_shift)
+#define GNTPIN_devr_mask     (GNTPIN_cntr_mask << GNTPIN_devr_shift)
 
     domid_t       domid;  /* Domain being granted access.             */
     unsigned int  start:15; /* For sub-page grants, the start offset
@@ -988,7 +991,8 @@ map_grant_ref(
     mfn_t mfn;
     struct page_info *pg = NULL;
     int            rc = GNTST_okay;
-    unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0;
+    unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0,
+                   pin_incr = 0;
     bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
@@ -998,7 +1002,14 @@ map_grant_ref(
 
     ld = current->domain;
 
-    if ( unlikely((op->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0) )
+    if ( op->flags & GNTMAP_device_map )
+        pin_incr += (op->flags & GNTMAP_readonly) ? GNTPIN_devr_inc
+                                                  : GNTPIN_devw_inc;
+    if ( op->flags & GNTMAP_host_map )
+        pin_incr += (op->flags & GNTMAP_readonly) ? GNTPIN_hstr_inc
+                                                  : GNTPIN_hstw_inc;
+
+    if ( unlikely(!pin_incr) )
     {
         gdprintk(XENLOG_INFO, "Bad flags in grant map op: %x\n", op->flags);
         op->status = GNTST_bad_gntref;
@@ -1052,19 +1063,19 @@ map_grant_ref(
     shah = shared_entry_header(rgt, ref);
     act = active_entry_acquire(rgt, ref);
 
-    /* Make sure we do not access memory speculatively */
-    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
-                                                 : &status_entry(rgt, ref);
-
     /* If already pinned, check the active domid and avoid refcnt overflow. */
     if ( act->pin &&
          ((act->domid != ld->domain_id) ||
-          (act->pin & 0x80808080U) != 0 ||
+          (act->pin & (pin_incr << (GNTPIN_cntr_width - 1))) ||
           (act->is_sub_page)) )
         PIN_FAIL(act_release_out, GNTST_general_error,
                  "Bad domain (%d != %d), or risk of counter overflow %08x, or subpage %d\n",
                  act->domid, ld->domain_id, act->pin, act->is_sub_page);
 
+    /* Make sure we do not access memory speculatively */
+    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
+                                                   : &status_entry(rgt, ref);
+
     if ( !act->pin ||
          (!(op->flags & GNTMAP_readonly) &&
           !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask))) )
@@ -1095,12 +1106,7 @@ map_grant_ref(
         }
     }
 
-    if ( op->flags & GNTMAP_device_map )
-        act->pin += (op->flags & GNTMAP_readonly) ?
-            GNTPIN_devr_inc : GNTPIN_devw_inc;
-    if ( op->flags & GNTMAP_host_map )
-        act->pin += (op->flags & GNTMAP_readonly) ?
-            GNTPIN_hstr_inc : GNTPIN_hstw_inc;
+    act->pin += pin_incr;
 
     mfn = act->mfn;
 
@@ -1275,13 +1281,7 @@ map_grant_ref(
     grant_read_lock(rgt);
 
     act = active_entry_acquire(rgt, op->ref);
-
-    if ( op->flags & GNTMAP_device_map )
-        act->pin -= (op->flags & GNTMAP_readonly) ?
-            GNTPIN_devr_inc : GNTPIN_devw_inc;
-    if ( op->flags & GNTMAP_host_map )
-        act->pin -= (op->flags & GNTMAP_readonly) ?
-            GNTPIN_hstr_inc : GNTPIN_hstw_inc;
+    act->pin -= pin_incr;
 
  unlock_out_clear:
     if ( !(op->flags & GNTMAP_readonly) &&
@@ -2516,6 +2516,7 @@ acquire_grant_for_copy(
     uint16_t trans_length;
     bool is_sub_page;
     s16 rc = GNTST_okay;
+    unsigned int pin_incr = readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
     unsigned int clear_flags = 0;
 
     *page = NULL;
@@ -2530,6 +2531,14 @@ acquire_grant_for_copy(
     shah = shared_entry_header(rgt, gref);
     act = active_entry_acquire(rgt, gref);
 
+    /* If already pinned, check the active domid and avoid refcnt overflow. */
+    if ( act->pin &&
+         ((act->domid != ldom) ||
+          (act->pin & (pin_incr << (GNTPIN_cntr_width - 1)))) )
+        PIN_FAIL(unlock_out, GNTST_general_error,
+                 "Bad domain (%d != %d), or risk of counter overflow %08x\n",
+                 act->domid, ldom, act->pin);
+
     if ( evaluate_nospec(rgt->gt_version == 1) )
     {
         sha2 = NULL;
@@ -2541,12 +2550,6 @@ acquire_grant_for_copy(
         status = &status_entry(rgt, gref);
     }
 
-    /* If already pinned, check the active domid and avoid refcnt overflow. */
-    if ( act->pin && ((act->domid != ldom) || (act->pin & 0x80808080U) != 0) )
-        PIN_FAIL(unlock_out, GNTST_general_error,
-                 "Bad domain (%d != %d), or risk of counter overflow %08x\n",
-                 act->domid, ldom, act->pin);
-
     old_pin = act->pin;
     if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
     {
@@ -2728,7 +2731,7 @@ acquire_grant_for_copy(
         }
     }
 
-    act->pin += readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
+    act->pin += pin_incr;
 
     *page_off = act->start;
     *length = act->length;

[PATCH 2/3] gnttab: consolidate pin-to-status syncing

[Jan Beulich]

Forever since the fix for XSA-230 the 2nd of the comments ahead of
fixup_status_for_copy_pin() has been stale - there's nothing specific to
transitive grants there anymore.

Move the function up, drop the "copy" part from its name again, add a
"readonly" parameter, and use it also on other paths having decremented
one (or not having got to increment any) of the pin counts.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -908,6 +908,25 @@ static int _set_status(const grant_entry
         return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
 }
 
+/*
+ * The status for a grant may indicate that we're taking more access than
+ * the pin requires.  Fix up the status to match the pin.  Called with the
+ * domain's grant table lock held at least in read mode and with the active
+ * entry lock held (iow act->pin can't change behind our backs).
+ */
+static void fixup_status_for_pin(struct domain *rd,
+                                 const struct active_grant_entry *act,
+                                 uint16_t *status, bool readonly)
+{
+    unsigned int clear_flags = act->pin ? 0 : GTF_reading;
+
+    if ( !readonly && !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
+        clear_flags |= GTF_writing;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
+}
+
 static struct active_grant_entry *grant_map_exists(const struct domain *ld,
                                                    struct grant_table *rgt,
                                                    mfn_t mfn,
@@ -991,8 +1010,7 @@ map_grant_ref(
     mfn_t mfn;
     struct page_info *pg = NULL;
     int            rc = GNTST_okay;
-    unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0,
-                   pin_incr = 0;
+    unsigned int   cache_flags, refcnt = 0, typecnt = 0, pin_incr = 0;
     bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
@@ -1284,15 +1302,7 @@ map_grant_ref(
     act->pin -= pin_incr;
 
  unlock_out_clear:
-    if ( !(op->flags & GNTMAP_readonly) &&
-         !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
-        clear_flags |= GTF_writing;
-
-    if ( !act->pin )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
+    fixup_status_for_pin(rd, act, status, op->flags & GNTMAP_readonly);
 
  act_release_out:
     active_entry_release(act);
@@ -1507,7 +1517,6 @@ unmap_common_complete(struct gnttab_unma
     grant_entry_header_t *sha;
     struct page_info *pg;
     uint16_t *status;
-    unsigned int clear_flags = 0;
 
     if ( evaluate_nospec(!op->done) )
     {
@@ -1566,15 +1575,7 @@ unmap_common_complete(struct gnttab_unma
             act->pin -= GNTPIN_hstw_inc;
     }
 
-    if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) &&
-         !(op->done & GNTMAP_readonly) )
-        clear_flags |= GTF_writing;
-
-    if ( act->pin == 0 )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
+    fixup_status_for_pin(rd, act, status, op->done & GNTMAP_readonly);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2414,7 +2415,6 @@ release_grant_for_copy(
     uint16_t *status;
     grant_ref_t trans_gref;
     struct domain *td;
-    unsigned int clear_flags = 0;
 
     grant_read_lock(rgt);
 
@@ -2444,15 +2444,9 @@ release_grant_for_copy(
         gnttab_mark_dirty(rd, mfn);
 
         act->pin -= GNTPIN_hstw_inc;
-        if ( !(act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) )
-            clear_flags |= GTF_writing;
     }
 
-    if ( !act->pin )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
+    fixup_status_for_pin(rd, act, status, readonly);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2469,27 +2463,6 @@ release_grant_for_copy(
     }
 }
 
-/* The status for a grant indicates that we're taking more access than
-   the pin requires.  Fix up the status to match the pin.  Called
-   under the domain's grant table lock. */
-/* Only safe on transitive grants.  Even then, note that we don't
-   attempt to drop any pin on the referent grant. */
-static void fixup_status_for_copy_pin(struct domain *rd,
-                                      const struct active_grant_entry *act,
-                                      uint16_t *status)
-{
-    unsigned int clear_flags = 0;
-
-    if ( !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
-        clear_flags |= GTF_writing;
-
-    if ( !act->pin )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
-}
-
 /*
  * Grab a machine frame number from a grant entry and update the flags
  * and pin count as appropriate. If rc == GNTST_okay, note that this *does*
@@ -2517,7 +2490,6 @@ acquire_grant_for_copy(
     bool is_sub_page;
     s16 rc = GNTST_okay;
     unsigned int pin_incr = readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
-    unsigned int clear_flags = 0;
 
     *page = NULL;
 
@@ -2604,8 +2576,8 @@ acquire_grant_for_copy(
 
         if ( rc != GNTST_okay )
         {
-            fixup_status_for_copy_pin(rd, act, status);
             rcu_unlock_domain(td);
+            fixup_status_for_pin(rd, act, status, readonly);
             active_entry_release(act);
             grant_read_unlock(rgt);
             return rc;
@@ -2627,8 +2599,8 @@ acquire_grant_for_copy(
                           !act->is_sub_page)) )
         {
             release_grant_for_copy(td, trans_gref, readonly);
-            fixup_status_for_copy_pin(rd, act, status);
             rcu_unlock_domain(td);
+            fixup_status_for_pin(rd, act, status, readonly);
             active_entry_release(act);
             grant_read_unlock(rgt);
             put_page(*page);
@@ -2742,15 +2714,7 @@ acquire_grant_for_copy(
     return rc;
 
  unlock_out_clear:
-    if ( !(readonly) &&
-         !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
-        clear_flags |= GTF_writing;
-
-    if ( !act->pin )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
+    fixup_status_for_pin(rd, act, status, readonly);
 
  unlock_out:
     active_entry_release(act);
@@ -3720,8 +3684,6 @@ gnttab_release_mappings(
 
     for ( handle = 0; handle < gt->maptrack_limit; handle++ )
     {
-        unsigned int clear_flags = 0;
-
         map = &maptrack_entry(gt, handle);
         if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
             continue;
@@ -3794,16 +3756,9 @@ gnttab_release_mappings(
                     put_page(pg);
                 }
             }
-
-            if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 )
-                clear_flags |= GTF_writing;
         }
 
-        if ( act->pin == 0 )
-            clear_flags |= GTF_reading;
-
-        if ( clear_flags )
-            gnttab_clear_flags(rd, clear_flags, status);
+        fixup_status_for_pin(rd, act, status, map->flags & GNTMAP_readonly);
 
         active_entry_release(act);
         grant_read_unlock(rgt);

[PATCH 3/3] Arm: don't hard-code grant table limits in create_domUs()

[Jan Beulich]

I can only assume that f2ae59bc4b9b ("Rationalize max_grant_frames and
max_maptrack_frames handling") unintentionally left Arm's create_domUs()
set limits to explicit values, as at least some of the same constraints
apply here.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2480,8 +2480,8 @@ void __init create_domUs(void)
             .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
             .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
             .max_evtchn_port = -1,
-            .max_grant_frames = 64,
-            .max_maptrack_frames = 1024,
+            .max_grant_frames = -1,
+            .max_maptrack_frames = -1,
         };
 
         if ( !dt_device_is_compatible(node, "xen,domain") )

Re: [PATCH 3/3] Arm: don't hard-code grant table limits in create_domUs()

[Stefano Stabellini]

On Thu, 14 Jan 2021, Jan Beulich wrote:
> I can only assume that f2ae59bc4b9b ("Rationalize max_grant_frames and
> max_maptrack_frames handling") unintentionally left Arm's create_domUs()
> set limits to explicit values, as at least some of the same constraints
> apply here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2480,8 +2480,8 @@ void __init create_domUs(void)
>              .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
>              .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>              .max_evtchn_port = -1,
> -            .max_grant_frames = 64,
> -            .max_maptrack_frames = 1024,
> +            .max_grant_frames = -1,
> +            .max_maptrack_frames = -1,
>          };
>  
>          if ( !dt_device_is_compatible(node, "xen,domain") )
> 

Re: [PATCH 1/3] gnttab: adjust pin count overflow checks

[Andrew Cooper]

On 14/01/2021 15:23, Jan Beulich wrote:
> It's at least odd to check counters which aren't going to be
> incremented. And it's also not helpful to use open-coded literal numbers
> in these checks.
>
> Calculate the increment values first and derive from them the mask to
> use in the checks.
>
> Also move the pin count checks ahead of the calculation of the status
> (and for copy also sha2) pointers: They're not needed in the failure
> cases, and this way the compiler may also have an easier time keeping
> the variables at least transiently in registers for the subsequent uses.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -323,22 +323,25 @@ shared_entry_header(struct grant_table *
>  /* Active grant entry - used for shadowing GTF_permit_access grants. */
>  struct active_grant_entry {
>      uint32_t      pin;    /* Reference count information:             */
> +                          /* Width of the individual counter fields.  */
> +#define GNTPIN_cntr_width    8
> +#define GNTPIN_cntr_mask     ((1U << GNTPIN_cntr_width) - 1)
>                            /* Count of writable host-CPU mappings.     */
>  #define GNTPIN_hstw_shift    0
>  #define GNTPIN_hstw_inc      (1U << GNTPIN_hstw_shift)
> -#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
> +#define GNTPIN_hstw_mask     (GNTPIN_cntr_mask << GNTPIN_hstw_shift)
>                            /* Count of read-only host-CPU mappings.    */
> -#define GNTPIN_hstr_shift    8
> +#define GNTPIN_hstr_shift    (GNTPIN_hstw_shift + GNTPIN_cntr_width)

While this patch is by-and-large an improvement, it unfortunately
further hides how the pin field works, which is already clear-as-mud.

I'd recommend replacing the "Reference count information:" comment with:

/*
 * 4x byte-wide reference counts, for {host,device}{read,write}
 * mappings, implemented as a single 32bit presumably to optimise
 * checking for any reference.
 */
uint32_t      pin;


I still can't make up my mind as to whether this is a sensible
optimisation.  It is borderline obfuscated code, due to having a totally
undocumented and weird data arrangement.

> @@ -1052,19 +1063,19 @@ map_grant_ref(
>      shah = shared_entry_header(rgt, ref);
>      act = active_entry_acquire(rgt, ref);
>  
> -    /* Make sure we do not access memory speculatively */
> -    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
> -                                                 : &status_entry(rgt, ref);
> -
>      /* If already pinned, check the active domid and avoid refcnt overflow. */
>      if ( act->pin &&
>           ((act->domid != ld->domain_id) ||
> -          (act->pin & 0x80808080U) != 0 ||
> +          (act->pin & (pin_incr << (GNTPIN_cntr_width - 1))) ||

This, I'm afraid, is not an improvement.  What we want is:

#define GNTPIN_overflow_mask 0x80808080U

The reason for checking all at once is defence in depth (not strictly
necessary, but also not a problem), and in this specific example, using
a constant results in better code because pin_incr doesn't need
unspilling from stack and manipulated.

If you're happy with both of these suggestions, then Reviewed-by: Andrew
Cooper <andrew.cooper3@citrix.com> to avoid a repost.

~Andrew

Re: [PATCH 2/3] gnttab: consolidate pin-to-status syncing

[Andrew Cooper]

On 14/01/2021 15:23, Jan Beulich wrote:
> Forever since the fix for XSA-230 the 2nd of the comments ahead of
> fixup_status_for_copy_pin() has been stale - there's nothing specific to
> transitive grants there anymore.
>
> Move the function up, drop the "copy" part from its name again, add a
> "readonly" parameter, and use it also on other paths having decremented
> one (or not having got to increment any) of the pin counts.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -908,6 +908,25 @@ static int _set_status(const grant_entry
>          return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
>  }
>  
> +/*
> + * The status for a grant may indicate that we're taking more access than
> + * the pin requires.  Fix up the status to match the pin.

This sentence isn't correct.  It will only clear status flags to match a
reduction in the pinned references.  IOW it cannot be used in the case
that a grant goes from unpinned to pinned.

How about renaming to clear_status_for_pin() to make this clearer?  I
don't think it is worth trying to make the operation more generic.

If so (and with a suitable adjustment to the comment), Reviewed-by:
Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 1/3] gnttab: adjust pin count overflow checks

[Jan Beulich]

On 15.01.2021 14:09, Andrew Cooper wrote:
> On 14/01/2021 15:23, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -323,22 +323,25 @@ shared_entry_header(struct grant_table *
>>  /* Active grant entry - used for shadowing GTF_permit_access grants. */
>>  struct active_grant_entry {
>>      uint32_t      pin;    /* Reference count information:             */
>> +                          /* Width of the individual counter fields.  */
>> +#define GNTPIN_cntr_width    8
>> +#define GNTPIN_cntr_mask     ((1U << GNTPIN_cntr_width) - 1)
>>                            /* Count of writable host-CPU mappings.     */
>>  #define GNTPIN_hstw_shift    0
>>  #define GNTPIN_hstw_inc      (1U << GNTPIN_hstw_shift)
>> -#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
>> +#define GNTPIN_hstw_mask     (GNTPIN_cntr_mask << GNTPIN_hstw_shift)
>>                            /* Count of read-only host-CPU mappings.    */
>> -#define GNTPIN_hstr_shift    8
>> +#define GNTPIN_hstr_shift    (GNTPIN_hstw_shift + GNTPIN_cntr_width)
> 
> While this patch is by-and-large an improvement, it unfortunately
> further hides how the pin field works, which is already clear-as-mud.
> 
> I'd recommend replacing the "Reference count information:" comment with:
> 
> /*
>  * 4x byte-wide reference counts, for {host,device}{read,write}
>  * mappings, implemented as a single 32bit presumably to optimise
>  * checking for any reference.
>  */
> uint32_t      pin;

Sure, added.

>> @@ -1052,19 +1063,19 @@ map_grant_ref(
>>      shah = shared_entry_header(rgt, ref);
>>      act = active_entry_acquire(rgt, ref);
>>  
>> -    /* Make sure we do not access memory speculatively */
>> -    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
>> -                                                 : &status_entry(rgt, ref);
>> -
>>      /* If already pinned, check the active domid and avoid refcnt overflow. */
>>      if ( act->pin &&
>>           ((act->domid != ld->domain_id) ||
>> -          (act->pin & 0x80808080U) != 0 ||
>> +          (act->pin & (pin_incr << (GNTPIN_cntr_width - 1))) ||
> 
> This, I'm afraid, is not an improvement.  What we want is:
> 
> #define GNTPIN_overflow_mask 0x80808080U
> 
> The reason for checking all at once is defence in depth (not strictly
> necessary, but also not a problem),

How is this not a problem? There is absolutely no reason to
reject a request just because some unrelated field may be
about to overflow. In fact I now think that I didn't even
leverage the full potential - the "act->pin != old_pin" check
could also be relaxed this way, I think. Just that it sits on
a path we probably don't really care about very much.

> and in this specific example, using
> a constant results in better code because pin_incr doesn't need
> unspilling from stack and manipulated.

That's, I think, an acceptable price to pay for getting the
checking correct.

> If you're happy with both of these suggestions, then Reviewed-by: Andrew
> Cooper <andrew.cooper3@citrix.com> to avoid a repost.

Thanks, but as per above - not yet. Plus if I made this 2nd
suggested change, the patch would change significantly (the
new local variable would then become pointless in at least
acquire_grant_for_copy()), which I think would better involve
re-posting then.

Jan

Re: [PATCH 2/3] gnttab: consolidate pin-to-status syncing

[Jan Beulich]

On 15.01.2021 14:25, Andrew Cooper wrote:
> On 14/01/2021 15:23, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -908,6 +908,25 @@ static int _set_status(const grant_entry
>>          return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
>>  }
>>  
>> +/*
>> + * The status for a grant may indicate that we're taking more access than
>> + * the pin requires.  Fix up the status to match the pin.
> 
> This sentence isn't correct.  It will only clear status flags to match a
> reduction in the pinned references.  IOW it cannot be used in the case
> that a grant goes from unpinned to pinned.

Of course not, hence "... more access than ...". But yes, I can
replace "Fix up" by "Reduce" if you think the wording isn't
unambiguous enough.

> How about renaming to clear_status_for_pin() to make this clearer?  I
> don't think it is worth trying to make the operation more generic.

Hmm, this name would suggest to me that the function clears
whatever the present pin count requires (e.g. acting on the
pre-update value when the post-update one is [going to be]
zero). Maybe reduce_status_for_pin(), matching the suggested
comment wording change?

> If so (and with a suitable adjustment to the comment), Reviewed-by:
> Andrew Cooper <andrew.cooper3@citrix.com>

As per above, I'll first wait for your further reply.

Jan

Re: [PATCH 1/3] gnttab: adjust pin count overflow checks

[Andrew Cooper]

On 15/01/2021 13:26, Jan Beulich wrote:
> On 15.01.2021 14:09, Andrew Cooper wrote:
>> On 14/01/2021 15:23, Jan Beulich wrote:
>>> @@ -1052,19 +1063,19 @@ map_grant_ref(
>>>      shah = shared_entry_header(rgt, ref);
>>>      act = active_entry_acquire(rgt, ref);
>>>  
>>> -    /* Make sure we do not access memory speculatively */
>>> -    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
>>> -                                                 : &status_entry(rgt, ref);
>>> -
>>>      /* If already pinned, check the active domid and avoid refcnt overflow. */
>>>      if ( act->pin &&
>>>           ((act->domid != ld->domain_id) ||
>>> -          (act->pin & 0x80808080U) != 0 ||
>>> +          (act->pin & (pin_incr << (GNTPIN_cntr_width - 1))) ||
>> This, I'm afraid, is not an improvement.  What we want is:
>>
>> #define GNTPIN_overflow_mask 0x80808080U
>>
>> The reason for checking all at once is defence in depth (not strictly
>> necessary, but also not a problem),
> How is this not a problem? There is absolutely no reason to
> reject a request just because some unrelated field may be
> about to overflow. In fact I now think that I didn't even
> leverage the full potential - the "act->pin != old_pin" check
> could also be relaxed this way, I think. Just that it sits on
> a path we probably don't really care about very much.

Hmm - I see your point.  I'd missed the fact that a previous operation
could leave this timebomb waiting for us.  (Probably wants a note to
this effect in the commit message).

However we go about fixing it, "pin_incr << (GNTPIN_cntr_width - 1)" is
too obscure IMO.  Perhaps all we need is a

#define GNTPIN_overflow_mask(x) ((x) << (GNTPIN_cntr_width - 1))

but it does occur to me that this logic only works to being with when
pin_incr is strictly 1 in the relevant bytes.

~Andrew

Re: [PATCH 2/3] gnttab: consolidate pin-to-status syncing

[Andrew Cooper]

On 15/01/2021 13:33, Jan Beulich wrote:
> On 15.01.2021 14:25, Andrew Cooper wrote:
>> On 14/01/2021 15:23, Jan Beulich wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -908,6 +908,25 @@ static int _set_status(const grant_entry
>>>          return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
>>>  }
>>>  
>>> +/*
>>> + * The status for a grant may indicate that we're taking more access than
>>> + * the pin requires.  Fix up the status to match the pin.
>> This sentence isn't correct.  It will only clear status flags to match a
>> reduction in the pinned references.  IOW it cannot be used in the case
>> that a grant goes from unpinned to pinned.
> Of course not, hence "... more access than ...". But yes, I can
> replace "Fix up" by "Reduce" if you think the wording isn't
> unambiguous enough.
>
>> How about renaming to clear_status_for_pin() to make this clearer?  I
>> don't think it is worth trying to make the operation more generic.
> Hmm, this name would suggest to me that the function clears
> whatever the present pin count requires (e.g. acting on the
> pre-update value when the post-update one is [going to be]
> zero). Maybe reduce_status_for_pin(), matching the suggested
> comment wording change?

Sounds good.  My R-by stands with this change.

~Andrew

Re: [PATCH 1/3] gnttab: adjust pin count overflow checks

[Jan Beulich]

On 15.01.2021 14:35, Andrew Cooper wrote:
> On 15/01/2021 13:26, Jan Beulich wrote:
>> On 15.01.2021 14:09, Andrew Cooper wrote:
>>> On 14/01/2021 15:23, Jan Beulich wrote:
>>>> @@ -1052,19 +1063,19 @@ map_grant_ref(
>>>>      shah = shared_entry_header(rgt, ref);
>>>>      act = active_entry_acquire(rgt, ref);
>>>>  
>>>> -    /* Make sure we do not access memory speculatively */
>>>> -    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
>>>> -                                                 : &status_entry(rgt, ref);
>>>> -
>>>>      /* If already pinned, check the active domid and avoid refcnt overflow. */
>>>>      if ( act->pin &&
>>>>           ((act->domid != ld->domain_id) ||
>>>> -          (act->pin & 0x80808080U) != 0 ||
>>>> +          (act->pin & (pin_incr << (GNTPIN_cntr_width - 1))) ||
>>> This, I'm afraid, is not an improvement.  What we want is:
>>>
>>> #define GNTPIN_overflow_mask 0x80808080U
>>>
>>> The reason for checking all at once is defence in depth (not strictly
>>> necessary, but also not a problem),
>> How is this not a problem? There is absolutely no reason to
>> reject a request just because some unrelated field may be
>> about to overflow. In fact I now think that I didn't even
>> leverage the full potential - the "act->pin != old_pin" check
>> could also be relaxed this way, I think. Just that it sits on
>> a path we probably don't really care about very much.
> 
> Hmm - I see your point.  I'd missed the fact that a previous operation
> could leave this timebomb waiting for us.  (Probably wants a note to
> this effect in the commit message).

I've added half a sentence.

> However we go about fixing it, "pin_incr << (GNTPIN_cntr_width - 1)" is
> too obscure IMO.  Perhaps all we need is a
> 
> #define GNTPIN_overflow_mask(x) ((x) << (GNTPIN_cntr_width - 1))
> 
> but it does occur to me that this logic only works to being with when
> pin_incr is strictly 1 in the relevant bytes.

Perhaps

#define GNTPIN_overflow_mask(x) ({ \
    ASSERT(!((x) & ~(GNTPIN_hstw_inc | GNTPIN_hstr_inc | \
                     GNTPIN_devw_inc | GNTPIN_devr_inc))); \
    (x) << (GNTPIN_cntr_width - 1); \
})

? And maybe name the whole thing e.g. GNTPIN_incr2oflow_mask()
to clarify the input is an increment (which I personally take
to mean consisting of only values of 1, but I realize views
may vary)?

Jan

Re: [PATCH 1/3] gnttab: adjust pin count overflow checks

[Andrew Cooper]

On 15/01/2021 14:21, Jan Beulich wrote:
> On 15.01.2021 14:35, Andrew Cooper wrote:
>> On 15/01/2021 13:26, Jan Beulich wrote:
>>> On 15.01.2021 14:09, Andrew Cooper wrote:
>>>> On 14/01/2021 15:23, Jan Beulich wrote:
>>>>> @@ -1052,19 +1063,19 @@ map_grant_ref(
>>>>>      shah = shared_entry_header(rgt, ref);
>>>>>      act = active_entry_acquire(rgt, ref);
>>>>>  
>>>>> -    /* Make sure we do not access memory speculatively */
>>>>> -    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
>>>>> -                                                 : &status_entry(rgt, ref);
>>>>> -
>>>>>      /* If already pinned, check the active domid and avoid refcnt overflow. */
>>>>>      if ( act->pin &&
>>>>>           ((act->domid != ld->domain_id) ||
>>>>> -          (act->pin & 0x80808080U) != 0 ||
>>>>> +          (act->pin & (pin_incr << (GNTPIN_cntr_width - 1))) ||
>>>> This, I'm afraid, is not an improvement.  What we want is:
>>>>
>>>> #define GNTPIN_overflow_mask 0x80808080U
>>>>
>>>> The reason for checking all at once is defence in depth (not strictly
>>>> necessary, but also not a problem),
>>> How is this not a problem? There is absolutely no reason to
>>> reject a request just because some unrelated field may be
>>> about to overflow. In fact I now think that I didn't even
>>> leverage the full potential - the "act->pin != old_pin" check
>>> could also be relaxed this way, I think. Just that it sits on
>>> a path we probably don't really care about very much.
>> Hmm - I see your point.  I'd missed the fact that a previous operation
>> could leave this timebomb waiting for us.  (Probably wants a note to
>> this effect in the commit message).
> I've added half a sentence.
>
>> However we go about fixing it, "pin_incr << (GNTPIN_cntr_width - 1)" is
>> too obscure IMO.  Perhaps all we need is a
>>
>> #define GNTPIN_overflow_mask(x) ((x) << (GNTPIN_cntr_width - 1))
>>
>> but it does occur to me that this logic only works to being with when
>> pin_incr is strictly 1 in the relevant bytes.
> Perhaps
>
> #define GNTPIN_overflow_mask(x) ({ \
>     ASSERT(!((x) & ~(GNTPIN_hstw_inc | GNTPIN_hstr_inc | \
>                      GNTPIN_devw_inc | GNTPIN_devr_inc))); \
>     (x) << (GNTPIN_cntr_width - 1); \
> })
>
> ? And maybe name the whole thing e.g. GNTPIN_incr2oflow_mask()
> to clarify the input is an increment (which I personally take
> to mean consisting of only values of 1, but I realize views
> may vary)?

Yeah - lets go with this.  Its a massive improvement on the current code.

~Andrew