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:
> > Currently, the KVMTOOL always creates a VM with all available
> > ISA extensions virtualized by the in-kernel KVM module.
> >
> > For better flexibility, add cpu-type command-line option using
> > which users can select one of the available CPU types for VM.
> >
> > There are two CPU types supported at the moment namely "min"
> > and "max". The "min" CPU type implies VCPU with rv[64|32]imafdc
> > ISA whereas the "max" CPU type implies VCPU with all available
> > ISA extensions.
> >
> > Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> > ---
> > riscv/aia.c | 2 +-
> > riscv/fdt.c | 220 +++++++++++++++++-----------
> > riscv/include/kvm/kvm-arch.h | 2 +
> > riscv/include/kvm/kvm-config-arch.h | 5 +
> > riscv/kvm.c | 2 +
> > 5 files changed, 143 insertions(+), 88 deletions(-)
> >
> > diff --git a/riscv/aia.c b/riscv/aia.c
> > index 21d9704..cad53d4 100644
> > --- a/riscv/aia.c
> > +++ b/riscv/aia.c
> > @@ -209,7 +209,7 @@ void aia__create(struct kvm *kvm)
> > .flags = 0,
> > };
> >
> > - if (kvm->cfg.arch.ext_disabled[KVM_RISCV_ISA_EXT_SSAIA])
> > + if (riscv__isa_extension_disabled(kvm, KVM_RISCV_ISA_EXT_SSAIA))
> > return;
> >
> > err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &aia_device);
> > diff --git a/riscv/fdt.c b/riscv/fdt.c
> > index 46efb47..4c018c8 100644
> > --- a/riscv/fdt.c
> > +++ b/riscv/fdt.c
> > @@ -13,82 +13,134 @@ struct isa_ext_info {
> > const char *name;
> > unsigned long ext_id;
> > bool multi_letter;
> > + bool min_cpu_included;
> > };
> >
> > struct isa_ext_info isa_info_arr[] = {
> > - /* single-letter */
> > - {"a", KVM_RISCV_ISA_EXT_A, false},
> > - {"c", KVM_RISCV_ISA_EXT_C, false},
> > - {"d", KVM_RISCV_ISA_EXT_D, false},
> > - {"f", KVM_RISCV_ISA_EXT_F, false},
> > - {"h", KVM_RISCV_ISA_EXT_H, false},
> > - {"i", KVM_RISCV_ISA_EXT_I, false},
> > - {"m", KVM_RISCV_ISA_EXT_M, false},
> > - {"v", KVM_RISCV_ISA_EXT_V, false},
> > + /* single-letter ordered canonically as "IEMAFDQCLBJTPVNSUHKORWXYZG" */
>
> The comment change and the tabulation should go in the previous patch.
Okay, I will update.
>
> > + {"i", KVM_RISCV_ISA_EXT_I, false, true},
> > + {"m", KVM_RISCV_ISA_EXT_M, false, true},
> > + {"a", KVM_RISCV_ISA_EXT_A, false, true},
> > + {"f", KVM_RISCV_ISA_EXT_F, false, true},
> > + {"d", KVM_RISCV_ISA_EXT_D, false, true},
> > + {"c", KVM_RISCV_ISA_EXT_C, false, true},
> > + {"v", KVM_RISCV_ISA_EXT_V, false, false},
> > + {"h", KVM_RISCV_ISA_EXT_H, false, false},
>
> To avoid keeping track of which boolean means what (and taking my .misa
> suggestion from the last patch), we can write these as, e.g.
>
> {"c", KVM_RISCV_ISA_EXT_C, .misa = true, .min_enabled = true },
> {"v", KVM_RISCV_ISA_EXT_V, .misa = true, },
>
> (Also renamed min_cpu_included to min_enabled since it better matches
> the enabled/disabled concept we use everywhere else.)
>
> And we don't need to change any of the multi-letter extension entries
> since we're adding something false for them.
Okay, I will update.
>
> > /* multi-letter sorted alphabetically */
> > - {"smnpm", KVM_RISCV_ISA_EXT_SMNPM, true},
> ...
> > };
> >
> > +static bool __isa_ext_disabled(struct kvm *kvm, struct isa_ext_info *info)
> > +{
> > + if (!strncmp(kvm->cfg.arch.cpu_type, "min", 3) &&
>
> kvm->cfg.arch.cpu_type can never be anything other than NULL, "min",
> "max", so we can just use strcmp.
Okay, I will update.
>
> > + !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.
>
> > +
> > + return kvm->cfg.arch.ext_disabled[info->ext_id];
> > +}
> > +
> > +static bool __isa_ext_warn_disable_failure(struct kvm *kvm, struct isa_ext_info *info)
> > +{
> > + if (!strncmp(kvm->cfg.arch.cpu_type, "min", 3) &&
>
> same strcmp comment
Okay, I will update.
>
> > + !info->min_cpu_included)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> > +{
> > + struct isa_ext_info *info = NULL;
> > + unsigned long i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(isa_info_arr); i++) {
> > + if (isa_info_arr[i].ext_id == isa_ext_id) {
> > + info = &isa_info_arr[i];
> > + break;
> > + }
> > + }
> > + if (!info)
> > + return true;
> > +
> > + return __isa_ext_disabled(kvm, info);
> > +}
> > +
> > +int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset)
> > +{
> > + struct kvm *kvm = opt->ptr;
> > +
> > + if ((strncmp(arg, "min", 3) && strncmp(arg, "max", 3)) || strlen(arg) != 3)
> > + die("Invalid CPU type %s\n", arg);
> > +
> > + if (!strncmp(arg, "max", 3))
> > + kvm->cfg.arch.cpu_type = "max";
> > +
> > + if (!strncmp(arg, "min", 3))
>
> We can use strcmp for these two checks since we already checked strlen
> above. We also already know it's either 'min' or 'max' so we can just
> check one and default to the other.
Okay, I will update.
>
> > + kvm->cfg.arch.cpu_type = "min";
> > +
> > + return 0;
> > +}
> > +
> > static void dump_fdt(const char *dtb_file, void *fdt)
> > {
> > int count, fd;
> > @@ -108,10 +160,8 @@ static void dump_fdt(const char *dtb_file, void *fdt)
> > #define CPU_NAME_MAX_LEN 15
> > static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
> > {
> > - int cpu, pos, i, index, valid_isa_len;
> > - const char *valid_isa_order = "IEMAFDQCLBJTPVNSUHKORWXYZG";
> > - int arr_sz = ARRAY_SIZE(isa_info_arr);
> > unsigned long cbom_blksz = 0, cboz_blksz = 0, satp_mode = 0;
> > + int i, cpu, pos, arr_sz = ARRAY_SIZE(isa_info_arr);
> >
> > _FDT(fdt_begin_node(fdt, "cpus"));
> > _FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
> > @@ -131,18 +181,8 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
> >
> > snprintf(cpu_isa, CPU_ISA_MAX_LEN, "rv%ld", vcpu->riscv_xlen);
> > pos = strlen(cpu_isa);
> > - valid_isa_len = strlen(valid_isa_order);
> > - for (i = 0; i < valid_isa_len; i++) {
> > - index = valid_isa_order[i] - 'A';
> > - if (vcpu->riscv_isa & (1 << (index)))
> > - cpu_isa[pos++] = 'a' + index;
> > - }
> >
> > for (i = 0; i < arr_sz; i++) {
> > - /* Skip single-letter extensions since these are taken care */
> > - if (!isa_info_arr[i].multi_letter)
> > - continue;
> > -
> > reg.id = RISCV_ISA_EXT_REG(isa_info_arr[i].ext_id);
> > reg.addr = (unsigned long)&isa_ext_out;
> > if (ioctl(vcpu->vcpu_fd, KVM_GET_ONE_REG, ®) < 0)
> > @@ -151,9 +191,10 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
> > /* This extension is not available in hardware */
> > continue;
> >
> > - if (kvm->cfg.arch.ext_disabled[isa_info_arr[i].ext_id]) {
> > + if (__isa_ext_disabled(kvm, &isa_info_arr[i])) {
> > isa_ext_out = 0;
> > - if (ioctl(vcpu->vcpu_fd, KVM_SET_ONE_REG, ®) < 0)
> > + if ((ioctl(vcpu->vcpu_fd, KVM_SET_ONE_REG, ®) < 0) &&
>
> Unnecessary () around the first expression.
Okay, I will update.
>
> > + __isa_ext_warn_disable_failure(kvm, &isa_info_arr[i]))
> > pr_warning("Failed to disable %s ISA exension\n",
> > isa_info_arr[i].name);
> > continue;
> > @@ -178,8 +219,13 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
> > isa_info_arr[i].name);
> > break;
> > }
> > - pos += snprintf(cpu_isa + pos, CPU_ISA_MAX_LEN - pos, "_%s",
> > - isa_info_arr[i].name);
> > +
> > + if (isa_info_arr[i].multi_letter)
> > + pos += snprintf(cpu_isa + pos, CPU_ISA_MAX_LEN - pos, "_%s",
> > + isa_info_arr[i].name);
> > + else
> > + pos += snprintf(cpu_isa + pos, CPU_ISA_MAX_LEN - pos, "%s",
> > + isa_info_arr[i].name);
>
> Can write this as
Okay, I will update.
>
> pos += snprintf(cpu_isa + pos, CPU_ISA_MAX_LEN - pos, "%s%s",
> isa_info_arr[i].misa ? "" : "_",
> isa_info_arr[i].name);
>
> > }
> > cpu_isa[pos] = '\0';
> >
> > diff --git a/riscv/include/kvm/kvm-arch.h b/riscv/include/kvm/kvm-arch.h
> > index f0f469f..1bb2d32 100644
> > --- a/riscv/include/kvm/kvm-arch.h
> > +++ b/riscv/include/kvm/kvm-arch.h
> > @@ -90,6 +90,8 @@ enum irqchip_type {
> > IRQCHIP_AIA
> > };
> >
> > +bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long ext_id);
> > +
> > extern enum irqchip_type riscv_irqchip;
> > extern bool riscv_irqchip_inkernel;
> > extern void (*riscv_irqchip_trigger)(struct kvm *kvm, int irq,
> > diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
> > index 7e54d8a..26b1b50 100644
> > --- a/riscv/include/kvm/kvm-config-arch.h
> > +++ b/riscv/include/kvm/kvm-config-arch.h
> > @@ -4,6 +4,7 @@
> > #include "kvm/parse-options.h"
> >
> > struct kvm_config_arch {
> > + const char *cpu_type;
> > const char *dump_dtb_filename;
> > u64 suspend_seconds;
> > u64 custom_mvendorid;
> > @@ -13,8 +14,12 @@ struct kvm_config_arch {
> > 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.
We certainly don't need opt->value so we can pass NULL for this.
>
> > OPT_STRING('\0', "dump-dtb", &(cfg)->dump_dtb_filename, \
> > ".dtb file", "Dump generated .dtb to specified file"),\
> > OPT_U64('\0', "suspend-seconds", \
> > 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.
Although, I do agree that the function name of
kvm__arch_validate_cfg() is misleading.
Maybe kvm__arch_validate_and_init_cfg() is a better name ?
Regards,
Anup
