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

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5895 bytes --]

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@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!

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv