Re: [PATCH 1/2] gnttab: introduce version agnostic macros

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.01.2025 14:37, Daniel P. Smith wrote:
> @@ -310,16 +311,30 @@ nr_maptrack_frames(struct grant_table *t)
>  #define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
>  #define shared_entry_v1(t, e) \
>      ((t)->shared_v1[(e)/SHGNT_PER_PAGE_V1][(e)%SHGNT_PER_PAGE_V1])
> +
> +/* Operations providing a single interface agnostic to grant table version */
>  #define SHGNT_PER_PAGE_V2 (PAGE_SIZE / sizeof(grant_entry_v2_t))
>  #define shared_entry_v2(t, e) \
>      ((t)->shared_v2[(e)/SHGNT_PER_PAGE_V2][(e)%SHGNT_PER_PAGE_V2])

The comment looks like it was misplaced, and rather wanted to go ahead of ...

> +
> +#define shared_entry_full_frame(gt, ref) \
> +    ( get_gt_version(gt) == 1 ? shared_entry_v1((gt), (ref)).frame : \
> +                                shared_entry_v2((gt), (ref)).full_page.frame 
> )
> +#define set_shared_entry(gt, ref, val) \
> +    ( get_gt_version(gt) == 1 ? (shared_entry_v1((gt), (ref)).frame = (val)) 
> : \
> +                                (shared_entry_v2((gt), 
> (ref)).full_page.frame = (val)) )
> +#define status_addr(gt, ref, flags_addr) \
> +    ( evaluate_nospec(get_gt_version(gt) == 1) ? (flags_addr) : 
> &status_entry((gt), (ref)) )

... this set of new #define-s?

Also a nit: There shouldn't be blanks immediately inside the opening parentheses
here. For readability you further want to omit parentheses around parameters
which are passed on unaltered (i.e. not becoming part of an expression). Plus
some of the lines are pretty obviously too long.

As to the single use of evaluate_nospec(): That looks odd here; on the surface
one would expect all three constructs to use it, or none of them. Deviations
from that likely require a comment, or at least some explanation in the
description.

> +
>  #define STGNT_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
>  #define status_entry(t, e) \
>      ((t)->status[(e)/STGNT_PER_PAGE][(e)%STGNT_PER_PAGE])
> +
> +

No double blank lines please.

> @@ -1090,23 +1105,20 @@ map_grant_ref(
>      }
>  
>      /* Make sure we do not access memory speculatively */
> -    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
> -                                                   : &status_entry(rgt, ref);
> +    status = status_addr(rgt, ref, &shah->flags);

I was already wondering at the macro definition: How can it be that a
version-abstracting macro is passed something that's clearly version
1 only? That is, consider the opposite of the goal of this series,
someone wanting to allow building a gnttab-v2-only hypervisor. In that
case grant_entry_header_t would ideally not be defined at all (as a
struct; there may be a placeholder type). It would then be impossible
to de-reference the pointer. Hence I think that de-referencing ought to
live inside the macro.

> @@ -1930,7 +1938,7 @@ gnttab_grow_table(struct domain *d, unsigned int 
> req_nr_frames)
>      }
>  
>      /* Status pages - version 2 */
> -    if ( evaluate_nospec(gt->gt_version > 1) )
> +    if ( evaluate_nospec(get_gt_version(gt) > 1) )

At this example: Assuming in the next patch get_gt_version() can end
up expanding to constant 1, the evaluate_nospec() would still be there,
for no good reason. I expect that wants further abstracting.

> @@ -2267,6 +2275,7 @@ gnttab_transfer(
>      mfn_t mfn;
>      unsigned int max_bitsize;
>      struct active_grant_entry *act;
> +    unsigned long frame;

Just going from indentation, ...

> @@ -2354,7 +2363,7 @@ gnttab_transfer(
>          }
>  
>          max_bitsize = domain_clamp_alloc_bitsize(
> -            e, e->grant_table->gt_version > 1 || paging_mode_translate(e)
> +            e, get_gt_version(e->grant_table) > 1 || paging_mode_translate(e)
>                 ? BITS_PER_LONG + PAGE_SHIFT : 32 + PAGE_SHIFT);
>          if ( max_bitsize < BITS_PER_LONG + PAGE_SHIFT &&
>               (mfn_x(mfn) >> (max_bitsize - PAGE_SHIFT)) )
> @@ -2452,22 +2461,12 @@ gnttab_transfer(
>          grant_read_lock(e->grant_table);
>          act = active_entry_acquire(e->grant_table, gop.ref);
>  
> -        if ( evaluate_nospec(e->grant_table->gt_version == 1) )
> -        {
> -            grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, 
> gop.ref);
> +        frame = shared_entry_full_frame(e->grant_table, gop.ref);
> +        rc = guest_physmap_add_page(e, _gfn(frame), mfn, 0);
>  
> -            rc = guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
> -            if ( !paging_mode_translate(e) )
> -                sha->frame = mfn_x(mfn);
> -        }
> -        else
> -        {
> -            grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, 
> gop.ref);
> +        if ( !paging_mode_translate(e) )
> +            set_shared_entry(e->grant_table, gop.ref, mfn_x(mfn));
>  
> -            rc = guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 
> 0);
> -            if ( !paging_mode_translate(e) )
> -                sha->full_page.frame = mfn_x(mfn);
> -        }
>          smp_wmb();
>          shared_entry_header(e->grant_table, gop.ref)->flags |=
>              GTF_transfer_completed;

... the variable looks to want declaring in a more narrow scope.

Also, related to an earlier comment: Where did the evaluate_nospec() go?

> @@ -2512,16 +2511,15 @@ release_grant_for_copy(
>      act = active_entry_acquire(rgt, gref);
>      sha = shared_entry_header(rgt, gref);
>      mfn = act->mfn;
> +    status = status_addr(rgt, gref, &sha->flags);
>  
> -    if ( evaluate_nospec(rgt->gt_version == 1) )
> +    if ( evaluate_nospec(get_gt_version(rgt) == 1) )
>      {
> -        status = &sha->flags;
>          td = rd;
>          trans_gref = gref;
>      }
>      else
>      {
> -        status = &status_entry(rgt, gref);
>          td = (act->src_domid == rd->domain_id)
>               ? rd : knownalive_domain_from_domid(act->src_domid);
>          trans_gref = act->trans_gref;

Same here - you're moving code across that speculation barrier.
Anything like this - assuming it is deliberate in the first place -
needs justifying.

> @@ -2573,7 +2571,6 @@ acquire_grant_for_copy(
>      struct active_grant_entry *act;
>      grant_status_t *status;
>      uint32_t old_pin;
> -    domid_t trans_domid;
>      grant_ref_t trans_gref;
>      struct domain *td;
>      mfn_t grant_mfn;

I fear I understand neither the movement of this variable, ...

> @@ -2597,6 +2594,7 @@ acquire_grant_for_copy(
>      /* This call also ensures the above check cannot be passed speculatively 
> */
>      shah = shared_entry_header(rgt, gref);
>      act = active_entry_acquire(rgt, gref);
> +    old_pin = act->pin;

... nor that of this assignment ...

> @@ -2610,7 +2608,7 @@ acquire_grant_for_copy(
>          goto unlock_out;
>      }
>  
> -    if ( evaluate_nospec(rgt->gt_version == 1) )
> +    if ( evaluate_nospec(get_gt_version(rgt) == 1) )
>      {
>          sha2 = NULL;
>          status = &shah->flags;
> @@ -2621,9 +2619,10 @@ acquire_grant_for_copy(
>          status = &status_entry(rgt, gref);
>      }
>  
> -    old_pin = act->pin;

... (from here).

>      if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
>      {
> +        domid_t trans_domid;

This makes me guess you want to narrow the scope of the variable, yet
then: How's that related to the subject of this patch?

> @@ -3878,10 +3877,7 @@ int gnttab_release_mappings(struct domain *d)
>  
>          act = active_entry_acquire(rgt, ref);
>          sha = shared_entry_header(rgt, ref);
> -        if ( rgt->gt_version == 1 )
> -            status = &sha->flags;
> -        else
> -            status = &status_entry(rgt, ref);
> +        status = status_addr(rgt, ref, &sha->flags);

Again relating to earlier comments, yet this time the other way around:
Suddenly we're gaining evaluate_nospec() here, for no apparent reason.

> @@ -4256,7 +4247,7 @@ int gnttab_map_frame(struct domain *d, unsigned long 
> idx, gfn_t gfn, mfn_t *mfn)
>  
>      grant_write_lock(gt);
>  
> -    if ( evaluate_nospec(gt->gt_version == 2) && (idx & 
> XENMAPIDX_grant_table_status) )
> +    if ( evaluate_nospec(get_gt_version(gt) == 2) && (idx & 
> XENMAPIDX_grant_table_status) )

I realize the line was already too long, but since you touch it you also
want to wrap it.

Jan