| Message ID | 20231017131456.2053396-1-cleger@rivosinc.com |
|---|---|
| Headers | show |
| Series | riscv: report more ISA extensions through hwprobe | expand |
On 18/10/2023 03:45, Jerry Shih wrote: > On Oct 17, 2023, at 21:14, Clément Léger <cleger@rivosinc.com> wrote: >> @@ -221,6 +261,22 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { >> __RISCV_ISA_EXT_DATA(zkt, RISCV_ISA_EXT_ZKT), >> __RISCV_ISA_EXT_DATA(zksed, RISCV_ISA_EXT_ZKSED), >> __RISCV_ISA_EXT_DATA(zksh, RISCV_ISA_EXT_ZKSH), >> + __RISCV_ISA_EXT_DATA(zvbb, RISCV_ISA_EXT_ZVBB), >> + __RISCV_ISA_EXT_DATA(zvbc, RISCV_ISA_EXT_ZVBC), >> + __RISCV_ISA_EXT_DATA(zvkb, RISCV_ISA_EXT_ZVKB), > > The `Zvkb` is the subset of `Zvbb`[1]. So, the `Zvkb` should be bundled with `Zvbb`. Hi Jerry, Thanks for catching this, I think some other extensions will fall in this category as well then (Zvknha/Zvknhb). I will verify that. Clément > > + __RISCV_ISA_EXT_DATA(zvbb, RISCV_ISA_EXT_ZVBB), > + __RISCV_ISA_EXT_DATA(zvbb, RISCV_ISA_EXT_ZVKB), > + __RISCV_ISA_EXT_DATA(zvkb, RISCV_ISA_EXT_ZVKB), > > or > > + __RISCV_ISA_EXT_BUNDLE(zvbb, riscv_zvbb_bundled_exts), > + __RISCV_ISA_EXT_DATA(zvkb, RISCV_ISA_EXT_ZVKB), > > [1] > https://github.com/riscv/riscv-crypto/blob/main/doc/vector/riscv-crypto-vector-zvkb.adoc > > -Jerry
On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > Factorize ISA extension reporting by using a macro rather than > copy/pasting extension names. This will allow adding new extensions more > easily. > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > index 473159b5f303..e207874e686e 100644 > --- a/arch/riscv/kernel/sys_riscv.c > +++ b/arch/riscv/kernel/sys_riscv.c > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > for_each_cpu(cpu, cpus) { > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > > - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > - pair->value |= RISCV_HWPROBE_EXT_ZBA; > - else > - missing |= RISCV_HWPROBE_EXT_ZBA; > - > - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > - pair->value |= RISCV_HWPROBE_EXT_ZBB; > - else > - missing |= RISCV_HWPROBE_EXT_ZBB; > - > - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > - pair->value |= RISCV_HWPROBE_EXT_ZBS; > - else > - missing |= RISCV_HWPROBE_EXT_ZBS; > +#define CHECK_ISA_EXT(__ext) \ > + do { \ > + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > + else \ > + missing |= RISCV_HWPROBE_EXT_##__ext; \ > + } while (false) > + > + /* > + * Only use CHECK_ISA_EXT() for extensions which can be exposed > + * to userspace, regardless of the kernel's configuration, as no > + * other checks, besides presence in the hart_isa bitmap, are > + * made. This comment alludes to a dangerous trap, but I'm having trouble understanding what it is. Perhaps some rewording to more explicitly state the danger would be appropriate. Other than that: Reviewed-by: Evan Green <evan@rivosinc.com>
On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > Export the following scalar crypto extensions through hwprobe: > > - Zbkb > - Zbkc > - Zbkx > - Zknd > - Zkne > - Zknh > - Zksed > - Zksh > - Zkt > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > Documentation/riscv/hwprobe.rst | 30 +++++++++++++++++++++++++++ > arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++++++ > arch/riscv/kernel/sys_riscv.c | 10 +++++++++ > 3 files changed, 50 insertions(+) > > diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst > index a52996b22f75..968895562d42 100644 > --- a/Documentation/riscv/hwprobe.rst > +++ b/Documentation/riscv/hwprobe.rst > @@ -77,6 +77,36 @@ The following keys are defined: > * :c:macro:`RISCV_HWPROBE_EXT_ZBS`: The Zbs extension is supported, as defined > in version 1.0 of the Bit-Manipulation ISA extensions. > > + * :c:macro:`RISCV_HWPROBE_EXT_ZBC` The Zbc extension is supported, as defined > + in version 1.0 of the Scalar Crypto ISA extensions. At least in my v1.0.1 version of the crypto scalar spec, I don't see Zbc. That seems to be defined in the bit manipulation extensions.
On Wed, Oct 18, 2023 at 5:53 AM Clément Léger <cleger@rivosinc.com> wrote: > > > > On 18/10/2023 03:45, Jerry Shih wrote: > > On Oct 17, 2023, at 21:14, Clément Léger <cleger@rivosinc.com> wrote: > >> @@ -221,6 +261,22 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > >> __RISCV_ISA_EXT_DATA(zkt, RISCV_ISA_EXT_ZKT), > >> __RISCV_ISA_EXT_DATA(zksed, RISCV_ISA_EXT_ZKSED), > >> __RISCV_ISA_EXT_DATA(zksh, RISCV_ISA_EXT_ZKSH), > >> + __RISCV_ISA_EXT_DATA(zvbb, RISCV_ISA_EXT_ZVBB), > >> + __RISCV_ISA_EXT_DATA(zvbc, RISCV_ISA_EXT_ZVBC), > >> + __RISCV_ISA_EXT_DATA(zvkb, RISCV_ISA_EXT_ZVKB), > > > > The `Zvkb` is the subset of `Zvbb`[1]. So, the `Zvkb` should be bundled with `Zvbb`. > > Hi Jerry, > > Thanks for catching this, I think some other extensions will fall in > this category as well then (Zvknha/Zvknhb). I will verify that. The bundling mechanism works well when an extension is a pure lasso around other extensions. We'd have to tweak that code if we wanted to support cases like this, where the extension is a superset of others, but also contains loose change not present anywhere else (and therefore also needs to stand as a separate bit). IMO, decomposing "pure" bundles makes sense since otherwise usermode would have to query multiple distinct bitmaps that meant the same thing (eg check the Zk bit, or maybe check the Zkn/Zkr/Zkt bits, or maybe check the Zbkb/Zbkc... bits, and they're all equivalent). But when an extension is a superset that also contains loose change, there really aren't two equivalent bitmasks, each bit adds something new. There's an argument to be made for still turning on the containing extensions to cover for silly ISA strings (eg ISA strings that advertise the superset but fail to advertise the containing extensions). We can decide if we want to work that hard to cover hypothetical broken ISA strings now, or wait until they show up. Personally I would wait until something broken shows up. But others may feel differently. -Evan
On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > Add parsing for Zihintntl ISA extension[1] that was ratified in commit > 0dc91f5 ("Zihintntl is ratified") of riscv-isa-manual[2]. > > Link: https://drive.google.com/file/d/13_wsN8YmRfH8YWysFyTX-DjTkCnBd9hj/view [1] > Link: https://github.com/riscv/riscv-isa-manual/commit/0dc91f505e6d [2] > Signed-off-by: Clément Léger <cleger@rivosinc.com> Reviewed-by: Evan Green <evan@rivosinc.com>
On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > Export Zvfh[min] ISA extension[1] through hwprobe. > > Link: https://drive.google.com/file/d/1_Yt60HGAf1r1hx7JnsIptw0sqkBd9BQ8/view [1] > Signed-off-by: Clément Léger <cleger@rivosinc.com> Reviewed-by: Evan Green <evan@rivosinc.com>
On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > Export Zfa ISA extension[1] through hwprobe. > > Link: https://drive.google.com/file/d/1VT6QIggpb59-8QRV266dEE4T8FZTxGq4/view [1] > Signed-off-by: Clément Léger <cleger@rivosinc.com> Reviewed-by: Evan Green <evan@rivosinc.com>
On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > > > Factorize ISA extension reporting by using a macro rather than > > copy/pasting extension names. This will allow adding new extensions more > > easily. > > > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > --- > > arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > > index 473159b5f303..e207874e686e 100644 > > --- a/arch/riscv/kernel/sys_riscv.c > > +++ b/arch/riscv/kernel/sys_riscv.c > > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > > for_each_cpu(cpu, cpus) { > > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > > - pair->value |= RISCV_HWPROBE_EXT_ZBA; > > - else > > - missing |= RISCV_HWPROBE_EXT_ZBA; > > - > > - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > > - pair->value |= RISCV_HWPROBE_EXT_ZBB; > > - else > > - missing |= RISCV_HWPROBE_EXT_ZBB; > > - > > - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > > - pair->value |= RISCV_HWPROBE_EXT_ZBS; > > - else > > - missing |= RISCV_HWPROBE_EXT_ZBS; > > +#define CHECK_ISA_EXT(__ext) \ > > + do { \ > > + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > > + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > > + else \ > > + missing |= RISCV_HWPROBE_EXT_##__ext; \ > > + } while (false) > > + > > + /* > > + * Only use CHECK_ISA_EXT() for extensions which can be exposed > > + * to userspace, regardless of the kernel's configuration, as no > > + * other checks, besides presence in the hart_isa bitmap, are > > + * made. > > This comment alludes to a dangerous trap, but I'm having trouble > understanding what it is. You cannot, for example, use this for communicating the presence of F or D, since they require a config option to be set before their use is safe. > Perhaps some rewording to more explicitly > state the danger would be appropriate. Other than that: > > Reviewed-by: Evan Green <evan@rivosinc.com>
On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: > On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: > > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > > > > > Factorize ISA extension reporting by using a macro rather than > > > copy/pasting extension names. This will allow adding new extensions more > > > easily. > > > > > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > > --- > > > arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > > > index 473159b5f303..e207874e686e 100644 > > > --- a/arch/riscv/kernel/sys_riscv.c > > > +++ b/arch/riscv/kernel/sys_riscv.c > > > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > > > for_each_cpu(cpu, cpus) { > > > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > > > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > > > - pair->value |= RISCV_HWPROBE_EXT_ZBA; > > > - else > > > - missing |= RISCV_HWPROBE_EXT_ZBA; > > > - > > > - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > > > - pair->value |= RISCV_HWPROBE_EXT_ZBB; > > > - else > > > - missing |= RISCV_HWPROBE_EXT_ZBB; > > > - > > > - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > > > - pair->value |= RISCV_HWPROBE_EXT_ZBS; > > > - else > > > - missing |= RISCV_HWPROBE_EXT_ZBS; > > > +#define CHECK_ISA_EXT(__ext) \ > > > + do { \ > > > + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > > > + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > > > + else \ > > > + missing |= RISCV_HWPROBE_EXT_##__ext; \ > > > + } while (false) > > > + > > > + /* > > > + * Only use CHECK_ISA_EXT() for extensions which can be exposed > > > + * to userspace, regardless of the kernel's configuration, as no > > > + * other checks, besides presence in the hart_isa bitmap, are > > > + * made. > > > > This comment alludes to a dangerous trap, but I'm having trouble > > understanding what it is. > > You cannot, for example, use this for communicating the presence of F or > D, since they require a config option to be set before their use is > safe. Funnily enough, this comment is immediately contradicted by the vector subset extensions, where these CHECK_ISA_EXT() macros are used wrapped in has_vector(). The code looks valid to me, since has_vector() contains the Kconfig check, but does fly in the face of this comment. > > > Perhaps some rewording to more explicitly > > state the danger would be appropriate. Other than that: > > > > Reviewed-by: Evan Green <evan@rivosinc.com>
On Wed, Oct 18, 2023 at 10:37 AM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: > > On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: > > > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: > > > > > > > > Factorize ISA extension reporting by using a macro rather than > > > > copy/pasting extension names. This will allow adding new extensions more > > > > easily. > > > > > > > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > > > --- > > > > arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- > > > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > > > > index 473159b5f303..e207874e686e 100644 > > > > --- a/arch/riscv/kernel/sys_riscv.c > > > > +++ b/arch/riscv/kernel/sys_riscv.c > > > > @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > > > > for_each_cpu(cpu, cpus) { > > > > struct riscv_isainfo *isainfo = &hart_isa[cpu]; > > > > > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBA)) > > > > - pair->value |= RISCV_HWPROBE_EXT_ZBA; > > > > - else > > > > - missing |= RISCV_HWPROBE_EXT_ZBA; > > > > - > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBB)) > > > > - pair->value |= RISCV_HWPROBE_EXT_ZBB; > > > > - else > > > > - missing |= RISCV_HWPROBE_EXT_ZBB; > > > > - > > > > - if (riscv_isa_extension_available(isainfo->isa, ZBS)) > > > > - pair->value |= RISCV_HWPROBE_EXT_ZBS; > > > > - else > > > > - missing |= RISCV_HWPROBE_EXT_ZBS; > > > > +#define CHECK_ISA_EXT(__ext) \ > > > > + do { \ > > > > + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ > > > > + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ > > > > + else \ > > > > + missing |= RISCV_HWPROBE_EXT_##__ext; \ > > > > + } while (false) > > > > + > > > > + /* > > > > + * Only use CHECK_ISA_EXT() for extensions which can be exposed > > > > + * to userspace, regardless of the kernel's configuration, as no > > > > + * other checks, besides presence in the hart_isa bitmap, are > > > > + * made. > > > > > > This comment alludes to a dangerous trap, but I'm having trouble > > > understanding what it is. > > > > You cannot, for example, use this for communicating the presence of F or > > D, since they require a config option to be set before their use is > > safe. > > Funnily enough, this comment is immediately contradicted by the vector > subset extensions, where these CHECK_ISA_EXT() macros are used wrapped > in has_vector(). The code looks valid to me, since has_vector() contains > the Kconfig check, but does fly in the face of this comment. Ohh, got it. The word "can" is doing a lot of heavy lifting in that comment. So maybe something like: "This macro performs little in the way of extension-specific kernel readiness checks. It's assumed other gating factors like required Kconfig settings have already been confirmed to support exposing the given extension to usermode". ... But, you know, make it sparkle. -Evan
On 18/10/2023 19:24, Evan Green wrote: > On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: >> >> Export the following scalar crypto extensions through hwprobe: >> >> - Zbkb >> - Zbkc >> - Zbkx >> - Zknd >> - Zkne >> - Zknh >> - Zksed >> - Zksh >> - Zkt >> >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> --- >> Documentation/riscv/hwprobe.rst | 30 +++++++++++++++++++++++++++ >> arch/riscv/include/uapi/asm/hwprobe.h | 10 +++++++++ >> arch/riscv/kernel/sys_riscv.c | 10 +++++++++ >> 3 files changed, 50 insertions(+) >> >> diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst >> index a52996b22f75..968895562d42 100644 >> --- a/Documentation/riscv/hwprobe.rst >> +++ b/Documentation/riscv/hwprobe.rst >> @@ -77,6 +77,36 @@ The following keys are defined: >> * :c:macro:`RISCV_HWPROBE_EXT_ZBS`: The Zbs extension is supported, as defined >> in version 1.0 of the Bit-Manipulation ISA extensions. >> >> + * :c:macro:`RISCV_HWPROBE_EXT_ZBC` The Zbc extension is supported, as defined >> + in version 1.0 of the Scalar Crypto ISA extensions. > > At least in my v1.0.1 version of the crypto scalar spec, I don't see > Zbc. That seems to be defined in the bit manipulation extensions. Thanks for catching this, this should be same than the previous line. I will actually move the ZBC on another patch since it is not scalar crypto. Clément
On 18/10/2023 19:36, Conor Dooley wrote: > On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: >> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: >>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: >>>> >>>> Factorize ISA extension reporting by using a macro rather than >>>> copy/pasting extension names. This will allow adding new extensions more >>>> easily. >>>> >>>> Signed-off-by: Clément Léger <cleger@rivosinc.com> >>>> --- >>>> arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- >>>> 1 file changed, 18 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c >>>> index 473159b5f303..e207874e686e 100644 >>>> --- a/arch/riscv/kernel/sys_riscv.c >>>> +++ b/arch/riscv/kernel/sys_riscv.c >>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, >>>> for_each_cpu(cpu, cpus) { >>>> struct riscv_isainfo *isainfo = &hart_isa[cpu]; >>>> >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBA)) >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBA; >>>> - else >>>> - missing |= RISCV_HWPROBE_EXT_ZBA; >>>> - >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBB)) >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBB; >>>> - else >>>> - missing |= RISCV_HWPROBE_EXT_ZBB; >>>> - >>>> - if (riscv_isa_extension_available(isainfo->isa, ZBS)) >>>> - pair->value |= RISCV_HWPROBE_EXT_ZBS; >>>> - else >>>> - missing |= RISCV_HWPROBE_EXT_ZBS; >>>> +#define CHECK_ISA_EXT(__ext) \ >>>> + do { \ >>>> + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ >>>> + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ >>>> + else \ >>>> + missing |= RISCV_HWPROBE_EXT_##__ext; \ >>>> + } while (false) >>>> + >>>> + /* >>>> + * Only use CHECK_ISA_EXT() for extensions which can be exposed >>>> + * to userspace, regardless of the kernel's configuration, as no >>>> + * other checks, besides presence in the hart_isa bitmap, are >>>> + * made. >>> >>> This comment alludes to a dangerous trap, but I'm having trouble >>> understanding what it is. >> >> You cannot, for example, use this for communicating the presence of F or >> D, since they require a config option to be set before their use is >> safe. > > Funnily enough, this comment is immediately contradicted by the vector > subset extensions, where these CHECK_ISA_EXT() macros are used wrapped > in has_vector(). The code looks valid to me, since has_vector() contains > the Kconfig check, but does fly in the face of this comment. Yes, the KConfig checks are already done by the headers, adding #ifdef would be redundant even if more coherent with the comment. BTW, wouldn't it make more sense to get rid out of the unsupported extensions directly at ISA string parsing ? ie, if kernel is compiled without V support, then do not set the bits corresponding to these in the riscv_isa_ext[] array ? But the initial intent was probably to be able to report the full string through cpuinfo. Clément > >> >>> Perhaps some rewording to more explicitly >>> state the danger would be appropriate. Other than that: >>> >>> Reviewed-by: Evan Green <evan@rivosinc.com> > >
On 18/10/2023 19:26, Evan Green wrote: > On Wed, Oct 18, 2023 at 5:53 AM Clément Léger <cleger@rivosinc.com> wrote: >> >> >> >> On 18/10/2023 03:45, Jerry Shih wrote: >>> On Oct 17, 2023, at 21:14, Clément Léger <cleger@rivosinc.com> wrote: >>>> @@ -221,6 +261,22 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { >>>> __RISCV_ISA_EXT_DATA(zkt, RISCV_ISA_EXT_ZKT), >>>> __RISCV_ISA_EXT_DATA(zksed, RISCV_ISA_EXT_ZKSED), >>>> __RISCV_ISA_EXT_DATA(zksh, RISCV_ISA_EXT_ZKSH), >>>> + __RISCV_ISA_EXT_DATA(zvbb, RISCV_ISA_EXT_ZVBB), >>>> + __RISCV_ISA_EXT_DATA(zvbc, RISCV_ISA_EXT_ZVBC), >>>> + __RISCV_ISA_EXT_DATA(zvkb, RISCV_ISA_EXT_ZVKB), >>> >>> The `Zvkb` is the subset of `Zvbb`[1]. So, the `Zvkb` should be bundled with `Zvbb`. >> >> Hi Jerry, >> >> Thanks for catching this, I think some other extensions will fall in >> this category as well then (Zvknha/Zvknhb). I will verify that. > > The bundling mechanism works well when an extension is a pure lasso > around other extensions. We'd have to tweak that code if we wanted to > support cases like this, where the extension is a superset of others, > but also contains loose change not present anywhere else (and > therefore also needs to stand as a separate bit). For Zvbb and Zvknhb, I used the following code: static const unsigned int riscv_zvbb_bundled_exts[] = { RISCV_ISA_EXT_ZVKB, RISCV_ISA_EXT_ZVBB }; static const unsigned int riscv_zvknhb_bundled_exts[] = { RISCV_ISA_EXT_ZVKNHA, RISCV_ISA_EXT_ZVKNHB }; Which correctly results in both extension (superset + base set) being enabled when only one is set. Is there something that I'm missing ? > > IMO, decomposing "pure" bundles makes sense since otherwise usermode > would have to query multiple distinct bitmaps that meant the same > thing (eg check the Zk bit, or maybe check the Zkn/Zkr/Zkt bits, or > maybe check the Zbkb/Zbkc... bits, and they're all equivalent). But > when an extension is a superset that also contains loose change, there > really aren't two equivalent bitmasks, each bit adds something new. Agreed but if a system only report ZVBB for instance and the user wants ZVKB, then it is clear that ZVKB should be reported as well I guess. So in the end, it works much like "bundle" extension, just that the bundle is actually a "real" ISA extension by itself. Clément > > There's an argument to be made for still turning on the containing > extensions to cover for silly ISA strings (eg ISA strings that > advertise the superset but fail to advertise the containing > extensions). We can decide if we want to work that hard to cover > hypothetical broken ISA strings now, or wait until they show up. > Personally I would wait until something broken shows up. But others > may feel differently. > > -Evan
On 19/10/2023 12:22, Conor Dooley wrote: > On Thu, Oct 19, 2023 at 09:26:31AM +0200, Clément Léger wrote: >> >> >> On 18/10/2023 19:36, Conor Dooley wrote: >>> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote: >>>> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote: >>>>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@rivosinc.com> wrote: >>>>>> >>>>>> Factorize ISA extension reporting by using a macro rather than >>>>>> copy/pasting extension names. This will allow adding new extensions more >>>>>> easily. >>>>>> >>>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com> >>>>>> --- >>>>>> arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++-------------- >>>>>> 1 file changed, 18 insertions(+), 14 deletions(-) >>>>>> >>>>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c >>>>>> index 473159b5f303..e207874e686e 100644 >>>>>> --- a/arch/riscv/kernel/sys_riscv.c >>>>>> +++ b/arch/riscv/kernel/sys_riscv.c >>>>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, >>>>>> for_each_cpu(cpu, cpus) { >>>>>> struct riscv_isainfo *isainfo = &hart_isa[cpu]; >>>>>> >>>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBA)) >>>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBA; >>>>>> - else >>>>>> - missing |= RISCV_HWPROBE_EXT_ZBA; >>>>>> - >>>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBB)) >>>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBB; >>>>>> - else >>>>>> - missing |= RISCV_HWPROBE_EXT_ZBB; >>>>>> - >>>>>> - if (riscv_isa_extension_available(isainfo->isa, ZBS)) >>>>>> - pair->value |= RISCV_HWPROBE_EXT_ZBS; >>>>>> - else >>>>>> - missing |= RISCV_HWPROBE_EXT_ZBS; >>>>>> +#define CHECK_ISA_EXT(__ext) \ >>>>>> + do { \ >>>>>> + if (riscv_isa_extension_available(isainfo->isa, __ext)) \ >>>>>> + pair->value |= RISCV_HWPROBE_EXT_##__ext; \ >>>>>> + else \ >>>>>> + missing |= RISCV_HWPROBE_EXT_##__ext; \ >>>>>> + } while (false) >>>>>> + >>>>>> + /* >>>>>> + * Only use CHECK_ISA_EXT() for extensions which can be exposed >>>>>> + * to userspace, regardless of the kernel's configuration, as no >>>>>> + * other checks, besides presence in the hart_isa bitmap, are >>>>>> + * made. >>>>> >>>>> This comment alludes to a dangerous trap, but I'm having trouble >>>>> understanding what it is. >>>> >>>> You cannot, for example, use this for communicating the presence of F or >>>> D, since they require a config option to be set before their use is >>>> safe. >>> >>> Funnily enough, this comment is immediately contradicted by the vector >>> subset extensions, where these CHECK_ISA_EXT() macros are used wrapped >>> in has_vector(). The code looks valid to me, since has_vector() contains >>> the Kconfig check, but does fly in the face of this comment. > >> Yes, the KConfig checks are already done by the headers, adding #ifdef >> would be redundant even if more coherent with the comment > > I don't really understand what the first part of this means, or why using > avoidable ifdeffery here would be desirable. Sorry, I was not clear enough. What I meant is that the has_fpu() and has_vector() functions are already ifdef'd in headers based on the KConfig options for their support (CONFIG_FPU/CONFIG_RISCV_ISA_V) So in the end, using ifdef here in hwprobe_isa_ext0() would be redundant. > >> BTW, wouldn't >> it make more sense to get rid out of the unsupported extensions directly >> at ISA string parsing ? ie, if kernel is compiled without V support, >> then do not set the bits corresponding to these in the riscv_isa_ext[] >> array ? But the initial intent was probably to be able to report the >> full string through cpuinfo. > > Yeah, hysterical raisins I guess, it's always been that way. I don't > think anyone originally thought about such configurations and that is > how the cpuinfo stuff behaves. I strongly dislike the > riscv_isa_extension_available() interface, but one of Drew's patches > does at least improve things a bit. Kinda waiting for some of the > patches in flight to settle down before deciding if I want to refactor > stuff to be less of a potential for shooting oneself in the foot. Make sense. Clément
On Thu, Oct 19, 2023 at 3:24 AM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Oct 19, 2023 at 11:46:51AM +0200, Clément Léger wrote: > > Indeed the comment was a bit misleading, is this more clear ? > > > > /* > > * Only use CHECK_ISA_EXT() for extensions which are usable by > > * userspace with respect to the kernel current configuration. > > * For instance, ISA extensions that uses float operations > > s/that uses/that use/ > > > * should not be exposed when CONFIG_FPU is not set. > > s/is not set/is not enabled/ > > But yeah, definitely more clear, thanks. Looks good to me too. Thanks, Clément! -Evan
On Thu, Oct 19, 2023 at 8:33 AM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Oct 19, 2023 at 11:35:59AM +0200, Clément Léger wrote: > > > > > > On 18/10/2023 19:26, Evan Green wrote: > > > On Wed, Oct 18, 2023 at 5:53 AM Clément Léger <cleger@rivosinc.com> wrote: > > >> > > >> > > >> > > >> On 18/10/2023 03:45, Jerry Shih wrote: > > >>> On Oct 17, 2023, at 21:14, Clément Léger <cleger@rivosinc.com> wrote: > > >>>> @@ -221,6 +261,22 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = { > > >>>> __RISCV_ISA_EXT_DATA(zkt, RISCV_ISA_EXT_ZKT), > > >>>> __RISCV_ISA_EXT_DATA(zksed, RISCV_ISA_EXT_ZKSED), > > >>>> __RISCV_ISA_EXT_DATA(zksh, RISCV_ISA_EXT_ZKSH), > > >>>> + __RISCV_ISA_EXT_DATA(zvbb, RISCV_ISA_EXT_ZVBB), > > >>>> + __RISCV_ISA_EXT_DATA(zvbc, RISCV_ISA_EXT_ZVBC), > > >>>> + __RISCV_ISA_EXT_DATA(zvkb, RISCV_ISA_EXT_ZVKB), > > >>> > > >>> The `Zvkb` is the subset of `Zvbb`[1]. So, the `Zvkb` should be bundled with `Zvbb`. > > >> > > >> Hi Jerry, > > >> > > >> Thanks for catching this, I think some other extensions will fall in > > >> this category as well then (Zvknha/Zvknhb). I will verify that. > > > > > > The bundling mechanism works well when an extension is a pure lasso > > > around other extensions. We'd have to tweak that code if we wanted to > > > support cases like this, where the extension is a superset of others, > > > but also contains loose change not present anywhere else (and > > > therefore also needs to stand as a separate bit). > > > > For Zvbb and Zvknhb, I used the following code: > > > > static const unsigned int riscv_zvbb_bundled_exts[] = { > > RISCV_ISA_EXT_ZVKB, > > RISCV_ISA_EXT_ZVBB > > }; > > > > static const unsigned int riscv_zvknhb_bundled_exts[] = { > > RISCV_ISA_EXT_ZVKNHA, > > RISCV_ISA_EXT_ZVKNHB > > }; > > > > Which correctly results in both extension (superset + base set) being > > enabled when only one is set. Is there something that I'm missing ? > > > > > > > > IMO, decomposing "pure" bundles makes sense since otherwise usermode > > > would have to query multiple distinct bitmaps that meant the same > > > thing (eg check the Zk bit, or maybe check the Zkn/Zkr/Zkt bits, or > > > maybe check the Zbkb/Zbkc... bits, and they're all equivalent). But > > > when an extension is a superset that also contains loose change, there > > > really aren't two equivalent bitmasks, each bit adds something new. > > > > Agreed but if a system only report ZVBB for instance and the user wants > > ZVKB, then it is clear that ZVKB should be reported as well I guess. So > > in the end, it works much like "bundle" extension, just that the bundle > > is actually a "real" ISA extension by itself. > > > > Clément > > > > > > > > There's an argument to be made for still turning on the containing > > > extensions to cover for silly ISA strings (eg ISA strings that > > > advertise the superset but fail to advertise the containing > > > extensions). We can decide if we want to work that hard to cover > > > hypothetical broken ISA strings now, or wait until they show up. > > > Personally I would wait until something broken shows up. But others > > > may feel differently. > > I'm not really sure that those are "silly" ISA strings. People are going > to do it that way because it is much easier than spelling out 5 dozen > sub-components, and it is pretty inevitable that subsets will be > introduced in the future for extensions we currently have. > > IMO, it's perfectly valid to say you have the supersets and not spell > out all the subcomponents. Hm, ok. If ISA strings are likely to be written that way, then I agree having the kernel flip on all the contained extensions is a good idea. We can tweak patch 2 to support the parsing of struct riscv_isa_ext_data with both .id and .bundle_size set (instead of only one or the other as it is now). Looking back at that patch, it looks quite doable. Alright! -Evan