Message ID | ad64e3c6-9f89-c0ea-05a9-cff995ac200f@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/mm: slightly relax TLB-flush-local check again | expand |
On 29/04/2022 14:20, Jan Beulich wrote: > system_state changes to SYS_STATE_smp_boot before alternative_branches() > is invoked, yet that function, with CET-SS enabled, needs to invoke > modify_xen_mappings(). Convert to check for the number of online CPUs, > just like was done also in 88a037e2cfe1 / fa6dc0879ffd ("page_alloc: > assert IRQs are enabled in heap alloc/free", both instance of which > needed reverting for other reasons). > > Fixes: 78e072bc3750 ("x86/mm: avoid inadvertently degrading a TLB flush to local only") > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Only build-tested, as I don't have suitable hardware at hand. I'll give it a test in just a moment, and while semantically I think it's probably right, I don't think we want to express the logic like this. num_online_cpus() is cpumask_weight(&cpu_online_map) behind the scenes which is obnoxiously expensive for what we want. For cases where we care just about UP vs SMP-ness, can't we just have an bool which is re-evaluated each time we take a CPU online/offline? That should be far lower overhead. ~Andrew
On 29.04.2022 15:32, Andrew Cooper wrote: > On 29/04/2022 14:20, Jan Beulich wrote: >> system_state changes to SYS_STATE_smp_boot before alternative_branches() >> is invoked, yet that function, with CET-SS enabled, needs to invoke >> modify_xen_mappings(). Convert to check for the number of online CPUs, >> just like was done also in 88a037e2cfe1 / fa6dc0879ffd ("page_alloc: >> assert IRQs are enabled in heap alloc/free", both instance of which >> needed reverting for other reasons). >> >> Fixes: 78e072bc3750 ("x86/mm: avoid inadvertently degrading a TLB flush to local only") >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Only build-tested, as I don't have suitable hardware at hand. > > I'll give it a test in just a moment, and while semantically I think > it's probably right, I don't think we want to express the logic like this. > > num_online_cpus() is cpumask_weight(&cpu_online_map) behind the scenes > which is obnoxiously expensive for what we want. > > For cases where we care just about UP vs SMP-ness, can't we just have an > bool which is re-evaluated each time we take a CPU online/offline? That > should be far lower overhead. Perhaps, but then I'd immediately ask: Why boolean? We could then as well have a variable holding the count, such that num_online_cpus() wouldn't need to invoke cpumask_weight() anymore at all. In any event I view this as an orthogonal change. It's not entirely without risk, as all updates to cpu_online_map would now also need to update the variable. There shouldn't be too many right now; my main concern would be with future additions. Jan
--- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5074,7 +5074,7 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned l * map_pages_to_xen() can be called early in boot before any other * CPUs are online. Use flush_area_local() in this case. */ -#define flush_area(v,f) (system_state < SYS_STATE_smp_boot ? \ +#define flush_area(v,f) (num_online_cpus() <= 1 ? \ flush_area_local((const void *)v, f) : \ flush_area_all((const void *)v, f))
system_state changes to SYS_STATE_smp_boot before alternative_branches() is invoked, yet that function, with CET-SS enabled, needs to invoke modify_xen_mappings(). Convert to check for the number of online CPUs, just like was done also in 88a037e2cfe1 / fa6dc0879ffd ("page_alloc: assert IRQs are enabled in heap alloc/free", both instance of which needed reverting for other reasons). Fixes: 78e072bc3750 ("x86/mm: avoid inadvertently degrading a TLB flush to local only") Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Only build-tested, as I don't have suitable hardware at hand.