[PATCH 2/2] RISC-V: Extract base ISA from device tree
Atish Patra
atishp at atishpatra.org
Tue Feb 22 10:10:37 PST 2022
On Mon, Feb 21, 2022 at 5:42 AM Tsukasa OI <research_trasio at irq.a4lg.com> wrote:
>
> On 2022/02/16 16:43, Atish Patra wrote:
> > On Tue, Feb 15, 2022 at 10:59 PM Tsukasa OI
> > <research_trasio at irq.a4lg.com> wrote:
> >>
> >> On 2022/02/16 15:01, Atish Patra wrote:
> >>> On Tue, Feb 15, 2022 at 9:04 PM Tsukasa OI <research_trasio at irq.a4lg.com> wrote:
> >>>>
> >>>> This commit replaces print_isa function to extract base ISA from device
> >>>> tree ("riscv,isa"). It uses a subset of Tsukasa's riscv_fill_hwcap ISA
> >>>> parser.
> >>>>
> >>>> Design choices (1):
> >>>>
> >>>> It constructs base ISA string from the original string (does not cut
> >>>> the original string like Atish's). This is to strip version numbers
> >>>> (too minor to represent as major ISA) but this behavior may be
> >>>> debatable.
> >>>>
> >>>
> >>> A simpler solution would be to just invoke
> >>> __riscv_isa_extension_available for all the base
> >>> extensions (imafdc) and print the base isa string.
> >>> We don't need to parse the ISA string again here.
> >>
> >> I agree that my patchset is ugly and simpler solution would be good.
> >> (Having two parser for same string is definitely not the best idea.)
> >> There are some testcases (some are not accepted by Spike's --isa option
> >> and requires custom Device tree) for testing:
> >>
> >> * rv64imac
> >> * rv64imacsu (invalid ISA string; test QEMU workaround)
> >> * rv64imafdch
> >> * rv64imafdcsuh (invalid ISA string; test QEMU workaround)
> >> * rv64imafdc_svinval_svnapot
> >> * rv64imafdcsvinval_svnapot
> >> * rv64i2p1m2a2fdcsvinval
> >> * rv64i2p1_m_afdcsvinval
> >>
> >
> > Thanks. This is a good set of test cases that covers most of the cases.
> >
> >>>
> >>>> Design choices (2):
> >>>>
> >>>> It skips when single-letter 'S' or 'U' is encountered. It improves
> >>>> familiarity with regular ISA string.
> >>>>
> >>>
> >>> I don't think this is necessary because only Qemu appends 'S' & 'U' in
> >>> the isa string.
> >>> Your patch in Qemu will fix that for future releases of Qemu.
> >>> All other commonly used platforms (hardware/spike) already use the
> >>> correct ISA string.
> >>
> >> Yes, only QEMU. It's not a serious problem like on cpufeature.c but would
> >> be nice to remove them (when exposing to the usermode) to prevent confusion
> >> on usermode side.
> >>
> >> If you invoke __riscv_isa_extension_available to generate base ISA string,
> >> would you exclude 'S' and 'U'?
> >>
> >
> > Yeah. That's easy. The possible valid base extensions are "IMAFHDQCBJPV".
> > (all the possible defined bits in misa except 'S' & 'U')
> > So we will invoke __riscv_isa_extension_available for those only.
>
> Order "IEMAFDQLCBKJTPVH" would be sufficient.
> Note that some of them may be incompatible with Linux or currently has
> no plan to be added as a single-letter extension.
>
> "E"**: RV32E
> (incompatible with Linux?)
Yes. No need to check for this.
> "L"**: Decimal floating point
> (placeholder is removed from the ISA specification)
> "B"**: Bit manipulation
> (no single-letter extension plan but multiple subextensions)
> "K"**: Cryptography
> (no single-letter extension plan but multiple subextensions)
> "J"**: Extensions for dynamic languages
> (no single-letter extension but planned multiple subextensions)
> "T"**: Transactional memory
> (placeholder is removed from the ISA specification)
That's why I did not include L & T as the ISA spec doesn't talk about
it anymore.
I am in favor of not including right now and add it later if any
future ratified spec includes it.
> "P" : narrow SIMD using GPR
> (this order is ratified but "P" is not)
> "H" : Hypervisor (different position compared to your post)
> (this order is not ratified but likely placed here
> for compatibility with QEMU and Spike)
>
I thought it would be after "imafdqc". I will change it as per your suggestion.
We can always fix if the ratified spec will specify a different order.
This is what base ISA extension (single letter) will look like (for now)
"imafdqcbkjpvh"
I will send the patch shortly.
> Tsukasa
>
> >
> >>>
> >>>> This commit is intended to be squashed into Atish's isa_framework_v4
> >>>> PATCH 6/6.
> >>>>
> >>>> Signed-off-by: Tsukasa OI <research_trasio at irq.a4lg.com>
> >>>> ---
> >>>> arch/riscv/kernel/cpu.c | 83 ++++++++++++++++++++++++++++++++++++-----
> >>>> 1 file changed, 74 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> >>>> index 6f9660c0a973..3f9607a66d95 100644
> >>>> --- a/arch/riscv/kernel/cpu.c
> >>>> +++ b/arch/riscv/kernel/cpu.c
> >>>> @@ -101,17 +101,82 @@ static void print_isa_ext(struct seq_file *f)
> >>>>
> >>>> static void print_isa(struct seq_file *f, const char *isa)
> >>>> {
> >>>> - char *ext_start;
> >>>> - int isa_len = strlen(isa);
> >>>> - int base_isa_len = isa_len;
> >>>> -
> >>>> - ext_start = strnchr(isa, isa_len, '_');
> >>>> - if (ext_start)
> >>>> - base_isa_len = isa_len - strlen(ext_start);
> >>>> + unsigned char parse_break = 0;
> >>>> + char base_isa[RISCV_ISA_EXT_BASE + 5];
> >>>> + char *base_isa_end = base_isa;
> >>>> + const char *isa_end = isa;
> >>>> +#if IS_ENABLED(CONFIG_32BIT)
> >>>> + if (!strncmp(isa, "rv32", 4))
> >>>> + isa_end += 4;
> >>>> +#elif IS_ENABLED(CONFIG_64BIT)
> >>>> + if (!strncmp(isa, "rv64", 4))
> >>>> + isa_end += 4;
> >>>> +#endif
> >>>> + if (isa != isa_end) {
> >>>> + strncpy(base_isa, isa, isa_end - isa);
> >>>> + base_isa_end += isa_end - isa;
> >>>> + for (; *isa_end; ++isa_end) {
> >>>> + switch (*isa_end++) {
> >>>> + case 'x':
> >>>> + case 'z':
> >>>> + parse_break = 1;
> >>>> + break;
> >>>> + case 's':
> >>>> + if (*isa_end != 'u') {
> >>>> + parse_break = 1;
> >>>> + break;
> >>>> + }
> >>>> + /**
> >>>> + * Workaround for invalid single-letter 's'
> >>>> + * (QEMU). Should break for valid ISA string.
> >>>> + */
> >>>> + fallthrough;
> >>>> + default:
> >>>> + if (unlikely(!islower(isa_end[-1])
> >>>> + || base_isa_end == base_isa + sizeof(base_isa))) {
> >>>> + parse_break = 2;
> >>>> + break;
> >>>> + }
> >>>> + switch (isa_end[-1]) {
> >>>> + case 's':
> >>>> + case 'u':
> >>>> + break;
> >>>> + default:
> >>>> + *base_isa_end++ = isa_end[-1];
> >>>> + }
> >>>> + /* Skip version number */
> >>>> + if (!isdigit(*isa_end))
> >>>> + break;
> >>>> + while (isdigit(*++isa_end))
> >>>> + ;
> >>>> + if (*isa_end != 'p')
> >>>> + break;
> >>>> + if (!isdigit(*++isa_end)) {
> >>>> + --isa_end;
> >>>> + break;
> >>>> + }
> >>>> + while (isdigit(*++isa_end))
> >>>> + ;
> >>>> + break;
> >>>> + }
> >>>> + if (*isa_end != '_')
> >>>> + --isa_end;
> >>>> + if (parse_break)
> >>>> + break;
> >>>> + }
> >>>> + }
> >>>>
> >>>> - /* Print only the base ISA as it is */
> >>>> + /**
> >>>> + * Print only the base ISA
> >>>> + * (original ISA string as is if an error is encountered)
> >>>> + */
> >>>> seq_puts(f, "isa\t\t: ");
> >>>> - seq_write(f, isa, base_isa_len);
> >>>> + if (parse_break < 2) {
> >>>> + seq_write(f, base_isa, base_isa_end - base_isa);
> >>>> + } else {
> >>>> + isa_end += strlen(isa_end);
> >>>> + seq_write(f, isa, isa_end - isa);
> >>>> + }
> >>>> seq_puts(f, "\n");
> >>>> }
> >>>>
> >>>> --
> >>>> 2.32.0
> >>>>
> >>>
> >>>
> >
> >
> >
--
Regards,
Atish
More information about the linux-riscv
mailing list