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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.05.2022 10:13, Roger Pau Monne wrote:
> Booting with Shadow Stacks leads to the following assert on a debug
> hypervisor:
> 
> Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
> ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
> CPU:    0
> RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
> [...]
> Xen call trace:
>    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
>    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
>    [<ffff82d0404474f9>] F 
> arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
>    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
>    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
>    [<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 of flush_area_all() in mm.c, 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().
> 
> While there also introduce an ASSERT to check that a vCPU state flush
> is not issued for the local CPU only.
> 
> Fixes: (78e072bc37 'x86/mm: avoid inadvertently degrading a TLB flush to 
> local only')
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with ...

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5070,13 +5070,8 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
>  #define l1f_to_lNf(f) (((f) & _PAGE_PRESENT) ? ((f) |  _PAGE_PSE) : (f))
>  #define lNf_to_l1f(f) (((f) & _PAGE_PRESENT) ? ((f) & ~_PAGE_PSE) : (f))
>  
> -/*
> - * 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 ?    \
> -                         flush_area_local((const void *)v, f) : \
> -                         flush_area_all((const void *)v, f))
> +/* flush_area_all() can be used prior to any other CPU being online.  */
> +#define flush_area(v, f) flush_area_all((const void *)v, f)

... v properly parenthesized here as the code is being touched anyway:
One less Misra-C violation. This surely can be done while committing.

Jan