[PATCH] x86/mm: slightly relax TLB-flush-local check again

[PATCH] x86/mm: slightly relax TLB-flush-local check again

[Jan Beulich]

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.

--- 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))
 

Re: [PATCH] x86/mm: slightly relax TLB-flush-local check again

[Andrew Cooper]

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

Re: [PATCH] x86/mm: slightly relax TLB-flush-local check again

[Jan Beulich]

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