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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2022 16:31, Roger Pau Monne wrote:
> --- a/xen/arch/x86/include/asm/flushtlb.h
> +++ b/xen/arch/x86/include/asm/flushtlb.h
> @@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, 
> unsigned int flags);
>  #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags)
>  
>  /* Flush all CPUs' TLBs/caches */
> -#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, flags)
> +#define flush_area(va, flags) \
> +    flush_area_mask(&cpu_online_map, (const void *)(va), flags)

I have to admit that I would prefer if we kept the "_all" name suffix,
to continue to clearly express the scope of the flush. I'm also not
really happy to see the cast being added globally now.

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -262,7 +262,8 @@ 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_equal(mask, cpumask_of(cpu)));

Further down we use cpumask_subset(mask, cpumask_of(cpu)),
apparently to also cover the case where mask is empty. I think
you want to do so here as well.

>      if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) &&
>           cpumask_test_cpu(cpu, mask) )

I suppose we want a further precaution here: Despite the
!cpumask_subset(mask, cpumask_of(cpu)) below I think we want to
extend what c64bf2d2a625 ("x86: make CPU state flush requests
explicit") and later changes (isolating uses of FLUSH_VCPU_STATE
from other FLUSH_*) did and exclude the use of FLUSH_VCPU_STATE
for the local CPU altogether. That's because if such somehow made
it into the conditional below here, it would still involve an IPI.
E.g.

    ASSERT(local_irq_is_enabled() ||
           (cpumask_subset(mask, cpumask_of(cpu)) &&
            !(flags & FLUSH_VCPU_STATE)));

Jan