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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.05.2022 12:50, 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 intorduce
> flush_area_all() as an alias for flush_area_mask(&cpu_online_map...),

This is now stale - you don't introduce flush_area_all() here.
Sadly nothing is said to justify the addition of a cast there,
which - as said before - I think is a little risky (as many
casts are), and hence would imo better be avoided.

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -262,7 +262,10 @@ void flush_area_mask(const cpumask_t *mask, const void 
> *va, unsigned int flags)
>  {
>      unsigned int cpu = smp_processor_id();
>  
> -    ASSERT(local_irq_is_enabled());
> +    /* Local flushes can be performed with interrupts disabled. */
> +    ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)));
> +    /* Exclude use of FLUSH_VCPU_STATE for the local CPU. */
> +    ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE));

What about FLUSH_FORCE_IPI? This won't work either with IRQs off,
I'm afraid. Or wait - that flag's name doesn't really look to
force the use of an IPI, it's still constrained to remote
requests. I think this wants mentioning in one of the comments,
not the least to also have grep match there then (right now grep
output gives the impression as if the flag wasn't consumed
anywhere).

Jan