[kvmtool PATCH 10/10] riscv: Allow including extensions in the min CPU type using command-line

[Anup Patel]

On Sat, Apr 12, 2025 at 7:15 PM Andrew Jones <ajones at ventanamicro.com> wrote:
>
> On Wed, Mar 26, 2025 at 12:26:44PM +0530, Anup Patel wrote:
> > It is useful to allow including extensions in the min CPU type on need
> > basis via command-line. To achieve this, parse extension names as comma
> > separated values appended to the "min" CPU type using command-line.
> >
> > For example, to include Sstc and Ssaia in the min CPU type use
> > "--cpu-type min,sstc,ssaia" command-line option.
> >
> > Signed-off-by: Anup Patel <apatel at ventanamicro.com>
> > ---
> >  riscv/fdt.c | 41 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/riscv/fdt.c b/riscv/fdt.c
> > index 4c018c8..9cefd2f 100644
> > --- a/riscv/fdt.c
> > +++ b/riscv/fdt.c
> > @@ -108,6 +108,20 @@ static bool __isa_ext_warn_disable_failure(struct kvm *kvm, struct isa_ext_info
> >       return true;
> >  }
> >
> > +static void __min_cpu_include(const char *ext, size_t ext_len)
>
> s/include/enable/

Okay, I will update.

>
> > +{
> > +     struct isa_ext_info *info;
> > +     unsigned long i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(isa_info_arr); i++) {
> > +             info = &isa_info_arr[i];
> > +             if (strlen(info->name) != ext_len)
> > +                     continue;
> > +             if (!strncmp(ext, info->name, ext_len))
>
> strcmp should be fine here since we already checked length.

Well, strcmp() does not work here because the "ext" pointer
points to a non-null terminated substring of a larger comma
separated string.

>
> > +                     info->min_cpu_included = true;
> > +     }
> > +}
> > +
> >  bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> >  {
> >       struct isa_ext_info *info = NULL;
> > @@ -128,16 +142,39 @@ bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> >  int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset)
> >  {
> >       struct kvm *kvm = opt->ptr;
> > +     const char *str, *nstr;
> > +     int len;
> >
> > -     if ((strncmp(arg, "min", 3) && strncmp(arg, "max", 3)) || strlen(arg) != 3)
> > +     if ((strncmp(arg, "min", 3) || strlen(arg) < 3) &&
>
> If arg == 'min', then it can't be less than 3 so the '|| strlen(arg) < 3'
> is dead code.
>
> > +         (strncmp(arg, "max", 3) || strlen(arg) != 3))
>
> I think we want
>
>  if (strlen(arg) < 3 ||
>      (strlen(arg) == 3 && strcmp(arg, "min") && strcmp(arg, "max")) ||
>      strncmp(arg, "min", 3))

Nope, for cpu-type = "min" the strlen(arg) can be greater than 3
because of comma separated extensions provided as part of
cpu-type value.

>
> >               die("Invalid CPU type %s\n", arg);
> >
> >       if (!strncmp(arg, "max", 3))
> >               kvm->cfg.arch.cpu_type = "max";
> >
> > -     if (!strncmp(arg, "min", 3))
> > +     if (!strncmp(arg, "min", 3)) {
> >               kvm->cfg.arch.cpu_type = "min";
> >
> > +             str = arg;
> > +             str += 3;
> > +             while (*str) {
> > +                     if (*str == ',') {
> > +                             str++;
> > +                             continue;
> > +                     }
> > +
> > +                     nstr = strchr(str, ',');
> > +                     if (!nstr)
> > +                             nstr = str + strlen(str);
> > +
> > +                     len = nstr - str;
> > +                     if (len) {
>
> I think len will always be nonzero since *str isn't \0 and we ate all
> consecutive ,'s above. __min_cpu_include() is also safe to call with
> len=0, so we could drop this check.

No point in unnecessarily calling __min_cpu_include() for
len == 0. I am sure this extra check won't hurt so I think
we should keep it.

>
> > +                             __min_cpu_include(str, len);
> > +                             str += len;
> > +                     }
> > +             }
> > +     }
> > +
> >       return 0;
> >  }
> >
> > --
> > 2.43.0
> >
>
> Thanks,
> drew

Regards,
Anup