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

From: Andrew Cooper <amc96@srcf.net>
To: Jan Beulich <jbeulich@suse.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	"Alexandru Isaila" <aisaila@bitdefender.com>,
	"Petre Pircalabu" <ppircalabu@bitdefender.com>
Subject: Re: [PATCH 3/4] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order
Date: Wed, 1 Dec 2021 12:44:09 +0000	[thread overview]
Message-ID: <420124f7-634d-8d9d-9cbe-34ed6a66d80f@srcf.net> (raw)
In-Reply-To: <09aaf19a-b03e-7f41-208e-bfc6bb968049@suse.com>

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.

~Andrew