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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.01.2022 17:25, Tamas K Lengyel wrote:
> 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.

Let me start with simpler questions then:

What's the purpose of calling get_gfn_type_access()?

Independent of the answer to the previous question, why isn't it
get_gfn_query() that is called?

What's the purpose of the "a" local variable? (While "t" also is
otherwise unused, it can't be eliminated as even get_gfn_query()
requires its address to be taken.)

Why is p2m_set_entry() called only when the original entry didn't
resolve to INVALID_MFN?

>> 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.

Hmm, I continue to be puzzled. Let's take the XSA-304 workaround as an
example. Suppose an introspection agent has removed X from a 4k page
in an altp2m of a guest. Suppose one of the vCPU-s of this guest runs
on the host p2m. If this vCPU hits the (presumably) 2M or 1G mapping
covering said 4k page for an insn fetch, the page will be shattered
and the removed X in one (or more) of the altp2m-s will, afaict, be
lost. This looks like a bug to me.

Jan