Re: [PATCH RFC v2 3/3] x86/altp2m: p2m_altp2m_propagate_change() should honor present page order

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Jan 5, 2022 at 3:59 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 04.01.2022 18:48, Tamas K Lengyel wrote:
> >> I may be entirely wrong and hence that part of the change may also be
> >> wrong, but I'm having trouble seeing why the original
> >> "!mfn_eq(m, INVALID_MFN)" wasn't "mfn_eq(m, INVALID_MFN)". Isn't the
> >> goal there to pre-fill entries that were previously invalid, instead of
> >> undoing prior intentional divergence from the host P2M? (I have
> >> intentionally not reflected this aspect in the description yet; I can't
> >> really write a description of this without understanding what's going on
> >> in case the original code was correct.)
> >
> > This function only gets called from p2m-ept when the hostp2m gets an
> > update. In that case this check goes through all altp2m's to see if
> > any of them has an entry for what just got changed in the host, and
> > overwrites the altp2m with that from the host. If there is no entry in
> > the altp2m it doesn't pre-populate. That should only happen if the
> > altp2m actually needs it and runs into a pagefault. So it is correct
> > as-is, albeit being a subtle (and undocumented) behavior of the
> > hostp2m and its effect on the altp2m's. But that's why we never
> > actually make any changes on the hostp2m, we always create an altp2m
> > and apply changes (mem_access/remapping) there.
>
> Thanks for the explanation. Effectively this means that the call to
> get_gfn_type_access() can simply be get_gfn_query(). For the patch
> this means that I shouldn't check its return value and also continue
> to pass the new order rather than the minimum of the two (as was the
> case before), as all we're after is the locking of the GFN. It would
> be nice if you could confirm this before I submit a non-RFC v3.

I'm a little lost here.

>
> What I still don't understand is why the function blindly replaces
> any possible entry in the altp2m, i.e. any possible override
> mapping, not even taking into account the original p2m_access_t.

I think the idea was that any changes to the hostp2m will just get
reflected in the altp2m's as a short-cut. If you have many custom
settings in different altp2ms, simply setting the entry in the hostp2m
will ensure all altp2m's get synced to that same setting instead of
having to do a hypercall for each altp2m. I don't see much use of it
otherwise and if we wanted to switch it to leave the altp2m entries
as-is I wouldn't object.

Tamas