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

[Andrew Jones <ajones@ventanamicro.com>]

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().

Thanks,
drew

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