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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan