[kvmtool PATCH 09/10] riscv: Add cpu-type command-line option
Andrew Jones
ajones at ventanamicro.com
Sat Apr 12 06:15:10 PDT 2025
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 at ventanamicro.com>
> ---
> 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.
> + {"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.
> /* 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.
> + !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].
> +
> + 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
> + !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.
> + 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.
> + __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
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.
> 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.
Thanks,
drew
>
> bool kvm__arch_cpu_supports_vm(void)
> --
> 2.43.0
>
More information about the kvm-riscv
mailing list