Re: [PATCH 3/4] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Jan Beulich <jbeulich@suse.com>]

On 01.12.2021 13:44, Andrew Cooper wrote:
> On 01/12/2021 10:54, Jan Beulich wrote:
>> @@ -2237,11 +2243,11 @@ bool p2m_altp2m_get_or_propagate(struct
>>       * to the start of the superpage.  NB that we repupose `amfn`
>>       * here.
>>       */
>> -    mask = ~((1UL << page_order) - 1);
>> +    mask = ~((1UL << cur_order) - 1);
>>      amfn = _mfn(mfn_x(*mfn) & mask);
>>      gfn = _gfn(gfn_l & mask);
>>  
>> -    rc = p2m_set_entry(ap2m, gfn, amfn, page_order, *p2mt, *p2ma);
>> +    rc = p2m_set_entry(ap2m, gfn, amfn, cur_order, *p2mt, *p2ma);
>>      p2m_unlock(ap2m);
> 
> While I agree with the problem you've identified, this function has some
> very broken return semantics.
> 
> Logically, it is taking some hostp2m properties for gfn, and replacing
> them with ap2m properties for the same gfn.
> 
> 
> It cannot be correct to only update the caller state on the error
> paths.  At a minimum, the
> 
>     if ( paged )
>         p2m_mem_paging_populate(currd, _gfn(gfn));
> 
> path in the success case is wrong when we've adjusted gfn down.

I wonder which of the exit paths you consider to be "error" ones. The
first one returning "false" pretty clearly isn't, for example. And the
path returning "true" is after a p2m_set_entry(), which means (if that
one succeeded) that caller values are now in sync with the P2M and
hence doen't need updating afaics.

And anyway - how does what you say relate to the patch at hand? I don't
think you mean to request that I fix further problems elsewhere, right
here?

Jan