[PATCH 2/2] RISC-V: Extract base ISA from device tree

[Tsukasa OI]

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

> 
>> 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'?

> 
>> 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
>>
> 
>