| Message ID | 296c3ecc-b04d-4734-a451-0d4f9ed312d4@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: assorted adjustments | expand |
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 <jbeulich@suse.com> > --- > 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
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@suse.com> >> --- >> 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
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@suse.com> >>> --- >>> 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
--- 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; }
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 <jbeulich@suse.com> --- I have to confess that this if() has been puzzling me before.