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

[Conor Dooley <conor@kernel.org>]

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

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?

>          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".

I'll try to take a proper look at this series tomorrow :)

Cheers,
Conor.

[-- 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