On Thu, Apr 24, 2025 at 6:59 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote:
>
> 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
Yes, adding "xyz" extension to "min" cpu-type as comma separated
value and then disabling using --disable-xyz looks silly but the intention
here is that --disable-xyz options should work min cpu-type as long as
xyz is included in min cpu-type.
This will help in future if we decide to add "min_v2" or "min++"
cpu-type.
>
> 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
This is also a possible way of using this but this was not my intention.
>
> > > > 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.
>
Good suggestion, let me try.
Regards,
Anup
