Re: [PATCH 2/3] RISC-V: hwprobe: Expose Zbc and the scalar crypto extensions

From: Stefan O'Rear
Date: Wed Jun 28 2023 - 09:26:41 EST


On Wed, Jun 28, 2023, at 6:04 AM, Samuel Ortiz wrote:
> On Tue, Jun 27, 2023 at 08:34:20PM -0400, Stefan O'Rear wrote:
>> On Tue, Jun 27, 2023, at 10:37 AM, Samuel Ortiz wrote:
>> > Zbc was missing from a previous Bit-Manipulation extension hwprobe
>> > patch.
>> >
>> > Add all scalar crypto extensions bits, and define a macro for setting
>> > the hwprobe key/pair in a more readable way.
>> >
>> > Signed-off-by: Samuel Ortiz <sameo@xxxxxxxxxxxx>
>> > ---
>> > Documentation/riscv/hwprobe.rst | 33 ++++++++++++++++++++++++
>> > arch/riscv/include/uapi/asm/hwprobe.h | 11 ++++++++
>> > arch/riscv/kernel/sys_riscv.c | 36 ++++++++++++++++-----------
>> > 3 files changed, 66 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
>> > index 19165ebd82ba..3177550106e0 100644
>> > --- a/Documentation/riscv/hwprobe.rst
>> > +++ b/Documentation/riscv/hwprobe.rst
>> > @@ -72,11 +72,44 @@ The following keys are defined:
>> > extensions.
>> >
>> > * :c:macro:`RISCV_HWPROBE_EXT_ZBB`: The Zbb 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 Bit-Manipulation ISA extensions.
>> >
>> > * :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_ZBKB`: The Zbkb extension is
>> > supported, as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZBKC`: The Zbkc extension is
>> > supported, as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZBKX`: The Zbkx extension is
>> > supported, as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZKND`: The Zknd extension is
>> > supported, as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZKNE`: The Zkne extension is
>> > supported, as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZKNH`: The Zknh extension is
>> > supported, as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZKR`: The Zkr extension is supported,
>> > as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZKSED`: The Zksed extension is
>> > supported, as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZKSH`: The Zksh extension is
>> > supported, as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > + * :c:macro:`RISCV_HWPROBE_EXT_ZKT`: The Zkt extension is supported,
>> > as defined
>> > + in version 1.0 of the Scalar Cryptography ISA extensions.
>> > +
>> > * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains
>> > performance
>> > information about the selected set of processors.
>> >
>> > diff --git a/arch/riscv/include/uapi/asm/hwprobe.h
>> > b/arch/riscv/include/uapi/asm/hwprobe.h
>> > index 006bfb48343d..8357052061b3 100644
>> > --- a/arch/riscv/include/uapi/asm/hwprobe.h
>> > +++ b/arch/riscv/include/uapi/asm/hwprobe.h
>> > @@ -29,6 +29,17 @@ struct riscv_hwprobe {
>> > #define RISCV_HWPROBE_EXT_ZBA (1 << 3)
>> > #define RISCV_HWPROBE_EXT_ZBB (1 << 4)
>> > #define RISCV_HWPROBE_EXT_ZBS (1 << 5)
>> > +#define RISCV_HWPROBE_EXT_ZBC (1 << 6)
>> > +#define RISCV_HWPROBE_EXT_ZBKB (1 << 7)
>> > +#define RISCV_HWPROBE_EXT_ZBKC (1 << 8)
>> > +#define RISCV_HWPROBE_EXT_ZBKX (1 << 9)
>> > +#define RISCV_HWPROBE_EXT_ZKND (1 << 10)
>> > +#define RISCV_HWPROBE_EXT_ZKNE (1 << 11)
>> > +#define RISCV_HWPROBE_EXT_ZKNH (1 << 12)
>> > +#define RISCV_HWPROBE_EXT_ZKR (1 << 13)
>> > +#define RISCV_HWPROBE_EXT_ZKSED (1 << 14)
>> > +#define RISCV_HWPROBE_EXT_ZKSH (1 << 15)
>> > +#define RISCV_HWPROBE_EXT_ZKT (1 << 16)
>> > #define RISCV_HWPROBE_KEY_CPUPERF_0 5
>> > #define RISCV_HWPROBE_MISALIGNED_UNKNOWN (0 << 0)
>> > #define RISCV_HWPROBE_MISALIGNED_EMULATED (1 << 0)
>> > diff --git a/arch/riscv/kernel/sys_riscv.c
>> > b/arch/riscv/kernel/sys_riscv.c
>> > index 26ef5526bfb4..df15926196b6 100644
>> > --- a/arch/riscv/kernel/sys_riscv.c
>> > +++ b/arch/riscv/kernel/sys_riscv.c
>> > @@ -145,20 +145,28 @@ 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 SET_HWPROBE_EXT_PAIR(ext) \
>> > + do { \
>> > + if (riscv_isa_extension_available(isainfo->isa, ext)) \
>> > + pair->value |= RISCV_HWPROBE_EXT_## ext; \
>> > + else \
>> > + missing |= RISCV_HWPROBE_EXT_## ext; \
>> > + } while (false) \
>> > +
>> > + SET_HWPROBE_EXT_PAIR(ZBA);
>> > + SET_HWPROBE_EXT_PAIR(ZBB);
>> > + SET_HWPROBE_EXT_PAIR(ZBC);
>> > + SET_HWPROBE_EXT_PAIR(ZBS);
>> > + SET_HWPROBE_EXT_PAIR(ZBKB);
>> > + SET_HWPROBE_EXT_PAIR(ZBKC);
>> > + SET_HWPROBE_EXT_PAIR(ZBKX);
>> > + SET_HWPROBE_EXT_PAIR(ZKND);
>> > + SET_HWPROBE_EXT_PAIR(ZKNE);
>> > + SET_HWPROBE_EXT_PAIR(ZKNH);
>> > + SET_HWPROBE_EXT_PAIR(ZKR);
>>
>> Does the presence of a HWPROBE_EXT bit imply that userspace software can
>> actually directly use the described feature? If so, we should probably
>> not set ZKR unless mseccfg.USEED=1.
>
> mseccfg is MRW, so only accessible from M-mode only afaiu. So I don't
> think we would be able to check that from Linux in S-mode.

Check directly, no, but your patch already makes the assumption that
mseccfg.SSEED=1 if zkr is present in the device tree. Which is fine as long
as that contract is documented somewhere (presumably, the device tree
binding; some of the language in the RVA22U64 profile spec implies USEED=0,
but linux does not require profiles and they don't exist for rv32).

If we want U-mode behavior to be discoverable and/or predictable, we have
three good options:

Simplest: Document that we expect USEED=0 or USEED=1. Set zkr appropriately
in hwprobe.

Most flexible: Work with the SBI people to add a SBI_EXT_FWFEATURE for USEED,
as well as defining the value on kernel entry. Expose this via hwprobe and
a new prctl.

Most robust: Add an illegal instruction handler for CSR_SEED that grabs 16
bits from the kernel entropy pool. Expose zkr in hwprobe unconditionally.

-s

> Cheers,
> Samuel.