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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2023 21:56, Andrew Cooper wrote:
> 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.

I agree; I chose the name simply to avoid further changes, while still
fitting the naming scheme (i.e. the FLUSH_ prefix). I'm okay to change
to ...

>  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.

... these, but I don't think they're much better (still primarily
because of the FLUSH_ prefixes).

FTAOD - while I'm going to make these adjustments (adding a single
prereq patch to carry out the initial rename), I don't really see me
doing any of there other adjustments you were outlining above (at
least not within this series).

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

Yes: map_pages_to_xen(), hvm_shadow_handle_cd(), memory_type_changed(),
and hvm_set_mem_pinned_cacheattr(), all mean to evict caches aiui.

clean_and_invalidate_dcache_va_range(), as you say, really shouldn't
be required, but I'm unconvinced we can easily drop support for a
gnttab sub-op that's been around for a number of years. (The more with
there (still) not being any support for GNTTAB_CACHE_SOURCE_GREF, I'm
(still) thinking this shouldn't have been a gnttab sub-op in the first
place, but something truly arch-specific [a platform-op, or a memory
one, or ...].)

Jan

> 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