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
