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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 23, 2022 at 06:24:48PM +0200, Roger Pau Monné wrote:
> On Mon, May 23, 2022 at 05:13:43PM +0200, Jan Beulich wrote:
> > On 23.05.2022 16:37, Roger Pau Monné wrote:
> > > On Wed, May 18, 2022 at 10:49:22AM +0200, Jan Beulich wrote:
> > >> On 16.05.2022 16:31, Roger Pau Monne wrote:
> > >>> --- 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.
> > > 
> > > Hm, yes.  I guess that's cheaper than adding an extra:
> > > 
> > > if ( cpumask_empty() )
> > >     return;
> > > 
> > > check at the start of the function.
> > > 
> > >>>      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.
> > > 
> > > If we really want to exclude the use of FLUSH_VCPU_STATE for the local
> > > CPU, we might wish to add this as a separate ASSERT, so that such
> > > checking doesn't depend on !local_irq_is_enabled():
> > > 
> > > ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu));
> > > ASSERT(!cpumask_subset(mask, cpumask_of(cpu)) || !(flags & 
> > > FLUSH_VCPU_STATE));

Actually, it would seem even more accurate to use:

ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE));

Thanks, Roger.