Re: [PATCH v2 1/2] x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.02.2024 07:32, George Dunlap wrote:
> On Thu, Jun 23, 2022 at 7:54 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
>> Grant P2M entries, which are covered by p2m_is_any_ram(), wouldn't pass
>> the get_page() unless the grant was a local one. These need to take the
>> same path as foreign entries. Just the assertion there is not valid for
>> local grants, and hence it triggering needs to be avoided.
>>
> 
> I think I'd say:
> 
> ---
> The 'fast' path of p2m_get_page_from_gfn handles three cases: normal ram,
> foreign p2m entries, and grant map entries.  For normal ram and grant table
> entries, get_page() is called, but for foreign entries,
> page_get_owner_and_reference() is called, since the current domain is
> expected not to be the owner.
> 
> Unfortunately, grant maps are *also* generally expected to be owned by
> foreign domains; so this function will fail for any p2m entry containing a
> grant map that doesn't happen to be local.
> 
> Have grant maps take the same path as foreign entries.  Since grants may
> actually be either foreign or local, adjust the assertion to allow for this.
> ---

Sure, thanks, I can use this, but then I'd perhaps ought to add your
S-o-b instead of ...

> One more comment...
> 
> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Using | instead of || helps the compiler fold the two p2m_is_*().
>> ---
>> v2: The shared case was fine; limit to grant adjustment.
>>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -357,11 +357,11 @@ struct page_info *p2m_get_page_from_gfn(
>>               && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
>>          {
>>              page = mfn_to_page(mfn);
>> -            if ( unlikely(p2m_is_foreign(*t)) )
>> +            if ( unlikely(p2m_is_foreign(*t) | p2m_is_grant(*t)) )
>>
> 
> I'm not a fan of this.  If you replace it with || you can have my R-b

... R-b, requiring yet someone else's ack?

> immediately; otherwise we'll have to wait until we can discuss our general
> policy on this sort of thing at the x86 maintainer's call.

I prefer to wait. Considering that even leaving aside the use of
p2m_is_...() "if ( a || b )" is equivalent to "if ( a | b )" (with a and
b of suitable types, of course), and typically requiring less branches
(on x86 at least; architectures with predicated insns of course are
different), personally I'd see us make more use of this in general.
(Hence also the post-commit-message remark.) But yes, Misra in principle
doesn't like such (we've already deviated the underlying pattern,
though).

Of course the compiler is generally in a position to do such a
transformation itself. Just that in at least this specific case I did
observe it not to. I didn't check simpler cases any time halfway
recently.

Jan