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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
>
> --- 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@xxxxxxxxxx> to avoid a repost.

~Andrew