Re: [PATCH for-4.22(?) v2] gnttab: simplify (really: drop) gnttab_set_frame_gfn()

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 12-May-26 16:46, 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.
> 
> 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.
> 
> As the result is a mere wrapper around guest_physmap_remove_page(), drop
> the hook altogether.
> 
> Suggested-by: Rafal Wojtczuk <rafal.wojtczuk@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Drop hook altogether.
> 
> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/grant_table.h
> @@ -50,13 +50,6 @@ 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_get_frame_gfn(gt, st, idx) ({                             \
>     (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
>          : gnttab_shared_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,6 @@ 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_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));
> +                     : guest_physmap_remove_page(d, gfn, page_to_mfn(pg), 0);
>  
>              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)) )
status is now a variable that is set but never read. Remove it.
With that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal