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

[Jan Beulich <jbeulich@xxxxxxxx>]

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/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.
> 
> But there where no direct callers of flush_area_all(), so the name was
> just relevant for it's use in flush_area().  With that now gone I
> don't see a need for a flush_area_all(), as flush_area_mask() is more
> appropriate.

And flush_area_all() is shorthand for flush_area_mask(&cpu_online_map, ...).
That's more clearly distinguished from flush_area_local() than simply
flush_area(); the latter was okay-ish with its mm.c-only exposure, but imo
isn't anymore when put in a header.

>>> --- 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));
> 
> 
>> That's because if such somehow made
>> it into the conditional below here, it would still involve an IPI.
> 
> Sorry, I'm confused by this: if the mask is empty there should be no
> IPI involved at all?  And we shouldn't even get into the second
> conditional on the function.

Should perhaps have made more explicit that "somehow" means a hypothetical
way, perhaps even as a result of some further breakage somewhere.

Jan