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@7bulls.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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)) )
- put_page(pg);
}
if ( rc )
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@7bulls.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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@amd.com>
~Michal
On 18.05.2026 20:00, Orzel, Michal wrote:
> On 12-May-26 16:46, Jan Beulich wrote:
>> --- 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.
Oh, good point. Thanks for spotting.
> With that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Thanks.
Jan
On 5/12/26 4:46 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. > > 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@7bulls.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> Thanks. ~ Oleksii
On 12/05/2026 3:46 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. > > 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@7bulls.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
© 2016 - 2026 Red Hat, Inc.