Re: [PATCH 08/19] riscv: Introduce vendor variants of extension helpers

From: Charlie Jenkins <charlie@rivosinc.com>
To: Conor Dooley <conor.dooley@microchip.com>
Cc: "Conor Dooley" <conor@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Guo Ren" <guoren@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Jernej Skrabec" <jernej.skrabec@gmail.com>,
	"Samuel Holland" <samuel@sholland.org>,
	"Evan Green" <evan@rivosinc.com>,
	"Clément Léger" <cleger@rivosinc.com>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Shuah Khan" <shuah@kernel.org>,
	linux-riscv@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"Palmer Dabbelt" <palmer@rivosinc.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@lists.linux.dev, linux-doc@vger.kernel.org,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH 08/19] riscv: Introduce vendor variants of extension helpers
Date: Fri, 12 Apr 2024 10:43:02 -0700	[thread overview]
Message-ID: <ZhlypvTdsFPZBr08@ghost> (raw)
In-Reply-To: <20240412-dwarf-shower-5a7300fcd283@wendy>

On Fri, Apr 12, 2024 at 12:49:57PM +0100, Conor Dooley wrote:
> On Thu, Apr 11, 2024 at 09:11:14PM -0700, Charlie Jenkins wrote:
> > Create vendor variants of the existing extension helpers. If the
> > existing functions were instead modified to support vendor extensions, a
> > branch based on the ext value being greater than
> > RISCV_ISA_VENDOR_EXT_BASE would have to be introduced. This additional
> > branch would have an unnecessary performance impact.
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> 
> I've not looked at the "main" patch in the series that adds all of the
> probing and structures for representing this info yet beyond a cursory
> glance, but it feels like we're duplicating a bunch of infrastructure
> here before it is necessary. The IDs are all internal to Linux, so I'd
> rather we kept everything in the same structure until we have more than
> a handful of vendor extensions. With this patch (and the theadpmu stuff)
> we will have three vendor extensions which feels like a drop in the
> bucket compared to the standard ones.

It is not duplicating infrastructure. If we merge this into the existing
infrastructure, we would be littering if (ext > RISCV_ISA_VENDOR_EXT_BASE)
in __riscv_isa_extension_available. This is particularily important
exactly because we have so few vendor extensions currently so this check
would be irrelevant in the vast majority of cases.

It is also unecessary to push off the refactoring until we have some
"sufficient" amount of vendor extensions to deem changing the
infrastructure when I already have the patch available here. This does
not introduce any extra overhead to existing functions and will be able
to support vendors into the future.

- Charlie

> 
> 
> > ---
> >  arch/riscv/include/asm/cpufeature.h | 54 +++++++++++++++++++++++++++++++++++++
> >  arch/riscv/kernel/cpufeature.c      | 34 ++++++++++++++++++++---
> >  2 files changed, 84 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> > index db2ab037843a..8f19e3681b4f 100644
> > --- a/arch/riscv/include/asm/cpufeature.h
> > +++ b/arch/riscv/include/asm/cpufeature.h
> > @@ -89,6 +89,10 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned i
> >  #define riscv_isa_extension_available(isa_bitmap, ext)	\
> >  	__riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext)
> >  
> > +bool __riscv_isa_vendor_extension_available(const unsigned long *vendor_isa_bitmap, unsigned int bit);
> > +#define riscv_isa_vendor_extension_available(isa_bitmap, ext)	\
> > +	__riscv_isa_vendor_extension_available(isa_bitmap, RISCV_ISA_VENDOR_EXT_##ext)
> > +
> >  static __always_inline bool
> >  __riscv_has_extension_likely_alternatives(const unsigned long ext)
> >  {
> > @@ -117,6 +121,8 @@ __riscv_has_extension_unlikely_alternatives(const unsigned long ext)
> >  	return true;
> >  }
> >  
> > +/* Standard extension helpers */
> > +
> >  static __always_inline bool
> >  riscv_has_extension_likely(const unsigned long ext)
> >  {
> > @@ -163,4 +169,52 @@ static __always_inline bool riscv_cpu_has_extension_unlikely(int cpu, const unsi
> >  		return __riscv_isa_extension_available(hart_isa[cpu].isa, ext);
> >  }
> >  
> > +/* Vendor extension helpers */
> > +
> > +static __always_inline bool
> > +riscv_has_vendor_extension_likely(const unsigned long ext)
> > +{
> > +	compiletime_assert(ext < RISCV_ISA_VENDOR_EXT_MAX,
> > +			   "ext must be < RISCV_ISA_VENDOR_EXT_MAX");
> > +
> > +	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))
> > +		return __riscv_has_extension_likely_alternatives(ext);
> > +	else
> > +		return __riscv_isa_vendor_extension_available(NULL, ext);
> > +}
> > +
> > +static __always_inline bool
> > +riscv_has_vendor_extension_unlikely(const unsigned long ext)
> > +{
> > +	compiletime_assert(ext < RISCV_ISA_VENDOR_EXT_MAX,
> > +			   "ext must be < RISCV_ISA_VENDOR_EXT_MAX");
> > +
> > +	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))
> > +		return __riscv_has_extension_unlikely_alternatives(ext);
> > +	else
> > +		return __riscv_isa_vendor_extension_available(NULL, ext);
> > +}
> > +
> > +static __always_inline bool riscv_cpu_has_vendor_extension_likely(int cpu, const unsigned long ext)
> > +{
> > +	compiletime_assert(ext < RISCV_ISA_VENDOR_EXT_MAX,
> > +			   "ext must be < RISCV_ISA_VENDOR_EXT_MAX");
> > +
> > +	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))
> > +		return __riscv_has_extension_likely_alternatives(ext);
> > +	else
> > +		return __riscv_isa_vendor_extension_available(hart_isa_vendor[cpu].isa, ext);
> > +}
> > +
> > +static __always_inline bool riscv_cpu_has_vendor_extension_unlikely(int cpu, const unsigned long ext)
> > +{
> > +	compiletime_assert(ext < RISCV_ISA_VENDOR_EXT_MAX,
> > +			   "ext must be < RISCV_ISA_VENDOR_EXT_MAX");
> > +
> > +	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))
> > +		return __riscv_has_extension_unlikely_alternatives(ext);
> > +	else
> > +		return __riscv_isa_vendor_extension_available(hart_isa_vendor[cpu].isa, ext);
> > +}
> 
> Same stuff about constant folding applies to these, I think these should
> just mirror the existing functions (if needed at all).
> 
> Cheers,
> Conor.

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