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

[Evan Green <evan@rivosinc.com>]

On Wed, Aug 9, 2023 at 9:58 AM Andrew Jones <ajones@ventanamicro.com> 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))
>          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().

Sounds ok to me!
-Evan

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