[PATCH 2/6] RISC-V: Enable cbo.zero in usermode

[Andrew Jones]

On Thu, Aug 10, 2023 at 10:34:33AM +0100, Conor Dooley wrote:
> On Thu, Aug 10, 2023 at 09:31:54AM +0200, Andrew Jones wrote:
> > On Wed, Aug 09, 2023 at 07:12:58PM +0100, Conor Dooley wrote:
> > > On Wed, Aug 09, 2023 at 06:58:15PM +0200, Andrew Jones wrote:
> > > > On Wed, Aug 09, 2023 at 09:00:35AM -0700, Evan Green wrote:
> > > > > On Wed, Aug 9, 2023 at 4:55 AM Andrew Jones <ajones at ventanamicro.com> wrote:
> > > > ...
> > > > > > +static __always_inline bool riscv_this_cpu_has_extension_likely(const unsigned long ext)
> > > > > > +{
> > > > > > +       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
> > > > > > +               return true;
> > > > > > +
> > > > > > +       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
> > > > > > +}
> > > > > > +
> > > > > > +static __always_inline bool riscv_this_cpu_has_extension_unlikely(const unsigned long ext)
> > > > > > +{
> > > > > > +       if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
> > > > > > +               return true;
> > > > > > +
> > > > > > +       return __riscv_isa_extension_available(hart_isa[smp_processor_id()].isa, ext);
> > > > > > +}
> > > > > 
> > > > > Another way to do this would be to add a parameter to
> > > > > riscv_has_extension_*() (as there are very few users), then these new
> > > > > functions can turn around and call those with the new parameter set to
> > > > > hart_isa[smp_processor_id()].isa. It's a tossup, so up to you. The
> > > > > only advantage to it I can argue is it keeps the code flows more
> > > > > unified.
> > > > >
> > > > 
> > > > I like unification, but I think I'd prefer we create wrappers and
> > > > try to avoid callers needing to construct hart_isa[].isa parameters
> > > > themselves. I'm also not a big fan of the NULL parameter needed when
> > > > riscv_isa_extension_available() is invoked for the riscv_isa bitmap.
> > > > So we need:
> > > > 
> > > >   1. check if an extension is in riscv_isa
> > > >   2. check if an extension is in a bitmap provided by the caller
> > > >   3. check if an extension is in this cpu's isa bitmap
> > > >   4. check if an extension is in the isa bitmap of a cpu provided by the
> > > >      caller
> > > > 
> > > > The only one we can optimize with alternatives is (1), so it definitely
> > > > gets wrappers (riscv_has_extension_likely/unlikely()). (3) and (4) can
> > > > also get wrappers which first try the optimized (1), like I have above.
> > > > Actually (3)'s wrapper could be based on (4)'s, or only provide wrappers
> > > > for (4)
> > > > 
> > > >  static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)
> > > >  {
> > > >      if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_likely(ext))
> > > >          return true;
> > > > 
> > > >      return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
> > > >  }
> > > > 
> > > >  static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsigned long ext)
> > > >  {
> > > >      if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE) && riscv_has_extension_unlikely(ext))
> > > 
> > > Why are you gating on CONFIG_RISCV_ALTERNATIVE here?
> > 
> > This ensures we remove the riscv_has_extension_[un]likely() call
> > when that call would end up using its
> > __riscv_isa_extension_available(NULL, ext) fallback. If that fallback
> > where to return false, then we'd still need to make the
> > __riscv_isa_extension_available(hart_isa[cpu].isa, ext) call, doubling
> > the cost. Whereas, when we gate on CONFIG_RISCV_ALTERNATIVE, we know that
> > riscv_has_extension_[un]likely() will use an alternative to check the
> > global set of extensions. When the extension is there, the compiler
> > ensures that everything vanishes. When it's not, we'll fallback to a
> > single search of the cpu's isa bitmap.
> 
> Right, that is what I suspected that you were trying to accomplish here.
> I was not just not entirely sure whether it was or you'd just missed the
> fallback path. In my original mail I was just going to say "Please add a
> comment here as to why you want to avoid the fallback", but figured I
> should figure out your intent first!

Thanks, I'll add a comment for v2.

> 
> Just to note, alternatives are available on all !XIP kernels now, so
> it's only in the case that the fallback path will be activated.
> 
> > > >          return true;
> > > > 
> > > >      return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
> > > >  }
> > > > 
> > > > and then use smp_processor_id() directly in the callers that need
> > > > to check this_cpu's extensions.
> > > > 
> > > > For case (2), I'd advocate we rename __riscv_isa_extension_available() to
> > > > riscv_has_extension() and drop the riscv_isa_extension_available() macro
> > > > in order to avoid having some calls with RISCV_ISA_EXT_* spelled out and
> > > > others that rely on the pasting.
> > > 
> > > > And, ideally, we'd convert most
> > > > riscv_has_extension(NULL, ext) calls to riscv_has_extension_[un]likely().
> > > 
> > > > I'm also not a big fan of the NULL parameter needed when
> > > > riscv_isa_extension_available() is invoked for the riscv_isa bitmap
> > > 
> > > Rather than actually act on my concerns about
> > > __riscv_isa_extension_available(), I've been busy devoting my spare
> > > time to playing MMOs with the excuse of not wanting to fiddle further
> > > with cpufeature.c et al until Palmer merged the new DT property stuff,
> > > but splitting out your case 1 above seems like it would really help
> > > there. The NULL argument case is the one I think has the potential to
> > > be a footgun in the face of config options.
> > > Split out we can document that purpose of each function & hopefully
> > > have one set of functions that deals with "this extension was detected
> > > to be present in the hardware" and one that does "this extension was
> > > detected & supported by this particular kernel".
> > 
> > Sounds good to me!
> 
> I figure said change should be independent of what's going on in this
> series?

Yeah, I considered doing it as part of this series, but then decided to
only start down the path with two new wrappers, which, for v2, I'll
change to the cpu parameter taking variant. The rest of the rework
would probably be best to do as a separate series, which I can start
soon.

Thanks,
drew