Re: [PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway
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
>>> ---
>>> 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
Re: [PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway
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
>> ---
>> 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
Re: [PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway
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 ..." ?
> 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
> ---
> I have to confess that this if() has been puzzling me before.
Puzzling yes, but it can't blindly be dropped.
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".
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.
~Andrew
[PATCH 1/4] x86/P2M: write_p2m_entry() is HVM-only anyway
The latest as of e2b2ff677958 ("x86/P2M: split out init/teardown
functions") the function is obviously unreachable for PV guests. 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
---
I have to confess that this if() has been puzzling me before.
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -110,12 +110,7 @@ static int write_p2m_entry(struct p2m_do
unsigned int level)
{
struct domain *d = p2m->domain;
-const struct vcpu *v = current;
-if ( v->domain != d )
-v = d->vcpu ? d->vcpu[0] : NULL;
-if ( likely(v && paging_mode_enabled(d) && paging_get_hostmode(v)) ||
- p2m_is_nestedp2m(p2m) )
{
unsigned int oflags;
mfn_t omfn;
@@ -156,8 +151,6 @@ static int write_p2m_entry(struct p2m_do
!perms_strictly_increased(oflags, l1e_get_flags(new))) )
p2m_flush_nestedp2m(d);
}
-else
-safe_write_pte(p, new);
return 0;
}
