RISC-V: expose extension bits in AT_HWCAP

[mhorne]

Summary

AT_HWCAP is an elf auxiliary vector that describes cpu-specific hardware
features. For RISC-V we want to use this to indicate the presence of any
standard extensions supported by the CPU. This allows userland
applications to query the system for supported extensions using
elf_aux_info(3).

Support for an extension is indicated by the presence of its
corresponding bit in AT_HWCAP -- e.g. systems supporting the 'c'
extension (compressed instructions) will have the second bit set.

Extensions advertised through AT_HWCAP are only those that are supported
by all harts in the system.

Test Plan

elf_aux_info(3) returns a value of 0x112D (imafdc).

[mhorne]

sys/riscv/riscv/identcpu.c

108 ↗

(On Diff #58153)

I'm not thrilled about the placement of this comment, but didn't want to leave a magic number. I suppose I could move it to a define if that seems preferable.

[markj]

Looks fine to me, a few minor comments below.

sys/riscv/include/elf.h

78 ↗

(On Diff #58153)

Maybe add

#define HWCAP_ISA_BIT(c) (1 << (c - 'A'))
#define HWCAP_ISA_I HWCAP_ISA_BIT('I')
...

Just a suggestion, feel free to keep it as-is.

sys/riscv/riscv/identcpu.c

108 ↗

(On Diff #58153)

I'd add a define, maybe ISA_NAME_MAXLEN.

126 ↗

(On Diff #58153)

Please prefix the message with the function name, ditto below.

[mhorne]

Address markj's comments.

[markj]

sys/riscv/riscv/identcpu.c

150 ↗

(On Diff #58166)

Hmm, should we also check for truncation, perhaps with an assertion?

[mhorne]

Add KASSERT for len <= ISA_NAME_MAXLEN.
Remove line adding null terminator since we don't actually need it.

[markj]

Approved.

[jhb]

sys/riscv/riscv/identcpu.c

110 ↗

(On Diff #58178)

SYSINIT's are 'void foo(void *)'. We use __unused when the argument isn't used, so I would change this to:

static void
fill_elf_hwcap(void *dummy __unused)

161 ↗

(On Diff #58178)

I think should probably check this instead of assuming len >= 5. This would also make it fail on rv128 if that ever exists. Something like:

if (len < 4 || memcmp(isa, "rv64") != 0) {
    printf("fill_elf_hwcap: Unsupported ISA string %.*s\n",
        (int)len, isa);
    return;
}

[mhorne]

Address jhb's comments.

[mhorne]

sys/riscv/riscv/identcpu.c

108 ↗

(On Diff #58335)

This is a little overkill but should handle any valid XLEN.

161 ↗

(On Diff #58178)

Very cool. I didn't know about the .* modifier, thanks.

[mhorne]

sys/riscv/include/elf.h

86 ↗

(On Diff #58335)

Also, added HWCAP_ISA_G for convenience since these are exposed to userspace through sys/auxv.h.

[markj]

sys/riscv/riscv/identcpu.c

159 ↗

(On Diff #58335)

I think you can use strncmp() in place of these two conditions.

[jhb]

sys/riscv/riscv/identcpu.c

159 ↗

(On Diff #58335)

I was assuming that OF_getprop wasn't returning a valid C string just an array of bytes with a len. However, you could do this to work around that:

char isa[ISA_NAME_MAXLEN + 1];

...
    len = OF_getprop(node, "riscv,isa", (pcell_t *)isa,
        sizeof(isa) - 1));
    KASSERT(len <= sizeof(isa) - 1, ...);
    if (len == -1) {
        ...
    }
    isa[len] = '\0';
    if (strncmp(isa, ISA_PREFIX, ISA_PREFIX_LEN) != 0) {
        ....
    }

That also lets you use a plain '%s' in the error handling instead of '%.*s'.

[markj]

sys/riscv/riscv/identcpu.c

159 ↗

(On Diff #58335)

I believe that OF_getprop() does in fact return a C string here. String properties are documented to be nul-terminated in OF_getprop(9).

[mhorne]

Convert memcmp to strncmp.

[jhb]

sys/riscv/riscv/identcpu.c

159 ↗

(On Diff #58335)

FWIW, I can't find anything in either the OFW or FDT backends that guarantees this. We are just assuming that the firmware call (in the OFW case) or the FDT blob is going to follow this rule. It seems we might be assuming this throughout the kernel, so being defensive in just this one place doesn't seem to make sense. Unfortunately, there is no way to detect missing nuls centrally (e.g. in ofw_fdt.c or ofw_standard.c) as the type of a given property is implicit knowledge between the consumer and provider.

[markj]

sys/riscv/riscv/identcpu.c

159 ↗

(On Diff #58335)

Yeah, that was my reasoning here: I found lots of other places that blindly assume the presence of a nul terminator.

[markj]

Approved.