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
