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

[Atish Patra]

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.

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