[kvmtool PATCH 09/10] riscv: Add cpu-type command-line option

[Andrew Jones]

On Thu, Apr 24, 2025 at 06:27:35PM +0530, Anup Patel wrote:
> On Sat, Apr 12, 2025 at 6:45 PM Andrew Jones <ajones at ventanamicro.com> wrote:
> >
> > On Wed, Mar 26, 2025 at 12:26:43PM +0530, Anup Patel wrote:
...
> > > +         !info->min_cpu_included)
> > > +             return true;
> >
> > If 'min_cpu_included' (or 'min_enabled') is all we plan to check for
> > whether or not an extension is enabled for the 'min' cpu type, then
> > we should write this as
> >
> >  if (!strcmp(kvm->cfg.arch.cpu_type, "min"))
> >     return !info->min_enabled;
> >
> > Otherwise when min_enabled is true we'd still check
> > kvm->cfg.arch.ext_disabled[info->ext_id].
> 
> The extensions part of "min" cpu_type can be disabled using
> "--disable-xyz" command-line options hence the current approach.
> 

Shouldn't we just have a single place to check? Otherwise something like
this may not work the way the user expects

  -cpu min,xyz --disable-xyz

Something like that makes sense if you have a runkvm script like this

  #!/bin/bash
  BASE_CPU=min,xyz
  lkvm ... -cpu $BASE_CPU $@

and then you invoke it with

  runkvm --disable-xyz

> > >       bool            sbi_ext_disabled[KVM_RISCV_SBI_EXT_MAX];
> > >  };
> > >
> > > +int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset);
> > > +
> > >  #define OPT_ARCH_RUN(pfx, cfg)                                               \
> > >       pfx,                                                            \
> > > +     OPT_CALLBACK('\0', "cpu-type", kvm, "min or max",               \
> > > +                  "Choose the cpu type (default is max).", riscv__cpu_type_parser, kvm),\
> >
> > I had to look ahead at the next patch to understand why we're setting kvm
> > as the opt pointer here. I think it should be added in the next patch
> > where it's used. Also, we don't use opt->value so we cna set that to NULL.
> 
> We are indeed using opt->ptr in this patch so we should be passing
> kvm as opt-ptr.

Oh yeah, I see that now.

> > > diff --git a/riscv/kvm.c b/riscv/kvm.c
> > > index 1d49479..6a1b154 100644
> > > --- a/riscv/kvm.c
> > > +++ b/riscv/kvm.c
> > > @@ -20,6 +20,8 @@ u64 kvm__arch_default_ram_address(void)
> > >
> > >  void kvm__arch_validate_cfg(struct kvm *kvm)
> > >  {
> > > +     if (!kvm->cfg.arch.cpu_type)
> > > +             kvm->cfg.arch.cpu_type = "max";
> > >  }
> >
> > Hmm, seems like we're missing the right place for this. A validate
> > function shouln't be setting defaults. I think we should rename
> > kvm__arch_default_ram_address() to
> >
> >   void kvm__arch_set_defaults(struct kvm_config *cfg)
> >
> > and set cfg->ram_addr inside it. Then add the cpu_type default
> > setting to riscv's impl.
> 
> Renaming kvm__arch_default_ram_address() is certainly not
> the right way because it has to be done after parsing options
> so that we set the default value of cpu_type only if it is not
> set by command-line options. Due to this reason, the
> kvm__arch_validate_cfg() is the best place to set default
> value of cpu_type.

Can't we just unconditionally set kvm->cfg.arch.cpu_type to "max" in
kvm__arch_set_defaults() and then if the command line parsing determines
it should be overridden it gets reassigned?

Actually, does cpu_type need to be a string? If we use an enum for it
we could save ourselves some of the strcmp pain.

Thanks,
drew