Re: [PATCH] target/riscv: Add isa extenstion strings to the device tree

[Atish Patra]

On Tue, Feb 15, 2022 at 8:20 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Dienstag, 15. Februar 2022, 10:05:30 CET schrieb Atish Patra:
> > Append the available ISA extensions to the "riscv,isa" string if it
> > is enabled so that kernel can process it.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  target/riscv/cpu.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index b0a40b83e7a8..c70260d0df15 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -34,6 +34,9 @@
> >
> >  /* RISC-V CPU definitions */
> >
> > +/* This includes the null terminated character '\0' */
> > +#define MAX_ISA_EXT_LEN 256
> > +
> >  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
> >
> >  const char * const riscv_int_regnames[] = {
> > @@ -881,10 +884,26 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> > *data)
> >      device_class_set_props(dc, riscv_cpu_properties);
> >  }
> >
> > +static void riscv_isa_string_ext(RISCVCPU *cpu, char *isa_str, int 
> > max_str_len)
> > +{
> > +    int offset = strnlen(isa_str, max_str_len);
> > +
> > +    if (cpu->cfg.ext_svpbmt) {
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", 
> > "_svpbmt");
> > +    }
> > +    if ((offset < max_str_len) && cpu->cfg.ext_svinval) {
>
> shouldn't offset + strlen("svinval") +1 be < max_str_len?
> snprintf will write partial strings but this would throw off a
> qemu client completely I guess.
>

We need that check to put out warnings to the user.

>
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", 
> > "_svinval");
> > +    }
> > +    if ((offset < max_str_len) && (cpu->cfg.ext_svnapot)) {
> > +        offset += snprintf(isa_str + offset, max_str_len, "_%s", 
> > "_svnapot");
> > +    }
>
> wouldn't it make more sense to do something like:
>
> +    struct {
> +        const char *value;
> +        bool enabled;
> +    } extensions[] = {
> +        { "svpbmt", cpu->cfg.ext_svpbmt },
> +        { "svinval", cpu->cfg.ext_svinval },
> +        { "svnapot", cpu->cfg.ext_svnapot },
> +    };
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(extensions); i++) {
> +        if (!extensions[i].enabled)
> +            continue;
> +
> +        /* check available space */
> +        if (offset + strlen(extensions[i].value) + 1 > max_str_len) {
> +            //do warn about exceeded length
> +            return;
> +        }
> +
> +        offset += snprintf(isa_str + offset, max_str_len, "_%s",
> +                                                          
> extensions[i].value);
> +    }
>
> instead?
>
> Because that list will get longer over time and repeating checks
> and snprintf calls will get harder to keep in sync over time?
>

Yeah. This is much better.

>
> Heiko
>
>
>
>
> > +}
> > +
> >  char *riscv_isa_string(RISCVCPU *cpu)
> >  {
> >      int i;
> > -    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> > +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) +
> > +                          MAX_ISA_EXT_LEN;
> >      char *isa_str = g_new(char, maxlen);
> >      char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", 
> > TARGET_LONG_BITS);
> >      for (i = 0; i < sizeof(riscv_exts); i++) {
> > @@ -893,6 +912,8 @@ char *riscv_isa_string(RISCVCPU *cpu)
> >          }
> >      }
> >      *p = '\0';
> > +    riscv_isa_string_ext(cpu, isa_str, maxlen);
> > +
> >      return isa_str;
> >  }
> >
> >
>
>
>
>
>


-- 
Regards,
Atish