Re: [PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2024 08:36, Jan Beulich wrote:
> On 23.04.2024 21:29, Andrew Cooper wrote:
>> On 23/04/2024 3:31 pm, Jan Beulich wrote:
>>> The latest as of e2b2ff677958 ("x86/P2M: split out init/teardown
>>> functions") the function is obviously unreachable for PV guests.
>>
>> This doesn't parse.  Do you mean "Since e2b2ff677958 ..." ?
> 
> Well. I'm sure you at least get the point of "the lastest as of", even
> if that may not be proper English. I specifically didn't use "since"
> because the fact mentioned may have been true before (more or less
> obviously). I'd therefore appreciate a wording suggestion which gets
> this across.
> 
>>>  Hence
>>> the paging_mode_enabled(d) check is pointless.
>>>
>>> Further host mode of a vCPU is always set, by virtue of
>>> paging_vcpu_init() being part of vCPU creation. Hence the
>>> paging_get_hostmode() check is pointless.
>>>
>>> With that the v local variable is unnecessary too. Drop the "if()"
>>> conditional and its corresponding "else".
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> I have to confess that this if() has been puzzling me before.
>>
>> Puzzling yes, but it can't blindly be dropped.
> 
> And I'm not doing so "blindly". Every part of what is being dropped is
> being explained.
> 
>> This is the "did the toolstack initiate this update" check.  i.e. I
>> think it's "bypass the normal side effects of making this update".
> 
> Why would we want to bypass side effects?
> 
>> I suspect it exists because of improper abstraction between the guest
>> physmap and the shadow pagetables as-were - which were/are tighly
>> coupled to vCPUs even for aspects where they shouldn't have been.
>>
>> For better or worse, the toolstack can add_to_physmap() before it
>> creates vCPUs, and it will take this path you're trying to delete. 
>> There may be other cases too; I could see foreign mapping ending up
>> ticking this too.
>>
>> Whether we ought to permit a toolstack to do this is a different
>> question, but seeing as we explicitly intend (eventually for AMX) have a
>> set_policy call between domain_create() and vcpu_create(), I don't think
>> we can reasably restrict other hypercalls too in this period.
> 
> None of which explains what's wrong with the provided justification.
> The P2M isn't per-vCPU. Presence of vCPU-s therefore shouldn't matter
> for alterations of the P2M.

I've gone and checked further: The "side effects" are what the
write_p2m_entry_{pre,post}() hooks would do. Prior to the VM being
started that is a little bit of extra code which all ends up doing
nothing: There's nothing to flush, and there are no shadows to drop.
There's in particular no use of a vCPU anywhere, afaics. Plus, just
to mention it explicitly, the full path was forced anyway for nested
P2Ms, so there's no behavioral change there at all.

In fact I question the correctness of the plain safe_write_pte(),
without p2m_entry_modify(), if that path would have been taken (when
the domain has no vCPU-s yet).

Jan