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@7bulls.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Do we even need the hook anymore? It has been expanding the same for Arm
and x86.
--- 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));
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 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@7bulls.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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 )
On 07.05.2026 19:49, Andrew Cooper wrote:
> 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@7bulls.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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.
Okay, unless I hear otherwise (pretty soon) I'll drop it altogether in v2.
>> --- 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));
With up-front
int rc = 0;
yes. As in many other cases I'm trying to keep churn low when there's no
pretty much objectively better way of writing things. Personally I prefer
the "just an initializer" form (iirc it was also me who introduced the
above code in that shape).
Jan
© 2016 - 2026 Red Hat, Inc.