Re: [PATCH 1/5] x86: support cache-writeback in flush_area_local() et al

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/04/2023 11:44 am, Jan Beulich wrote:
> --- a/xen/arch/x86/flushtlb.c
> +++ b/xen/arch/x86/flushtlb.c
> @@ -232,7 +232,7 @@ unsigned int flush_area_local(const void
>      if ( flags & FLUSH_HVM_ASID_CORE )
>          hvm_flush_guest_tlbs();
>  
> -    if ( flags & FLUSH_CACHE )
> +    if ( flags & (FLUSH_CACHE | FLUSH_WRITEBACK) )

Given that we already have FLUSH_CACHE, then adding writeback also seems
fine, but we need to get the naming corrected first.

We've got a file called flushtlb.c which flushes more than the TLBs now,
and various APIs in it.

We have a bunch of ARM specific APIs which AFAICT exist purely to prop
up the ARM-specific gnttab_cache_flush().  That needs to go and live
behind an ifdef and stop polluting other architectures with an
incredibly short sighted hypercall interface decision.

The "area" in the low level calls isn't good.  Range might be better,
but I'm not sure.  The "mask" part of the name would be better as "some"
or perhaps "others", to be a better counterpoint to "local".  Some of
the wrappers too really ought to be dropped - there are lots of them,
and too few users to justify.

But on to the main thing which caught my eye...

The FLUSH in FLUSH_CACHE means the flush infrastructure, not "cache
flushing", and FLUSH_WRITEBACK is nonsensical next to this.  All other
things we flush have a qualification that makes them clear in context.
(other than the assist one which I'm going to time out objections to and
revert back to name which made more sense.)

At an absolutely minimum, FLUSH_CACHE first needs renaming to
FLUSH_CACHE_EVICT and then this new one you're adding needs to be
FLUSH_CACHE_WRITEBACK.

Except...

Is there any credible reason to have EVICT as an option by the end of
this cleanup?

CLDEMOTE does exist for a reason (reducing coherency traffic overhead
when you know the consumer is on a different CPU), but it would be
totally bogus to use this in an all or mask form, and you wouldn't want
to use it in local form either, simply from an overhead point of view.

We have evict behaviour simply because `clflush` was the only game in
town for decades, not because evicting the cacheline is what you want
actually want to do.

~Andrew