Re: [PATCH] arch/cacheflush: Introduce flush_all_caches()

[Davidlohr Bueso]

On Tue, 16 Aug 2022, Dan Williams wrote:

> On Tue, Aug 16, 2022 at 10:30 AM Davidlohr Bueso <dave@xxxxxxxxxxxx> wrote:
> > On Tue, 16 Aug 2022, Dan Williams wrote:
> >
> > >Peter Zijlstra wrote:
> > >> On Mon, Aug 15, 2022 at 09:07:06AM -0700, Davidlohr Bueso wrote:
> > >> > diff --git a/arch/x86/include/asm/cacheflush.h b/arch/x86/include/asm/cacheflush.h
> > >> > index b192d917a6d0..ce2ec9556093 100644
> > >> > --- a/arch/x86/include/asm/cacheflush.h
> > >> > +++ b/arch/x86/include/asm/cacheflush.h
> > >> > @@ -10,4 +10,7 @@
> > >> >
> > >> >  void clflush_cache_range(void *addr, unsigned int size);
> > >> >
> > >> > +#define flush_all_caches() \
> > >> > +  do { wbinvd_on_all_cpus(); } while(0)
> > >> > +
> > >>
> > >> This is horrific... we've done our utmost best to remove all WBINVD
> > >> usage and here you're adding it back in the most horrible form possible
> > >> ?!?
> > >>
> > >> Please don't do this, do *NOT* use WBINVD.
> > >
> > >Unfortunately there are a few good options here, and the changelog did
> > >not make clear that this is continuing legacy [1], not adding new wbinvd
> > >usage.
> >
> > While I was hoping that it was obvious from the intel.c changes that this
> > was not a new wbinvd, I can certainly improve the changelog with the below.
>
> I also think this cache_flush_region() API wants a prominent comment
> clarifying the limited applicability of this API. I.e. that it is not
> for general purpose usage, not for VMs, and only for select bare metal
> scenarios that instantaneously invalidate wide swaths of memory.
> Otherwise, I can now see how this looks like a potentially scary
> expansion of the usage of wbinvd.

Sure.

Also, in the future we might be able to bypass this hammer in the presence
of persistent cpu caches.

Thanks,
Davidlohr