Re: [PATCH 2/9] x86/pv: fix emulation of wb{,no}invd to flush all pCPU caches

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2025 10:31, Roger Pau Monne wrote:
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -1193,17 +1193,18 @@ static int cf_check cache_op(
>  {
>      ASSERT(op == x86emul_wbinvd || op == x86emul_wbnoinvd);
>  
> -    /* Ignore the instruction if unprivileged. */
> -    if ( !cache_flush_permitted(current->domain) )
> +    /*
> +     * Ignore the instruction if domain doesn't have cache control.
> +     * Non-physdev domain attempted WBINVD; ignore for now since
> +     * newer linux uses this in some start-of-day timing loops.

That's very old comment, and hence I think "newer" isn't quite applicable
anymore. Either omit the word (if Linux still does so), or say "older"
instead? Also since you touch the comment, upper-casing the L in Linux
might be nice.

> +     */
> +    if ( cache_flush_permitted(current->domain) )
>          /*
> -         * Non-physdev domain attempted WBINVD; ignore for now since
> -         * newer linux uses this in some start-of-day timing loops.
> +         * Handle wbnoinvd as wbinvd, at the expense of higher cost.  
> Broadcast
> +         * the flush to all pCPUs, Xen doesn't track where the vCPU has ran
> +         * previously.
>           */
> -        ;
> -    else if ( op == x86emul_wbnoinvd /* && cpu_has_wbnoinvd */ )
> -        wbnoinvd();

So this goes away altogether, which isn't nice. It was "only" 2 years ago that
I posted a series where an additional ...

> -    else
> -        wbinvd();
> +        flush_all(FLUSH_CACHE);

... FLUSH_CACHE_WRITEBACK is introduced [1]. I really, really think that should
go in first, for it to then be used here. The more that it's the 1st patch in
that series.

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2023-05/msg00242.html