| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230626-0176f6ea5768a6013a7aecdc@orel>
Date: Mon, 26 Jun 2023 18:17:51 +0200
From: Andrew Jones <ajones@...tanamicro.com>
To: Conor Dooley <conor.dooley@...rochip.com>
Cc: palmer@...belt.com, conor@...nel.org,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Albert Ou <aou@...s.berkeley.edu>,
Heiko Stuebner <heiko.stuebner@...ll.eu>,
Evan Green <evan@...osinc.com>,
Sunil V L <sunilvl@...tanamicro.com>,
linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 7/9] RISC-V: split riscv_fill_hwcap() in 3
On Mon, Jun 26, 2023 at 12:19:45PM +0100, Conor Dooley wrote:
> Before adding more complexity to it, split riscv_fill_hwcap() into 3
> distinct sections:
> - riscv_fill_hwcap() still is the top level function, into which the
> additional complexity will be added.
> - riscv_fill_hwcap_from_isa_string() handles getting the information
> from the riscv,isa/ACPI equivalent across harts & the various quirks
> there
> - riscv_parse_isa_string() does what it says on the tin.
>
> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
> ---
> arch/riscv/kernel/cpufeature.c | 350 +++++++++++++++++----------------
> 1 file changed, 182 insertions(+), 168 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 5405d8a58537..366477ba1eea 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -180,29 +180,172 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>
> const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
>
> -void __init riscv_fill_hwcap(void)
> +static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct riscv_isainfo *isainfo,
> + unsigned long *isa2hwcap, const char *isa)
> +{
> + /*
> + * For all possible cpus, we have already validated in
> + * the boot process that they at least contain "rv" and
> + * whichever of "32"/"64" this kernel supports, and so this
> + * section can be skipped.
> + */
> + isa += 4;
> +
> + while (*isa) {
> + const char *ext = isa++;
> + const char *ext_end = isa;
> + bool ext_long = false, ext_err = false;
> +
> + switch (*ext) {
> + case 's':
> + /*
> + * Workaround for invalid single-letter 's' & 'u'(QEMU).
> + * No need to set the bit in riscv_isa as 's' & 'u' are
> + * not valid ISA extensions. It works until multi-letter
> + * extension starting with "Su" appears.
> + */
> + if (ext[-1] != '_' && ext[1] == 'u') {
> + ++isa;
> + ext_err = true;
> + break;
> + }
> + fallthrough;
> + case 'S':
> + case 'x':
> + case 'X':
> + case 'z':
> + case 'Z':
> + /*
> + * Before attempting to parse the extension itself, we find its end.
> + * As multi-letter extensions must be split from other multi-letter
> + * extensions with an "_", the end of a multi-letter extension will
> + * either be the null character or the "_" at the start of the next
> + * multi-letter extension.
> + *
> + * Next, as the extensions version is currently ignored, we
> + * eliminate that portion. This is done by parsing backwards from
> + * the end of the extension, removing any numbers. This may be a
> + * major or minor number however, so the process is repeated if a
> + * minor number was found.
> + *
> + * ext_end is intended to represent the first character *after* the
> + * name portion of an extension, but will be decremented to the last
> + * character itself while eliminating the extensions version number.
> + * A simple re-increment solves this problem.
> + */
> + ext_long = true;
> + for (; *isa && *isa != '_'; ++isa)
> + if (unlikely(!isalnum(*isa)))
> + ext_err = true;
> +
> + ext_end = isa;
> + if (unlikely(ext_err))
> + break;
> +
> + if (!isdigit(ext_end[-1]))
> + break;
> +
> + while (isdigit(*--ext_end))
> + ;
> +
> + if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
> + ++ext_end;
> + break;
> + }
> +
> + while (isdigit(*--ext_end))
> + ;
> +
> + ++ext_end;
> + break;
> + default:
> + /*
> + * Things are a little easier for single-letter extensions, as they
> + * are parsed forwards.
> + *
> + * After checking that our starting position is valid, we need to
> + * ensure that, when isa was incremented at the start of the loop,
> + * that it arrived at the start of the next extension.
> + *
> + * If we are already on a non-digit, there is nothing to do. Either
> + * we have a multi-letter extension's _, or the start of an
> + * extension.
> + *
> + * Otherwise we have found the current extension's major version
> + * number. Parse past it, and a subsequent p/minor version number
> + * if present. The `p` extension must not appear immediately after
> + * a number, so there is no fear of missing it.
> + *
> + */
> + if (unlikely(!isalpha(*ext))) {
> + ext_err = true;
> + break;
> + }
> +
> + if (!isdigit(*isa))
> + break;
> +
> + while (isdigit(*++isa))
> + ;
> +
> + if (tolower(*isa) != 'p')
> + break;
> +
> + if (!isdigit(*++isa)) {
> + --isa;
> + break;
> + }
> +
> + while (isdigit(*++isa))
> + ;
> +
> + break;
> + }
> +
> + /*
> + * The parser expects that at the start of an iteration isa points to the
> + * first character of the next extension. As we stop parsing an extension
> + * on meeting a non-alphanumeric character, an extra increment is needed
> + * where the succeeding extension is a multi-letter prefixed with an "_".
> + */
> + if (*isa == '_')
> + ++isa;
> +
> +#define SET_ISA_EXT_MAP(name, bit) \
> + do { \
> + if ((ext_end - ext == sizeof(name) - 1) && \
> + !strncasecmp(ext, name, sizeof(name) - 1) && \
> + riscv_isa_extension_check(bit)) \
> + set_bit(bit, isainfo->isa); \
> + } while (false) \
> +
> + if (unlikely(ext_err))
> + continue;
> + if (!ext_long) {
> + int nr = tolower(*ext) - 'a';
> +
> + if (riscv_isa_extension_check(nr)) {
> + *this_hwcap |= isa2hwcap[nr];
> + set_bit(nr, isainfo->isa);
> + }
> + } else {
> + for (int i = 0; i < riscv_isa_ext_count; i++)
> + SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
> + riscv_isa_ext[i].id);
> + }
> +#undef SET_ISA_EXT_MAP
> + }
> +}
> +
> +static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
> {
> struct device_node *node;
> const char *isa;
> - char print_str[NUM_ALPHA_EXTS + 1];
> - int i, j, rc;
> - unsigned long isa2hwcap[26] = {0};
> + int rc;
> struct acpi_table_header *rhct;
> acpi_status status;
> unsigned int cpu;
>
> - isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
> - isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
> - isa2hwcap['a' - 'a'] = COMPAT_HWCAP_ISA_A;
> - isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
> - isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
> - isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
> - isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
> -
> - elf_hwcap = 0;
> -
> - bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX);
> -
> if (!acpi_disabled) {
> status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> if (ACPI_FAILURE(status))
> @@ -234,158 +377,7 @@ void __init riscv_fill_hwcap(void)
> }
> }
>
> - /*
> - * For all possible cpus, we have already validated in
> - * the boot process that they at least contain "rv" and
> - * whichever of "32"/"64" this kernel supports, and so this
> - * section can be skipped.
> - */
> - isa += 4;
> -
> - while (*isa) {
> - const char *ext = isa++;
> - const char *ext_end = isa;
> - bool ext_long = false, ext_err = false;
> -
> - switch (*ext) {
> - case 's':
> - /*
> - * Workaround for invalid single-letter 's' & 'u'(QEMU).
> - * No need to set the bit in riscv_isa as 's' & 'u' are
> - * not valid ISA extensions. It works until multi-letter
> - * extension starting with "Su" appears.
> - */
> - if (ext[-1] != '_' && ext[1] == 'u') {
> - ++isa;
> - ext_err = true;
> - break;
> - }
> - fallthrough;
> - case 'S':
> - case 'x':
> - case 'X':
> - case 'z':
> - case 'Z':
> - /*
> - * Before attempting to parse the extension itself, we find its end.
> - * As multi-letter extensions must be split from other multi-letter
> - * extensions with an "_", the end of a multi-letter extension will
> - * either be the null character or the "_" at the start of the next
> - * multi-letter extension.
> - *
> - * Next, as the extensions version is currently ignored, we
> - * eliminate that portion. This is done by parsing backwards from
> - * the end of the extension, removing any numbers. This may be a
> - * major or minor number however, so the process is repeated if a
> - * minor number was found.
> - *
> - * ext_end is intended to represent the first character *after* the
> - * name portion of an extension, but will be decremented to the last
> - * character itself while eliminating the extensions version number.
> - * A simple re-increment solves this problem.
> - */
> - ext_long = true;
> - for (; *isa && *isa != '_'; ++isa)
> - if (unlikely(!isalnum(*isa)))
> - ext_err = true;
> -
> - ext_end = isa;
> - if (unlikely(ext_err))
> - break;
> -
> - if (!isdigit(ext_end[-1]))
> - break;
> -
> - while (isdigit(*--ext_end))
> - ;
> -
> - if (tolower(ext_end[0]) != 'p' || !isdigit(ext_end[-1])) {
> - ++ext_end;
> - break;
> - }
> -
> - while (isdigit(*--ext_end))
> - ;
> -
> - ++ext_end;
> - break;
> - default:
> - /*
> - * Things are a little easier for single-letter extensions, as they
> - * are parsed forwards.
> - *
> - * After checking that our starting position is valid, we need to
> - * ensure that, when isa was incremented at the start of the loop,
> - * that it arrived at the start of the next extension.
> - *
> - * If we are already on a non-digit, there is nothing to do. Either
> - * we have a multi-letter extension's _, or the start of an
> - * extension.
> - *
> - * Otherwise we have found the current extension's major version
> - * number. Parse past it, and a subsequent p/minor version number
> - * if present. The `p` extension must not appear immediately after
> - * a number, so there is no fear of missing it.
> - *
> - */
> - if (unlikely(!isalpha(*ext))) {
> - ext_err = true;
> - break;
> - }
> -
> - if (!isdigit(*isa))
> - break;
> -
> - while (isdigit(*++isa))
> - ;
> -
> - if (tolower(*isa) != 'p')
> - break;
> -
> - if (!isdigit(*++isa)) {
> - --isa;
> - break;
> - }
> -
> - while (isdigit(*++isa))
> - ;
> -
> - break;
> - }
> -
> - /*
> - * The parser expects that at the start of an iteration isa points to the
> - * first character of the next extension. As we stop parsing an extension
> - * on meeting a non-alphanumeric character, an extra increment is needed
> - * where the succeeding extension is a multi-letter prefixed with an "_".
> - */
> - if (*isa == '_')
> - ++isa;
> -
> -#define SET_ISA_EXT_MAP(name, bit) \
> - do { \
> - if ((ext_end - ext == sizeof(name) - 1) && \
> - !strncasecmp(ext, name, sizeof(name) - 1) && \
> - riscv_isa_extension_check(bit)) \
> - set_bit(bit, isainfo->isa); \
> - } while (false) \
> -
> - if (unlikely(ext_err))
> - continue;
> - if (!ext_long) {
> - int nr = tolower(*ext) - 'a';
> -
> - if (riscv_isa_extension_check(nr)) {
> - this_hwcap |= isa2hwcap[nr];
> - set_bit(nr, isainfo->isa);
> - }
> - } else {
> - for (int i = 0; i < riscv_isa_ext_count; i++)
> - SET_ISA_EXT_MAP(riscv_isa_ext[i].name,
> - riscv_isa_ext[i].id);
> - }
> -#undef SET_ISA_EXT_MAP
> - }
> + riscv_parse_isa_string(&this_hwcap, isainfo, isa2hwcap, isa);
>
> /*
> * Linux requires the following extensions, so we may as well
> @@ -422,6 +414,28 @@ void __init riscv_fill_hwcap(void)
>
> if (!acpi_disabled && rhct)
> acpi_put_table((struct acpi_table_header *)rhct);
> +}
> +
> +void __init riscv_fill_hwcap(void)
> +{
> + struct device_node *node;
> + const char *isa;
> + char print_str[NUM_ALPHA_EXTS + 1];
> + int i, j, rc;
> + unsigned long isa2hwcap[26] = {0};
> + struct acpi_table_header *rhct;
> + acpi_status status;
> + unsigned int cpu;
I see all these unused variables get removed in the next patch, but they
should get removed here, lest they trigger some warnings and bots come
after you!
> +
> + isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
> + isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
> + isa2hwcap['a' - 'a'] = COMPAT_HWCAP_ISA_A;
> + isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
> + isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
> + isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
> + isa2hwcap['v' - 'a'] = COMPAT_HWCAP_ISA_V;
> +
> + riscv_fill_hwcap_from_isa_string(isa2hwcap);
>
> /* We don't support systems with F but without D, so mask those out
> * here. */
> --
> 2.40.1
>
Otherwise,
Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
Thanks,
drew
Powered by blists - more mailing lists