Re: [PATCH] gnttab: simplify (replace) gnttab_set_frame_gfn()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/05/2026 12:41 pm, Jan Beulich wrote:
> It's not really doing anything for valid GFNs, which renders its one use
> site pretty pointless. The other isn't so much about setting anything, but
> rather about clearing. Rename the macro accordingly.
>
> The main point here, however, is about Rafal spotting the double
> fetching of the GFN (first in gnttab_unpopulate_status_frames(), then
> again in gnttab_set_frame_gfn()). Re-purpose the macro parameter to pass
> in the already fetched GFN, while dropping the no longer used parameters.
>
> Suggested-by: Rafal Wojtczuk <rafal.wojtczuk@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Do we even need the hook anymore? It has been expanding the same for Arm
> and x86.

At this point, I'd say no.

The macro has changed contents several times since it's introduction. 
The grant table macros especially demonstrate how poor the common/arch
interfaces are.


>
> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/grant_table.h
> @@ -50,12 +50,8 @@ int replace_grant_host_mapping(uint64_t
>  #define gnttab_dom0_frames()                                             \
>      min_t(unsigned int, opt_max_grant_frames, PFN_DOWN(_etext - _stext))
>  
> -#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
> -    (gfn_eq(gfn, INVALID_GFN)                                            \
> -     ? guest_physmap_remove_page((gt)->domain,                           \
> -                                 gnttab_get_frame_gfn(gt, st, idx),      \
> -                                 mfn, 0)                                 \
> -     : 0)
> +#define gnttab_clear_frame_gfn(gt, gfn, mfn)                             \
> +    guest_physmap_remove_page((gt)->domain, gfn, mfn, 0)
>  
>  #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
>     (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
> --- a/xen/arch/x86/include/asm/grant_table.h
> +++ b/xen/arch/x86/include/asm/grant_table.h
> @@ -32,12 +32,8 @@ static inline int replace_grant_host_map
>      return replace_grant_pv_mapping(addr, frame, new_addr, flags);
>  }
>  
> -#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
> -    (gfn_eq(gfn, INVALID_GFN)                                            \
> -     ? guest_physmap_remove_page((gt)->domain,                           \
> -                                 gnttab_get_frame_gfn(gt, st, idx),      \
> -                                 mfn, 0)                                 \
> -     : 0 /* Handled in add_to_physmap_one(). */)
> +#define gnttab_clear_frame_gfn(gt, gfn, mfn)                             \
> +    guest_physmap_remove_page((gt)->domain, gfn, mfn, 0)
>  #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
>      mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
>                        : gnttab_shared_mfn(gt, idx);                      \
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1844,8 +1844,7 @@ gnttab_unpopulate_status_frames(struct d
>          {
>              int rc = gfn_eq(gfn, INVALID_GFN)
>                       ? 0
> -                     : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN,
> -                                            page_to_mfn(pg));
> +                     : gnttab_clear_frame_gfn(gt, gfn, page_to_mfn(pg));
>  

This is just a more complex way of writing

    if ( !gfn_eq(gfn, INVALID_GFN) )
        rc = gnttab_clear_frame_gfn(gt, gfn, page_to_mfn(pg));

~Andrew

>              if ( rc )
>              {
> @@ -4285,8 +4284,6 @@ int gnttab_map_frame_begin(
>           */
>          if ( !get_page(pg, d) )
>              rc = -EBUSY;
> -        else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) )
> -            put_page(pg);
>      }
>  
>      if ( rc )