Re: [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Oct 16, 2020 at 08:44:10AM +0200, Jan Beulich wrote:
> In this case up to now we've been freeing the page (through
> guest_remove_page(), with the actual free typically happening at the
> put_page() later in the function), but then failing the call on the
> subsequent GFN consistency check. However, in my opinion such a request
> should complete as an "expensive" no-op (leaving aside the potential
> unsharing of the page).
> 
> This points out that f33d653f46f5 ("x86: replace bad ASSERT() in
> xenmem_add_to_physmap_one()" would really have needed an XSA, despite
> its description claiming otherwise, as in release builds we then put in
> place a P2M entry referencing the about to be freed page.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I've been considering to make such operations "cheap" NOPs rather than
> "expensive" ones, by comparing idx and gpfn early in the function in
> the XENMAPSPACE_gmfn case block, but I've come to the conclusion that
> having the operation perform otherwise normally is better - this way,
> errors that would result if idx != gpfn will still result. While I'm
> open to reasons towards the other alternative, having the added check be
> MFN-based makes crystal clear that we're dealing with the same
> underlying physical resource, i.e. also covers the hypothetical(?) case
> of two GFNs referring to the same MFN.
> 
> I'm unconvinced that it is correct for prev_mfn's p2mt to not be
> inspected at all - I don't think things will go right if p2m_shared()
> was true for it. But I'm afraid I'm not up to correcting mem-sharing
> related logic.
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4555,7 +4555,7 @@ int xenmem_add_to_physmap_one(
>          if ( is_special_page(mfn_to_page(prev_mfn)) )
>              /* Special pages are simply unhooked from this phys slot. */
>              rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K);
> -        else
> +        else if ( !mfn_eq(mfn, prev_mfn) )
>              /* Normal domain memory is freed, to avoid leaking memory. */
>              rc = guest_remove_page(d, gfn_x(gpfn));

What about the access differing between the old and the new entries,
while pointing to the same mfn, would Xen install the new entry
successfully?

Seems easier to me to use guest_physmap_remove_page in that case to
remove the entry from the p2m without freeing the page.

Thanks, Roger.