[PATCH] x86/P2M: correct type use in p2m_put_gfn()

[PATCH] x86/P2M: correct type use in p2m_put_gfn()

[Jan Beulich]

Everywhere else gfn_t are passed into respective GFN locking macros: Do so
here as well.

Amends: 819cdc5a7301 ("x86/p2m: re-arrange {,__}put_gfn()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Easy to spot by adding ASSERT(!gfn_eq(g, INVALID_GFN)) to the respective
macros. While imo that should be a correct thing to do (as with
hypothetical split locks a valid GFN would really need passing in, in
order to be able to figure out which lock to use), we can't do so right
now: The lock is acquired ahead of respective checking in a number of
places, e.g. in p2m_get_gfn_type_access().

There's no clear Fixes: tag to use, I think - the problem looks to have
been introduced by the gradual conversion to gfn_t. I probably should not
have added gfn_x() in the referenced commit, to unbreak things already at
that time.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -338,9 +338,9 @@ mfn_t p2m_get_gfn_type_access(struct p2m
 
 void p2m_put_gfn(struct p2m_domain *p2m, gfn_t gfn)
 {
-    ASSERT(gfn_locked_by_me(p2m, gfn_x(gfn)));
+    ASSERT(gfn_locked_by_me(p2m, gfn));
 
-    gfn_unlock(p2m, gfn_x(gfn), 0);
+    gfn_unlock(p2m, gfn, 0);
 }
 
 static struct page_info *get_page_from_mfn_and_type(

Re: [PATCH] x86/P2M: correct type use in p2m_put_gfn()

[Roger Pau Monné]

On Tue, Feb 03, 2026 at 03:01:27PM +0100, Jan Beulich wrote:
> Everywhere else gfn_t are passed into respective GFN locking macros: Do so
> here as well.
> 
> Amends: 819cdc5a7301 ("x86/p2m: re-arrange {,__}put_gfn()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> Easy to spot by adding ASSERT(!gfn_eq(g, INVALID_GFN)) to the respective
> macros. While imo that should be a correct thing to do (as with
> hypothetical split locks a valid GFN would really need passing in, in
> order to be able to figure out which lock to use), we can't do so right
> now: The lock is acquired ahead of respective checking in a number of
> places, e.g. in p2m_get_gfn_type_access().

Could we convert those macros into static inlines?  It's dangerous to
use macros like those when the parameters are dropped, as the
parameter is not evaluated at all.

Thanks, Roger.

Re: [PATCH] x86/P2M: correct type use in p2m_put_gfn()

[Jan Beulich]

On 04.02.2026 08:35, Roger Pau Monné wrote:
> On Tue, Feb 03, 2026 at 03:01:27PM +0100, Jan Beulich wrote:
>> Everywhere else gfn_t are passed into respective GFN locking macros: Do so
>> here as well.
>>
>> Amends: 819cdc5a7301 ("x86/p2m: re-arrange {,__}put_gfn()")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> Easy to spot by adding ASSERT(!gfn_eq(g, INVALID_GFN)) to the respective
>> macros. While imo that should be a correct thing to do (as with
>> hypothetical split locks a valid GFN would really need passing in, in
>> order to be able to figure out which lock to use), we can't do so right
>> now: The lock is acquired ahead of respective checking in a number of
>> places, e.g. in p2m_get_gfn_type_access().
> 
> Could we convert those macros into static inlines?  It's dangerous to
> use macros like those when the parameters are dropped, as the
> parameter is not evaluated at all.

It is. Seeing how the header is used, converting may be possible. There's
one slight concern I'd have with doing so: It would move us one step
closer to giving the impression that the arguments passed are correct at
all use sites (while as long as they're entirely ignored, that's kind of
a hint that they may need checking). I can't point at it right now, but
I'm pretty sure I had come across at least one place where they're pretty
clearly wrong.

Jan

Re: [PATCH] x86/P2M: correct type use in p2m_put_gfn()

[Roger Pau Monné]

On Wed, Feb 04, 2026 at 08:49:53AM +0100, Jan Beulich wrote:
> On 04.02.2026 08:35, Roger Pau Monné wrote:
> > On Tue, Feb 03, 2026 at 03:01:27PM +0100, Jan Beulich wrote:
> >> Everywhere else gfn_t are passed into respective GFN locking macros: Do so
> >> here as well.
> >>
> >> Amends: 819cdc5a7301 ("x86/p2m: re-arrange {,__}put_gfn()")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> ---
> >> Easy to spot by adding ASSERT(!gfn_eq(g, INVALID_GFN)) to the respective
> >> macros. While imo that should be a correct thing to do (as with
> >> hypothetical split locks a valid GFN would really need passing in, in
> >> order to be able to figure out which lock to use), we can't do so right
> >> now: The lock is acquired ahead of respective checking in a number of
> >> places, e.g. in p2m_get_gfn_type_access().
> > 
> > Could we convert those macros into static inlines?  It's dangerous to
> > use macros like those when the parameters are dropped, as the
> > parameter is not evaluated at all.
> 
> It is. Seeing how the header is used, converting may be possible. There's
> one slight concern I'd have with doing so: It would move us one step
> closer to giving the impression that the arguments passed are correct at
> all use sites (while as long as they're entirely ignored, that's kind of
> a hint that they may need checking). I can't point at it right now, but
> I'm pretty sure I had come across at least one place where they're pretty
> clearly wrong.

Well, having at least the type check is better than not checking
anything at all.  By clearly wrong you mean passing INVALID_GFN, or a
random GFN that had something do to with the context?

Thanks, Roger.

Re: [PATCH] x86/P2M: correct type use in p2m_put_gfn()

[Jan Beulich]

On 04.02.2026 08:54, Roger Pau Monné wrote:
> On Wed, Feb 04, 2026 at 08:49:53AM +0100, Jan Beulich wrote:
>> On 04.02.2026 08:35, Roger Pau Monné wrote:
>>> On Tue, Feb 03, 2026 at 03:01:27PM +0100, Jan Beulich wrote:
>>>> ---
>>>> Easy to spot by adding ASSERT(!gfn_eq(g, INVALID_GFN)) to the respective
>>>> macros. While imo that should be a correct thing to do (as with
>>>> hypothetical split locks a valid GFN would really need passing in, in
>>>> order to be able to figure out which lock to use), we can't do so right
>>>> now: The lock is acquired ahead of respective checking in a number of
>>>> places, e.g. in p2m_get_gfn_type_access().
>>>
>>> Could we convert those macros into static inlines?  It's dangerous to
>>> use macros like those when the parameters are dropped, as the
>>> parameter is not evaluated at all.
>>
>> It is. Seeing how the header is used, converting may be possible. There's
>> one slight concern I'd have with doing so: It would move us one step
>> closer to giving the impression that the arguments passed are correct at
>> all use sites (while as long as they're entirely ignored, that's kind of
>> a hint that they may need checking). I can't point at it right now, but
>> I'm pretty sure I had come across at least one place where they're pretty
>> clearly wrong.
> 
> Well, having at least the type check is better than not checking
> anything at all.  By clearly wrong you mean passing INVALID_GFN, or a
> random GFN that had something do to with the context?

What I seem to recall is a bogus order value being passed somewhere.

Jan