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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 12, 2025 at 04:20:30PM +0200, Jan Beulich wrote:
> 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.

There's a wbinvd at the beginning of trampoline_start, which is
possibly to what this comment is referring to?

I will just drop the mention of "new" or "old" and capitalize the L in
Linux.

> > +     */
> > +    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.

Saw that series when doing this, I was going to ask about them but you
where away and then I forgot about it.

Let me take a look now.

Thanks, Roger.