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