Re: [PATCH] x86/flushtlb: remove flush_area check on system state

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 16/05/2022 15:31, Roger Pau Monne wrote:
> Booting with Shadow Stacks leads to the following assert on a debug
> hypervisor:
>
> (XEN) [   11.625166] Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
> (XEN) [   11.629410] ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
> (XEN) [   11.633679] CPU:    0
> (XEN) [   11.637834] RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
> [...]
> (XEN) [   11.806158] Xen call trace:
> (XEN) [   11.811255]    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
> (XEN) [   11.816459]    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
> (XEN) [   11.821689]    [<ffff82d0404474f9>] F arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
> (XEN) [   11.827053]    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
> (XEN) [   11.832416]    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
> (XEN) [   11.837809]    [<ffff82d040203344>] F __high_start+0x94/0xa0
>
>
> This is due to SYS_STATE_smp_boot being set before calling
> alternative_branches(), and the flush in modify_xen_mappings() then
> using flush_area_all() with interrupts disabled.  Note that
> alternative_branches() is called before APs are started, so the flush
> must be a local one (and indeed the cpumask passed to
> flush_area_mask() just contains one CPU).
>
> Take the opportunity to simplify a bit the logic and make flush_area()
> an alias for flush_area_mask(&cpu_online_map...), taking into account
> that cpu_online_map just contains the BSP before APs are started.
> This requires widening the assert in flush_area_mask() to allow
> being called with interrupts disabled as long as it's strictly a local
> only flush.
>
> The overall result is that a conditional can be removed from
> flush_area().
>
> Fixes: (78e072bc37 'x86/mm: avoid inadvertently degrading a TLB flush to local only')
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Tentatively Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

This seems like the least bad option of a lot of bad options.  I'd say
it's more than just removing a conditional from flush_area(); it's
removing a runtime special case for init-time code.

~Andrew