Re: [PATCH v8 01/11] riscv: Add B to hwcap and hwprobe

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]

On Tue, Sep 29, 2026 at 06:30:49PM +0200, Heinrich Schuchardt wrote:
> On 9/29/26 17:12, Radim Krcmar wrote:
> > 2026-09-24T06:38:17-04:00, Guodong Xu <guodong.xu@oss.qualcomm.com>:
> > > On Mon, 21 Sep 2026 16:24:53 +0200, Heinrich Schuchardt wrote:
> > > > On 9/20/26 09:18, Guodong Xu wrote:
> > > > > [ ... ]
> > > > >    	__RISCV_ISA_EXT_DATA(q, RISCV_ISA_EXT_Q),
> > > > >    	__RISCV_ISA_EXT_SUPERSET(c, RISCV_ISA_EXT_C, riscv_c_exts),
> > > > > +	__RISCV_ISA_EXT_SUPERSET(b, RISCV_ISA_EXT_B, riscv_b_exts),
> > > > 
> > > > Hello Guodong,
> > > > 
> > > > The RISC-V Unpriviledged ISA specification has this description of
> > > > extension B:
> > > > 
> > > > "The B standard extension comprises instructions provided by the Zba,
> > > > Zbb, and Zbs extensions."
> > > > 
> > > > __RISCV_ISA_EXT_SUPERSET would imply that something else but
> > > > riscv_b_exts is in B. But such an extra seems not to exist.
> > > > 
> > > > So shouldn't __RISCV_ISA_EXT_BUNDLE be used here? Some code further
> > > > change may be needed to set extension B if riscv_b_exts is fulfilled.
> > > 
> > > Thanks for the review. Intentional, and the difference between the two
> > > macros is whether the extension gets a bit of its own.
> > > 
> > > __RISCV_ISA_EXT_BUNDLE carries RISCV_ISA_EXT_INVALID as its id: parsing
> > > the name only sets the bits of its parts. That fits zk, zkn names, which
> > > are shorthands with no identity of their own beyond the ISA string.
> > > 
> > > B is different: it is a single-letter standard extension with its own
> > > misa bit (in the same way as A), and AT_HWCAP on RISC-V is the bitmask
> > > of exactly those single letters, so the kernel needs a bit for B itself.
> > > 
> > > A is declared the same way; with the spec defines A in the same words as
> > > B. If I can take that as a precedence.
> > > 
> > > IMHO, "superset" in this table means "also sets these subset bits", not
> > > "contains something extra".
> > 
> > Zba, Zbb, and Zbs are equivalent to B for our purposes.
> > 
> > Are we sure that B will always be listed in the ISA string when Zba,
> > Zbb, and Zbs are present?
> > 
> > We could incorrectly lose RVA23U64 bit otherwise, and I think this was
> > Heinrich's concern as well...
> > 
> > (The "A" extension has the same issue...)
> > 
> > Thanks.
> 
> If Zba, Zbb, and Zbs are present the kernel should set the B flag in
> hwprobe. This is why RISCV_ISA_EXT_SUPERSET() cannot be used to describe the
> B extension. RISCV_ISA_EXT_BUNDLE looks more appropriate but may lack
> functionality.
> 
> RVA23U64 looks like an RISCV_ISA_EXT_BUNDLE() to me, too.
> 
> Unfortunately these macros are not properly documented.
> 
> It would be helpful to first align on the meaning and usage of the macros
> and document them properly.

The intended meaning was "bundle contains no additional features beyond
the components" and "superset contains additional features beyond the
components".

IIRC the reason for differentiation between the two was to simplify
things in the kernel and avoid having code which requires y feature checking
for "bundle extension xyz", because firmware might only set "component
extension y" and therefore get a false negative on support.
Probably ditto for userspace parsing /proc/cpuinfo, since I don't think
hwprobe existed at that point. The things that are using superset now
don't quite match that, because some extensions have been retroactively
changed by RVI to match the bundle definition (due to new extensions being
created for subsets of an existing extension) and superset was used also for
the xlinuxenvcfg stuff.

At this point, I think we could probably just cull the differentiation
entirely, retaining a macro called "bundle" that has the behaviour of
the current "superset". People should just know to check the minimum
required extension (that's common sense surely?!?) and the kernel will
always propagate support down to components. This is at least the 3rd
time recently that I have seen confusion over what each is supposed to
do.


> 
> ---
> 
> The benefit of an additional hwprobe flags for B is limited. When I want to
> check for B I can already use:
> 
> RISCV_HWPROBE_EXT_B =
> RISCV_HWPROBE_EXT_ZBA | RISCV_HWPROBE_EXT_ZBB | RISCV_HWPROBE_EXT_ZBS;
> 
> if ((value & RISCV_HWPROBE_EXT_B) == RISCV_HWPROBE_EXT_B) {
> 	// Hurray, I have the B extension.
> }
> 
> There is more utility in the RVA23U64 flag because it combines values from
> RISCV_HWPROBE_KEY_BASE_BEHAVIOR, RISCV_HWPROBE_KEY_IMA_EXT_0, and
> RISCV_HWPROBE_KEY_IMA_EXT_1.
> 
> Best regards
> 
> Heinrich

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