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
