[kvmtool PATCH 00/10] Add SBI system suspend and cpu-type option

[kvmtool PATCH 00/10] Add SBI system suspend and cpu-type option

[Anup Patel]

This series does the following improvements:
1) Add Svvptc, Zabha, and Ziccrse extension support (PATCH2 to PATCH3)
2) Add SBI system suspend support (PATCH5 to PATCH6)
3) Add "--cpu-type" command-line option supporting "min" and "max"
   CPU types where "max" is the default (PATCH8 to PATCH10)

These patches can also be found in the riscv_more_exts_round6_v1 branch
at: https://github.com/avpatel/kvmtool.git

Andrew Jones (3):
  riscv: Add SBI system suspend support
  riscv: Make system suspend time configurable
  riscv: Fix no params with nodefault segfault

Anup Patel (7):
  Sync-up headers with Linux-6.14 kernel
  riscv: Add Svvptc extension support
  riscv: Add Zabha extension support
  riscv: Add Ziccrse extension support
  riscv: Include single-letter extensions in isa_info_arr[]
  riscv: Add cpu-type command-line option
  riscv: Allow including extensions in the min CPU type using
    command-line

 arm/aarch64/include/asm/kvm.h       |   3 -
 include/linux/kvm.h                 |   8 +-
 include/linux/virtio_pci.h          |  14 ++
 riscv/aia.c                         |   2 +-
 riscv/fdt.c                         | 247 ++++++++++++++++++++--------
 riscv/include/asm/kvm.h             |   7 +-
 riscv/include/kvm/kvm-arch.h        |   2 +
 riscv/include/kvm/kvm-config-arch.h |  21 +++
 riscv/include/kvm/sbi.h             |   9 +
 riscv/kvm-cpu.c                     |  36 ++++
 riscv/kvm.c                         |   2 +
 x86/include/asm/kvm.h               |   1 +
 12 files changed, 268 insertions(+), 84 deletions(-)

-- 
2.43.0

[kvmtool PATCH 01/10] Sync-up headers with Linux-6.14 kernel

[Anup Patel]

We sync-up Linux headers to get latest KVM RISC-V headers having
newly added ISA extensions in ONE_REG interface.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 arm/aarch64/include/asm/kvm.h |  3 ---
 include/linux/kvm.h           |  8 ++++----
 include/linux/virtio_pci.h    | 14 ++++++++++++++
 riscv/include/asm/kvm.h       |  7 ++++---
 x86/include/asm/kvm.h         |  1 +
 5 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/arm/aarch64/include/asm/kvm.h b/arm/aarch64/include/asm/kvm.h
index 66736ff..568bf85 100644
--- a/arm/aarch64/include/asm/kvm.h
+++ b/arm/aarch64/include/asm/kvm.h
@@ -43,9 +43,6 @@
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 #define KVM_DIRTY_LOG_PAGE_OFFSET 64
 
-#define KVM_REG_SIZE(id)						\
-	(1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
-
 struct kvm_regs {
 	struct user_pt_regs regs;	/* sp = sp_el0 */
 
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 502ea63..45e6d8f 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -617,10 +617,6 @@ struct kvm_ioeventfd {
 #define KVM_X86_DISABLE_EXITS_HLT            (1 << 1)
 #define KVM_X86_DISABLE_EXITS_PAUSE          (1 << 2)
 #define KVM_X86_DISABLE_EXITS_CSTATE         (1 << 3)
-#define KVM_X86_DISABLE_VALID_EXITS          (KVM_X86_DISABLE_EXITS_MWAIT | \
-                                              KVM_X86_DISABLE_EXITS_HLT | \
-                                              KVM_X86_DISABLE_EXITS_PAUSE | \
-                                              KVM_X86_DISABLE_EXITS_CSTATE)
 
 /* for KVM_ENABLE_CAP */
 struct kvm_enable_cap {
@@ -1070,6 +1066,10 @@ struct kvm_dirty_tlb {
 
 #define KVM_REG_SIZE_SHIFT	52
 #define KVM_REG_SIZE_MASK	0x00f0000000000000ULL
+
+#define KVM_REG_SIZE(id)		\
+	(1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
+
 #define KVM_REG_SIZE_U8		0x0000000000000000ULL
 #define KVM_REG_SIZE_U16	0x0010000000000000ULL
 #define KVM_REG_SIZE_U32	0x0020000000000000ULL
diff --git a/include/linux/virtio_pci.h b/include/linux/virtio_pci.h
index 1beb317..8549d45 100644
--- a/include/linux/virtio_pci.h
+++ b/include/linux/virtio_pci.h
@@ -116,6 +116,8 @@
 #define VIRTIO_PCI_CAP_PCI_CFG		5
 /* Additional shared memory capability */
 #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
+/* PCI vendor data configuration */
+#define VIRTIO_PCI_CAP_VENDOR_CFG	9
 
 /* This is the PCI capability header: */
 struct virtio_pci_cap {
@@ -130,6 +132,18 @@ struct virtio_pci_cap {
 	__le32 length;		/* Length of the structure, in bytes. */
 };
 
+/* This is the PCI vendor data capability header: */
+struct virtio_pci_vndr_data {
+	__u8 cap_vndr;		/* Generic PCI field: PCI_CAP_ID_VNDR */
+	__u8 cap_next;		/* Generic PCI field: next ptr. */
+	__u8 cap_len;		/* Generic PCI field: capability length */
+	__u8 cfg_type;		/* Identifies the structure. */
+	__u16 vendor_id;	/* Identifies the vendor-specific format. */
+	/* For Vendor Definition */
+	/* Pads structure to a multiple of 4 bytes */
+	/* Reads must not have side effects */
+};
+
 struct virtio_pci_cap64 {
 	struct virtio_pci_cap cap;
 	__le32 offset_hi;             /* Most sig 32 bits of offset */
diff --git a/riscv/include/asm/kvm.h b/riscv/include/asm/kvm.h
index 3482c9a..f06bc5e 100644
--- a/riscv/include/asm/kvm.h
+++ b/riscv/include/asm/kvm.h
@@ -179,6 +179,9 @@ enum KVM_RISCV_ISA_EXT_ID {
 	KVM_RISCV_ISA_EXT_SSNPM,
 	KVM_RISCV_ISA_EXT_SVADE,
 	KVM_RISCV_ISA_EXT_SVADU,
+	KVM_RISCV_ISA_EXT_SVVPTC,
+	KVM_RISCV_ISA_EXT_ZABHA,
+	KVM_RISCV_ISA_EXT_ZICCRSE,
 	KVM_RISCV_ISA_EXT_MAX,
 };
 
@@ -198,6 +201,7 @@ enum KVM_RISCV_SBI_EXT_ID {
 	KVM_RISCV_SBI_EXT_VENDOR,
 	KVM_RISCV_SBI_EXT_DBCN,
 	KVM_RISCV_SBI_EXT_STA,
+	KVM_RISCV_SBI_EXT_SUSP,
 	KVM_RISCV_SBI_EXT_MAX,
 };
 
@@ -211,9 +215,6 @@ struct kvm_riscv_sbi_sta {
 #define KVM_RISCV_TIMER_STATE_OFF	0
 #define KVM_RISCV_TIMER_STATE_ON	1
 
-#define KVM_REG_SIZE(id)		\
-	(1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
-
 /* If you need to interpret the index values, here is the key: */
 #define KVM_REG_RISCV_TYPE_MASK		0x00000000FF000000
 #define KVM_REG_RISCV_TYPE_SHIFT	24
diff --git a/x86/include/asm/kvm.h b/x86/include/asm/kvm.h
index 88585c1..9e75da9 100644
--- a/x86/include/asm/kvm.h
+++ b/x86/include/asm/kvm.h
@@ -925,5 +925,6 @@ struct kvm_hyperv_eventfd {
 #define KVM_X86_SEV_VM		2
 #define KVM_X86_SEV_ES_VM	3
 #define KVM_X86_SNP_VM		4
+#define KVM_X86_TDX_VM		5
 
 #endif /* _ASM_X86_KVM_H */
-- 
2.43.0

[kvmtool PATCH 02/10] riscv: Add Svvptc extension support

[Anup Patel]

When the Svvptc extension is available expose it to the guest
via device tree so that guest can use it.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 riscv/fdt.c                         | 1 +
 riscv/include/kvm/kvm-config-arch.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/riscv/fdt.c b/riscv/fdt.c
index 03113cc..c1e688d 100644
--- a/riscv/fdt.c
+++ b/riscv/fdt.c
@@ -27,6 +27,7 @@ struct isa_ext_info isa_info_arr[] = {
 	{"svinval", KVM_RISCV_ISA_EXT_SVINVAL},
 	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT},
 	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT},
+	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC},
 	{"zacas", KVM_RISCV_ISA_EXT_ZACAS},
 	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS},
 	{"zba", KVM_RISCV_ISA_EXT_ZBA},
diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
index e56610b..ae01e14 100644
--- a/riscv/include/kvm/kvm-config-arch.h
+++ b/riscv/include/kvm/kvm-config-arch.h
@@ -58,6 +58,9 @@ struct kvm_config_arch {
 	OPT_BOOLEAN('\0', "disable-svpbmt",				\
 		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_SVPBMT],	\
 		    "Disable Svpbmt Extension"),			\
+	OPT_BOOLEAN('\0', "disable-svvptc",				\
+		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_SVVPTC],	\
+		    "Disable Svvptc Extension"),			\
 	OPT_BOOLEAN('\0', "disable-zacas",				\
 		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZACAS],	\
 		    "Disable Zacas Extension"),				\
-- 
2.43.0

Re: [kvmtool PATCH 02/10] riscv: Add Svvptc extension support

[Andrew Jones]

On Wed, Mar 26, 2025 at 12:26:36PM +0530, Anup Patel wrote:
> When the Svvptc extension is available expose it to the guest
> via device tree so that guest can use it.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  riscv/fdt.c                         | 1 +
>  riscv/include/kvm/kvm-config-arch.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/riscv/fdt.c b/riscv/fdt.c
> index 03113cc..c1e688d 100644
> --- a/riscv/fdt.c
> +++ b/riscv/fdt.c
> @@ -27,6 +27,7 @@ struct isa_ext_info isa_info_arr[] = {
>  	{"svinval", KVM_RISCV_ISA_EXT_SVINVAL},
>  	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT},
>  	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT},
> +	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC},
>  	{"zacas", KVM_RISCV_ISA_EXT_ZACAS},
>  	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS},
>  	{"zba", KVM_RISCV_ISA_EXT_ZBA},
> diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
> index e56610b..ae01e14 100644
> --- a/riscv/include/kvm/kvm-config-arch.h
> +++ b/riscv/include/kvm/kvm-config-arch.h
> @@ -58,6 +58,9 @@ struct kvm_config_arch {
>  	OPT_BOOLEAN('\0', "disable-svpbmt",				\
>  		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_SVPBMT],	\
>  		    "Disable Svpbmt Extension"),			\
> +	OPT_BOOLEAN('\0', "disable-svvptc",				\
> +		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_SVVPTC],	\
> +		    "Disable Svvptc Extension"),			\
>  	OPT_BOOLEAN('\0', "disable-zacas",				\
>  		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZACAS],	\
>  		    "Disable Zacas Extension"),				\
> -- 
> 2.43.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[kvmtool PATCH 03/10] riscv: Add Zabha extension support

[Anup Patel]

When the Zabha extension is available expose it to the guest
via device tree so that guest can use it.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 riscv/fdt.c                         | 1 +
 riscv/include/kvm/kvm-config-arch.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/riscv/fdt.c b/riscv/fdt.c
index c1e688d..ddd0b28 100644
--- a/riscv/fdt.c
+++ b/riscv/fdt.c
@@ -28,6 +28,7 @@ struct isa_ext_info isa_info_arr[] = {
 	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT},
 	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT},
 	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC},
+	{"zabha", KVM_RISCV_ISA_EXT_ZABHA},
 	{"zacas", KVM_RISCV_ISA_EXT_ZACAS},
 	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS},
 	{"zba", KVM_RISCV_ISA_EXT_ZBA},
diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
index ae01e14..d86158d 100644
--- a/riscv/include/kvm/kvm-config-arch.h
+++ b/riscv/include/kvm/kvm-config-arch.h
@@ -61,6 +61,9 @@ struct kvm_config_arch {
 	OPT_BOOLEAN('\0', "disable-svvptc",				\
 		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_SVVPTC],	\
 		    "Disable Svvptc Extension"),			\
+	OPT_BOOLEAN('\0', "disable-zabha",				\
+		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZABHA],	\
+		    "Disable Zabha Extension"),				\
 	OPT_BOOLEAN('\0', "disable-zacas",				\
 		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZACAS],	\
 		    "Disable Zacas Extension"),				\
-- 
2.43.0

Re: [kvmtool PATCH 03/10] riscv: Add Zabha extension support

[Andrew Jones]

On Wed, Mar 26, 2025 at 12:26:37PM +0530, Anup Patel wrote:
> When the Zabha extension is available expose it to the guest
> via device tree so that guest can use it.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  riscv/fdt.c                         | 1 +
>  riscv/include/kvm/kvm-config-arch.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/riscv/fdt.c b/riscv/fdt.c
> index c1e688d..ddd0b28 100644
> --- a/riscv/fdt.c
> +++ b/riscv/fdt.c
> @@ -28,6 +28,7 @@ struct isa_ext_info isa_info_arr[] = {
>  	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT},
>  	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT},
>  	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC},
> +	{"zabha", KVM_RISCV_ISA_EXT_ZABHA},
>  	{"zacas", KVM_RISCV_ISA_EXT_ZACAS},
>  	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS},
>  	{"zba", KVM_RISCV_ISA_EXT_ZBA},
> diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
> index ae01e14..d86158d 100644
> --- a/riscv/include/kvm/kvm-config-arch.h
> +++ b/riscv/include/kvm/kvm-config-arch.h
> @@ -61,6 +61,9 @@ struct kvm_config_arch {
>  	OPT_BOOLEAN('\0', "disable-svvptc",				\
>  		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_SVVPTC],	\
>  		    "Disable Svvptc Extension"),			\
> +	OPT_BOOLEAN('\0', "disable-zabha",				\
> +		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZABHA],	\
> +		    "Disable Zabha Extension"),				\
>  	OPT_BOOLEAN('\0', "disable-zacas",				\
>  		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZACAS],	\
>  		    "Disable Zacas Extension"),				\
> -- 
> 2.43.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[kvmtool PATCH 04/10] riscv: Add Ziccrse extension support

[Anup Patel]

When the Ziccrse extension is available expose it to the guest
via device tree so that guest can use it.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 riscv/fdt.c                         | 1 +
 riscv/include/kvm/kvm-config-arch.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/riscv/fdt.c b/riscv/fdt.c
index ddd0b28..3ee20a9 100644
--- a/riscv/fdt.c
+++ b/riscv/fdt.c
@@ -48,6 +48,7 @@ struct isa_ext_info isa_info_arr[] = {
 	{"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN},
 	{"zicbom", KVM_RISCV_ISA_EXT_ZICBOM},
 	{"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ},
+	{"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE},
 	{"zicntr", KVM_RISCV_ISA_EXT_ZICNTR},
 	{"zicond", KVM_RISCV_ISA_EXT_ZICOND},
 	{"zicsr", KVM_RISCV_ISA_EXT_ZICSR},
diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
index d86158d..5badb74 100644
--- a/riscv/include/kvm/kvm-config-arch.h
+++ b/riscv/include/kvm/kvm-config-arch.h
@@ -121,6 +121,9 @@ struct kvm_config_arch {
 	OPT_BOOLEAN('\0', "disable-zicboz",				\
 		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZICBOZ],	\
 		    "Disable Zicboz Extension"),			\
+	OPT_BOOLEAN('\0', "disable-ziccrse",				\
+		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZICCRSE],	\
+		    "Disable Ziccrse Extension"),			\
 	OPT_BOOLEAN('\0', "disable-zicntr",				\
 		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZICNTR],	\
 		    "Disable Zicntr Extension"),			\
-- 
2.43.0

Re: [kvmtool PATCH 04/10] riscv: Add Ziccrse extension support

[Andrew Jones]

On Wed, Mar 26, 2025 at 12:26:38PM +0530, Anup Patel wrote:
> When the Ziccrse extension is available expose it to the guest
> via device tree so that guest can use it.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  riscv/fdt.c                         | 1 +
>  riscv/include/kvm/kvm-config-arch.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/riscv/fdt.c b/riscv/fdt.c
> index ddd0b28..3ee20a9 100644
> --- a/riscv/fdt.c
> +++ b/riscv/fdt.c
> @@ -48,6 +48,7 @@ struct isa_ext_info isa_info_arr[] = {
>  	{"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN},
>  	{"zicbom", KVM_RISCV_ISA_EXT_ZICBOM},
>  	{"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ},
> +	{"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE},
>  	{"zicntr", KVM_RISCV_ISA_EXT_ZICNTR},
>  	{"zicond", KVM_RISCV_ISA_EXT_ZICOND},
>  	{"zicsr", KVM_RISCV_ISA_EXT_ZICSR},
> diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
> index d86158d..5badb74 100644
> --- a/riscv/include/kvm/kvm-config-arch.h
> +++ b/riscv/include/kvm/kvm-config-arch.h
> @@ -121,6 +121,9 @@ struct kvm_config_arch {
>  	OPT_BOOLEAN('\0', "disable-zicboz",				\
>  		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZICBOZ],	\
>  		    "Disable Zicboz Extension"),			\
> +	OPT_BOOLEAN('\0', "disable-ziccrse",				\
> +		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZICCRSE],	\
> +		    "Disable Ziccrse Extension"),			\
>  	OPT_BOOLEAN('\0', "disable-zicntr",				\
>  		    &(cfg)->ext_disabled[KVM_RISCV_ISA_EXT_ZICNTR],	\
>  		    "Disable Zicntr Extension"),			\
> -- 
> 2.43.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[kvmtool PATCH 05/10] riscv: Add SBI system suspend support

[Anup Patel]

From: Andrew Jones <ajones@ventanamicro.com>

Provide a handler for SBI system suspend requests forwarded to the
VMM by KVM and enable system suspend by default. This is just for
testing purposes, so the handler only sleeps 5 seconds before
resuming the guest. Resuming a system suspend only requires running
the system suspend invoking hart again, everything else is handled by
KVM.

Note, resuming after a suspend doesn't work with 9p used for the
guest's rootfs. Testing with a disk works though, e.g.

  lkvm-static run --nodefaults -m 256 -c 2 -d rootfs.ext2 -k Image \
    -p 'console=ttyS0 rootwait root=/dev/vda ro no_console_suspend'

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 riscv/include/kvm/kvm-config-arch.h |  3 +++
 riscv/include/kvm/sbi.h             |  9 ++++++++
 riscv/kvm-cpu.c                     | 36 +++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
index 5badb74..0553004 100644
--- a/riscv/include/kvm/kvm-config-arch.h
+++ b/riscv/include/kvm/kvm-config-arch.h
@@ -238,6 +238,9 @@ struct kvm_config_arch {
 	OPT_BOOLEAN('\0', "disable-sbi-dbcn",				\
 		    &(cfg)->sbi_ext_disabled[KVM_RISCV_SBI_EXT_DBCN],	\
 		    "Disable SBI DBCN Extension"),			\
+	OPT_BOOLEAN('\0', "disable-sbi-susp",				\
+		    &(cfg)->sbi_ext_disabled[KVM_RISCV_SBI_EXT_SUSP],	\
+		    "Disable SBI SUSP Extension"),			\
 	OPT_BOOLEAN('\0', "disable-sbi-sta",				\
 		    &(cfg)->sbi_ext_disabled[KVM_RISCV_SBI_EXT_STA],	\
 		    "Disable SBI STA Extension"),
diff --git a/riscv/include/kvm/sbi.h b/riscv/include/kvm/sbi.h
index a0f2c70..10bbe1b 100644
--- a/riscv/include/kvm/sbi.h
+++ b/riscv/include/kvm/sbi.h
@@ -21,6 +21,7 @@ enum sbi_ext_id {
 	SBI_EXT_0_1_SHUTDOWN = 0x8,
 	SBI_EXT_BASE = 0x10,
 	SBI_EXT_DBCN = 0x4442434E,
+	SBI_EXT_SUSP = 0x53555350,
 };
 
 enum sbi_ext_base_fid {
@@ -39,6 +40,14 @@ enum sbi_ext_dbcn_fid {
 	SBI_EXT_DBCN_CONSOLE_WRITE_BYTE = 2,
 };
 
+enum sbi_ext_susp_fid {
+	SBI_EXT_SUSP_SYSTEM_SUSPEND = 0,
+};
+
+enum sbi_ext_susp_sleep_type {
+	SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM = 0,
+};
+
 #define SBI_SPEC_VERSION_DEFAULT	0x1
 #define SBI_SPEC_VERSION_MAJOR_OFFSET	24
 #define SBI_SPEC_VERSION_MAJOR_MASK	0x7f
diff --git a/riscv/kvm-cpu.c b/riscv/kvm-cpu.c
index 0c171da..ad68b58 100644
--- a/riscv/kvm-cpu.c
+++ b/riscv/kvm-cpu.c
@@ -1,3 +1,5 @@
+#include <unistd.h>
+
 #include "kvm/csr.h"
 #include "kvm/kvm-cpu.h"
 #include "kvm/kvm.h"
@@ -117,6 +119,17 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 				   KVM_RISCV_SBI_EXT_DBCN);
 	}
 
+	/* Force enable SBI system suspend if not disabled from command line */
+	if (!kvm->cfg.arch.sbi_ext_disabled[KVM_RISCV_SBI_EXT_SUSP]) {
+		id = 1;
+		reg.id = RISCV_SBI_EXT_REG(KVM_REG_RISCV_SBI_SINGLE,
+					   KVM_RISCV_SBI_EXT_SUSP);
+		reg.addr = (unsigned long)&id;
+		if (ioctl(vcpu->vcpu_fd, KVM_SET_ONE_REG, &reg) < 0)
+			pr_warning("KVM_SET_ONE_REG failed (sbi_ext %d)",
+				   KVM_RISCV_SBI_EXT_SUSP);
+	}
+
 	/* Populate the vcpu structure. */
 	vcpu->kvm		= kvm;
 	vcpu->cpu_id		= cpu_id;
@@ -203,6 +216,29 @@ static bool kvm_cpu_riscv_sbi(struct kvm_cpu *vcpu)
 			break;
 		}
 		break;
+	case SBI_EXT_SUSP:
+	{
+		unsigned long susp_type, ret = SBI_SUCCESS;
+
+		switch (vcpu->kvm_run->riscv_sbi.function_id) {
+		case SBI_EXT_SUSP_SYSTEM_SUSPEND:
+			susp_type = vcpu->kvm_run->riscv_sbi.args[0];
+			if (susp_type != SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM) {
+				ret = SBI_ERR_INVALID_PARAM;
+				break;
+			}
+
+			sleep(5);
+
+			break;
+		default:
+			ret = SBI_ERR_NOT_SUPPORTED;
+			break;
+		}
+
+		vcpu->kvm_run->riscv_sbi.ret[0] = ret;
+		break;
+	}
 	default:
 		dprintf(dfd, "Unhandled SBI call\n");
 		dprintf(dfd, "extension_id=0x%lx function_id=0x%lx\n",
-- 
2.43.0

[kvmtool PATCH 06/10] riscv: Make system suspend time configurable

[Anup Patel]

From: Andrew Jones <ajones@ventanamicro.com>

If the default of 5 seconds for a system suspend test is too long or
too short, then feel free to change it.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 riscv/include/kvm/kvm-config-arch.h | 4 ++++
 riscv/kvm-cpu.c                     | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/riscv/include/kvm/kvm-config-arch.h b/riscv/include/kvm/kvm-config-arch.h
index 0553004..7e54d8a 100644
--- a/riscv/include/kvm/kvm-config-arch.h
+++ b/riscv/include/kvm/kvm-config-arch.h
@@ -5,6 +5,7 @@
 
 struct kvm_config_arch {
 	const char	*dump_dtb_filename;
+	u64		suspend_seconds;
 	u64		custom_mvendorid;
 	u64		custom_marchid;
 	u64		custom_mimpid;
@@ -16,6 +17,9 @@ struct kvm_config_arch {
 	pfx,								\
 	OPT_STRING('\0', "dump-dtb", &(cfg)->dump_dtb_filename,		\
 		   ".dtb file", "Dump generated .dtb to specified file"),\
+	OPT_U64('\0', "suspend-seconds",				\
+		&(cfg)->suspend_seconds,				\
+		"Number of seconds to suspend for system suspend (default is 5)"), \
 	OPT_U64('\0', "custom-mvendorid",				\
 		&(cfg)->custom_mvendorid,				\
 		"Show custom mvendorid to Guest VCPU"),			\
diff --git a/riscv/kvm-cpu.c b/riscv/kvm-cpu.c
index ad68b58..7a86d71 100644
--- a/riscv/kvm-cpu.c
+++ b/riscv/kvm-cpu.c
@@ -228,7 +228,7 @@ static bool kvm_cpu_riscv_sbi(struct kvm_cpu *vcpu)
 				break;
 			}
 
-			sleep(5);
+			sleep(vcpu->kvm->cfg.arch.suspend_seconds ? : 5);
 
 			break;
 		default:
-- 
2.43.0

[kvmtool PATCH 07/10] riscv: Fix no params with nodefault segfault

[Anup Patel]

From: Andrew Jones <ajones@ventanamicro.com>

Fix segfault received when using --nodefault without --params.

Fixes: 7c9aac003925 ("riscv: Generate FDT at runtime for Guest/VM")
Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Link: https://lore.kernel.org/r/20250123151339.185908-2-ajones@ventanamicro.com
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 riscv/fdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/riscv/fdt.c b/riscv/fdt.c
index 3ee20a9..251821e 100644
--- a/riscv/fdt.c
+++ b/riscv/fdt.c
@@ -263,9 +263,10 @@ static int setup_fdt(struct kvm *kvm)
 		if (kvm->cfg.kernel_cmdline)
 			_FDT(fdt_property_string(fdt, "bootargs",
 						 kvm->cfg.kernel_cmdline));
-	} else
+	} else if (kvm->cfg.real_cmdline) {
 		_FDT(fdt_property_string(fdt, "bootargs",
 					 kvm->cfg.real_cmdline));
+	}
 
 	_FDT(fdt_property_string(fdt, "stdout-path", "serial0"));
 
-- 
2.43.0

[kvmtool PATCH 08/10] riscv: Include single-letter extensions in isa_info_arr[]

[Anup Patel]

Currently, the isa_info_arr[] only include multi-letter extensions but
the KVM ONE_REG interface covers both single-letter and multi-letter
extensions so extend isa_info_arr[] to include single-letter extensions.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 riscv/fdt.c | 138 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 76 insertions(+), 62 deletions(-)

diff --git a/riscv/fdt.c b/riscv/fdt.c
index 251821e..46efb47 100644
--- a/riscv/fdt.c
+++ b/riscv/fdt.c
@@ -12,71 +12,81 @@
 struct isa_ext_info {
 	const char *name;
 	unsigned long ext_id;
+	bool multi_letter;
 };
 
 struct isa_ext_info isa_info_arr[] = {
-	/* sorted alphabetically */
-	{"smnpm", KVM_RISCV_ISA_EXT_SMNPM},
-	{"smstateen", KVM_RISCV_ISA_EXT_SMSTATEEN},
-	{"ssaia", KVM_RISCV_ISA_EXT_SSAIA},
-	{"sscofpmf", KVM_RISCV_ISA_EXT_SSCOFPMF},
-	{"ssnpm", KVM_RISCV_ISA_EXT_SSNPM},
-	{"sstc", KVM_RISCV_ISA_EXT_SSTC},
-	{"svade", KVM_RISCV_ISA_EXT_SVADE},
-	{"svadu", KVM_RISCV_ISA_EXT_SVADU},
-	{"svinval", KVM_RISCV_ISA_EXT_SVINVAL},
-	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT},
-	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT},
-	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC},
-	{"zabha", KVM_RISCV_ISA_EXT_ZABHA},
-	{"zacas", KVM_RISCV_ISA_EXT_ZACAS},
-	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS},
-	{"zba", KVM_RISCV_ISA_EXT_ZBA},
-	{"zbb", KVM_RISCV_ISA_EXT_ZBB},
-	{"zbc", KVM_RISCV_ISA_EXT_ZBC},
-	{"zbkb", KVM_RISCV_ISA_EXT_ZBKB},
-	{"zbkc", KVM_RISCV_ISA_EXT_ZBKC},
-	{"zbkx", KVM_RISCV_ISA_EXT_ZBKX},
-	{"zbs", KVM_RISCV_ISA_EXT_ZBS},
-	{"zca", KVM_RISCV_ISA_EXT_ZCA},
-	{"zcb", KVM_RISCV_ISA_EXT_ZCB},
-	{"zcd", KVM_RISCV_ISA_EXT_ZCD},
-	{"zcf", KVM_RISCV_ISA_EXT_ZCF},
-	{"zcmop", KVM_RISCV_ISA_EXT_ZCMOP},
-	{"zfa", KVM_RISCV_ISA_EXT_ZFA},
-	{"zfh", KVM_RISCV_ISA_EXT_ZFH},
-	{"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN},
-	{"zicbom", KVM_RISCV_ISA_EXT_ZICBOM},
-	{"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ},
-	{"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE},
-	{"zicntr", KVM_RISCV_ISA_EXT_ZICNTR},
-	{"zicond", KVM_RISCV_ISA_EXT_ZICOND},
-	{"zicsr", KVM_RISCV_ISA_EXT_ZICSR},
-	{"zifencei", KVM_RISCV_ISA_EXT_ZIFENCEI},
-	{"zihintntl", KVM_RISCV_ISA_EXT_ZIHINTNTL},
-	{"zihintpause", KVM_RISCV_ISA_EXT_ZIHINTPAUSE},
-	{"zihpm", KVM_RISCV_ISA_EXT_ZIHPM},
-	{"zimop", KVM_RISCV_ISA_EXT_ZIMOP},
-	{"zknd", KVM_RISCV_ISA_EXT_ZKND},
-	{"zkne", KVM_RISCV_ISA_EXT_ZKNE},
-	{"zknh", KVM_RISCV_ISA_EXT_ZKNH},
-	{"zkr", KVM_RISCV_ISA_EXT_ZKR},
-	{"zksed", KVM_RISCV_ISA_EXT_ZKSED},
-	{"zksh", KVM_RISCV_ISA_EXT_ZKSH},
-	{"zkt", KVM_RISCV_ISA_EXT_ZKT},
-	{"ztso", KVM_RISCV_ISA_EXT_ZTSO},
-	{"zvbb", KVM_RISCV_ISA_EXT_ZVBB},
-	{"zvbc", KVM_RISCV_ISA_EXT_ZVBC},
-	{"zvfh", KVM_RISCV_ISA_EXT_ZVFH},
-	{"zvfhmin", KVM_RISCV_ISA_EXT_ZVFHMIN},
-	{"zvkb", KVM_RISCV_ISA_EXT_ZVKB},
-	{"zvkg", KVM_RISCV_ISA_EXT_ZVKG},
-	{"zvkned", KVM_RISCV_ISA_EXT_ZVKNED},
-	{"zvknha", KVM_RISCV_ISA_EXT_ZVKNHA},
-	{"zvknhb", KVM_RISCV_ISA_EXT_ZVKNHB},
-	{"zvksed", KVM_RISCV_ISA_EXT_ZVKSED},
-	{"zvksh", KVM_RISCV_ISA_EXT_ZVKSH},
-	{"zvkt", KVM_RISCV_ISA_EXT_ZVKT},
+	/* 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},
+	/* multi-letter sorted alphabetically */
+	{"smnpm", KVM_RISCV_ISA_EXT_SMNPM, true},
+	{"smstateen", KVM_RISCV_ISA_EXT_SMSTATEEN, true},
+	{"ssaia", KVM_RISCV_ISA_EXT_SSAIA, true},
+	{"sscofpmf", KVM_RISCV_ISA_EXT_SSCOFPMF, true},
+	{"ssnpm", KVM_RISCV_ISA_EXT_SSNPM, true},
+	{"sstc", KVM_RISCV_ISA_EXT_SSTC, true},
+	{"svade", KVM_RISCV_ISA_EXT_SVADE, true},
+	{"svadu", KVM_RISCV_ISA_EXT_SVADU, true},
+	{"svinval", KVM_RISCV_ISA_EXT_SVINVAL, true},
+	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT, true},
+	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT, true},
+	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC, true},
+	{"zabha", KVM_RISCV_ISA_EXT_ZABHA, true},
+	{"zacas", KVM_RISCV_ISA_EXT_ZACAS, true},
+	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS, true},
+	{"zba", KVM_RISCV_ISA_EXT_ZBA, true},
+	{"zbb", KVM_RISCV_ISA_EXT_ZBB, true},
+	{"zbc", KVM_RISCV_ISA_EXT_ZBC, true},
+	{"zbkb", KVM_RISCV_ISA_EXT_ZBKB, true},
+	{"zbkc", KVM_RISCV_ISA_EXT_ZBKC, true},
+	{"zbkx", KVM_RISCV_ISA_EXT_ZBKX, true},
+	{"zbs", KVM_RISCV_ISA_EXT_ZBS, true},
+	{"zca", KVM_RISCV_ISA_EXT_ZCA, true},
+	{"zcb", KVM_RISCV_ISA_EXT_ZCB, true},
+	{"zcd", KVM_RISCV_ISA_EXT_ZCD, true},
+	{"zcf", KVM_RISCV_ISA_EXT_ZCF, true},
+	{"zcmop", KVM_RISCV_ISA_EXT_ZCMOP, true},
+	{"zfa", KVM_RISCV_ISA_EXT_ZFA, true},
+	{"zfh", KVM_RISCV_ISA_EXT_ZFH, true},
+	{"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN, true},
+	{"zicbom", KVM_RISCV_ISA_EXT_ZICBOM, true},
+	{"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ, true},
+	{"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE, true},
+	{"zicntr", KVM_RISCV_ISA_EXT_ZICNTR, true},
+	{"zicond", KVM_RISCV_ISA_EXT_ZICOND, true},
+	{"zicsr", KVM_RISCV_ISA_EXT_ZICSR, true},
+	{"zifencei", KVM_RISCV_ISA_EXT_ZIFENCEI, true},
+	{"zihintntl", KVM_RISCV_ISA_EXT_ZIHINTNTL, true},
+	{"zihintpause", KVM_RISCV_ISA_EXT_ZIHINTPAUSE, true},
+	{"zihpm", KVM_RISCV_ISA_EXT_ZIHPM, true},
+	{"zimop", KVM_RISCV_ISA_EXT_ZIMOP, true},
+	{"zknd", KVM_RISCV_ISA_EXT_ZKND, true},
+	{"zkne", KVM_RISCV_ISA_EXT_ZKNE, true},
+	{"zknh", KVM_RISCV_ISA_EXT_ZKNH, true},
+	{"zkr", KVM_RISCV_ISA_EXT_ZKR, true},
+	{"zksed", KVM_RISCV_ISA_EXT_ZKSED, true},
+	{"zksh", KVM_RISCV_ISA_EXT_ZKSH, true},
+	{"zkt", KVM_RISCV_ISA_EXT_ZKT, true},
+	{"ztso", KVM_RISCV_ISA_EXT_ZTSO, true},
+	{"zvbb", KVM_RISCV_ISA_EXT_ZVBB, true},
+	{"zvbc", KVM_RISCV_ISA_EXT_ZVBC, true},
+	{"zvfh", KVM_RISCV_ISA_EXT_ZVFH, true},
+	{"zvfhmin", KVM_RISCV_ISA_EXT_ZVFHMIN, true},
+	{"zvkb", KVM_RISCV_ISA_EXT_ZVKB, true},
+	{"zvkg", KVM_RISCV_ISA_EXT_ZVKG, true},
+	{"zvkned", KVM_RISCV_ISA_EXT_ZVKNED, true},
+	{"zvknha", KVM_RISCV_ISA_EXT_ZVKNHA, true},
+	{"zvknhb", KVM_RISCV_ISA_EXT_ZVKNHB, true},
+	{"zvksed", KVM_RISCV_ISA_EXT_ZVKSED, true},
+	{"zvksh", KVM_RISCV_ISA_EXT_ZVKSH, true},
+	{"zvkt", KVM_RISCV_ISA_EXT_ZVKT, true},
 };
 
 static void dump_fdt(const char *dtb_file, void *fdt)
@@ -129,6 +139,10 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
 		}
 
 		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, &reg) < 0)
-- 
2.43.0

Re: [kvmtool PATCH 08/10] riscv: Include single-letter extensions in isa_info_arr[]

[Andrew Jones]

On Wed, Mar 26, 2025 at 12:26:42PM +0530, Anup Patel wrote:
> Currently, the isa_info_arr[] only include multi-letter extensions but
> the KVM ONE_REG interface covers both single-letter and multi-letter
> extensions so extend isa_info_arr[] to include single-letter extensions.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  riscv/fdt.c | 138 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 76 insertions(+), 62 deletions(-)
> 
> diff --git a/riscv/fdt.c b/riscv/fdt.c
> index 251821e..46efb47 100644
> --- a/riscv/fdt.c
> +++ b/riscv/fdt.c
> @@ -12,71 +12,81 @@
>  struct isa_ext_info {
>  	const char *name;
>  	unsigned long ext_id;
> +	bool multi_letter;
>  };
>  
>  struct isa_ext_info isa_info_arr[] = {
> -	/* sorted alphabetically */
> -	{"smnpm", KVM_RISCV_ISA_EXT_SMNPM},
> -	{"smstateen", KVM_RISCV_ISA_EXT_SMSTATEEN},
> -	{"ssaia", KVM_RISCV_ISA_EXT_SSAIA},
> -	{"sscofpmf", KVM_RISCV_ISA_EXT_SSCOFPMF},
> -	{"ssnpm", KVM_RISCV_ISA_EXT_SSNPM},
> -	{"sstc", KVM_RISCV_ISA_EXT_SSTC},
> -	{"svade", KVM_RISCV_ISA_EXT_SVADE},
> -	{"svadu", KVM_RISCV_ISA_EXT_SVADU},
> -	{"svinval", KVM_RISCV_ISA_EXT_SVINVAL},
> -	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT},
> -	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT},
> -	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC},
> -	{"zabha", KVM_RISCV_ISA_EXT_ZABHA},
> -	{"zacas", KVM_RISCV_ISA_EXT_ZACAS},
> -	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS},
> -	{"zba", KVM_RISCV_ISA_EXT_ZBA},
> -	{"zbb", KVM_RISCV_ISA_EXT_ZBB},
> -	{"zbc", KVM_RISCV_ISA_EXT_ZBC},
> -	{"zbkb", KVM_RISCV_ISA_EXT_ZBKB},
> -	{"zbkc", KVM_RISCV_ISA_EXT_ZBKC},
> -	{"zbkx", KVM_RISCV_ISA_EXT_ZBKX},
> -	{"zbs", KVM_RISCV_ISA_EXT_ZBS},
> -	{"zca", KVM_RISCV_ISA_EXT_ZCA},
> -	{"zcb", KVM_RISCV_ISA_EXT_ZCB},
> -	{"zcd", KVM_RISCV_ISA_EXT_ZCD},
> -	{"zcf", KVM_RISCV_ISA_EXT_ZCF},
> -	{"zcmop", KVM_RISCV_ISA_EXT_ZCMOP},
> -	{"zfa", KVM_RISCV_ISA_EXT_ZFA},
> -	{"zfh", KVM_RISCV_ISA_EXT_ZFH},
> -	{"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN},
> -	{"zicbom", KVM_RISCV_ISA_EXT_ZICBOM},
> -	{"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ},
> -	{"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE},
> -	{"zicntr", KVM_RISCV_ISA_EXT_ZICNTR},
> -	{"zicond", KVM_RISCV_ISA_EXT_ZICOND},
> -	{"zicsr", KVM_RISCV_ISA_EXT_ZICSR},
> -	{"zifencei", KVM_RISCV_ISA_EXT_ZIFENCEI},
> -	{"zihintntl", KVM_RISCV_ISA_EXT_ZIHINTNTL},
> -	{"zihintpause", KVM_RISCV_ISA_EXT_ZIHINTPAUSE},
> -	{"zihpm", KVM_RISCV_ISA_EXT_ZIHPM},
> -	{"zimop", KVM_RISCV_ISA_EXT_ZIMOP},
> -	{"zknd", KVM_RISCV_ISA_EXT_ZKND},
> -	{"zkne", KVM_RISCV_ISA_EXT_ZKNE},
> -	{"zknh", KVM_RISCV_ISA_EXT_ZKNH},
> -	{"zkr", KVM_RISCV_ISA_EXT_ZKR},
> -	{"zksed", KVM_RISCV_ISA_EXT_ZKSED},
> -	{"zksh", KVM_RISCV_ISA_EXT_ZKSH},
> -	{"zkt", KVM_RISCV_ISA_EXT_ZKT},
> -	{"ztso", KVM_RISCV_ISA_EXT_ZTSO},
> -	{"zvbb", KVM_RISCV_ISA_EXT_ZVBB},
> -	{"zvbc", KVM_RISCV_ISA_EXT_ZVBC},
> -	{"zvfh", KVM_RISCV_ISA_EXT_ZVFH},
> -	{"zvfhmin", KVM_RISCV_ISA_EXT_ZVFHMIN},
> -	{"zvkb", KVM_RISCV_ISA_EXT_ZVKB},
> -	{"zvkg", KVM_RISCV_ISA_EXT_ZVKG},
> -	{"zvkned", KVM_RISCV_ISA_EXT_ZVKNED},
> -	{"zvknha", KVM_RISCV_ISA_EXT_ZVKNHA},
> -	{"zvknhb", KVM_RISCV_ISA_EXT_ZVKNHB},
> -	{"zvksed", KVM_RISCV_ISA_EXT_ZVKSED},
> -	{"zvksh", KVM_RISCV_ISA_EXT_ZVKSH},
> -	{"zvkt", KVM_RISCV_ISA_EXT_ZVKT},
> +	/* 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},
> +	/* multi-letter sorted alphabetically */
> +	{"smnpm", KVM_RISCV_ISA_EXT_SMNPM, true},
> +	{"smstateen", KVM_RISCV_ISA_EXT_SMSTATEEN, true},
> +	{"ssaia", KVM_RISCV_ISA_EXT_SSAIA, true},
> +	{"sscofpmf", KVM_RISCV_ISA_EXT_SSCOFPMF, true},
> +	{"ssnpm", KVM_RISCV_ISA_EXT_SSNPM, true},
> +	{"sstc", KVM_RISCV_ISA_EXT_SSTC, true},
> +	{"svade", KVM_RISCV_ISA_EXT_SVADE, true},
> +	{"svadu", KVM_RISCV_ISA_EXT_SVADU, true},
> +	{"svinval", KVM_RISCV_ISA_EXT_SVINVAL, true},
> +	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT, true},
> +	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT, true},
> +	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC, true},
> +	{"zabha", KVM_RISCV_ISA_EXT_ZABHA, true},
> +	{"zacas", KVM_RISCV_ISA_EXT_ZACAS, true},
> +	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS, true},
> +	{"zba", KVM_RISCV_ISA_EXT_ZBA, true},
> +	{"zbb", KVM_RISCV_ISA_EXT_ZBB, true},
> +	{"zbc", KVM_RISCV_ISA_EXT_ZBC, true},
> +	{"zbkb", KVM_RISCV_ISA_EXT_ZBKB, true},
> +	{"zbkc", KVM_RISCV_ISA_EXT_ZBKC, true},
> +	{"zbkx", KVM_RISCV_ISA_EXT_ZBKX, true},
> +	{"zbs", KVM_RISCV_ISA_EXT_ZBS, true},
> +	{"zca", KVM_RISCV_ISA_EXT_ZCA, true},
> +	{"zcb", KVM_RISCV_ISA_EXT_ZCB, true},
> +	{"zcd", KVM_RISCV_ISA_EXT_ZCD, true},
> +	{"zcf", KVM_RISCV_ISA_EXT_ZCF, true},
> +	{"zcmop", KVM_RISCV_ISA_EXT_ZCMOP, true},
> +	{"zfa", KVM_RISCV_ISA_EXT_ZFA, true},
> +	{"zfh", KVM_RISCV_ISA_EXT_ZFH, true},
> +	{"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN, true},
> +	{"zicbom", KVM_RISCV_ISA_EXT_ZICBOM, true},
> +	{"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ, true},
> +	{"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE, true},
> +	{"zicntr", KVM_RISCV_ISA_EXT_ZICNTR, true},
> +	{"zicond", KVM_RISCV_ISA_EXT_ZICOND, true},
> +	{"zicsr", KVM_RISCV_ISA_EXT_ZICSR, true},
> +	{"zifencei", KVM_RISCV_ISA_EXT_ZIFENCEI, true},
> +	{"zihintntl", KVM_RISCV_ISA_EXT_ZIHINTNTL, true},
> +	{"zihintpause", KVM_RISCV_ISA_EXT_ZIHINTPAUSE, true},
> +	{"zihpm", KVM_RISCV_ISA_EXT_ZIHPM, true},
> +	{"zimop", KVM_RISCV_ISA_EXT_ZIMOP, true},
> +	{"zknd", KVM_RISCV_ISA_EXT_ZKND, true},
> +	{"zkne", KVM_RISCV_ISA_EXT_ZKNE, true},
> +	{"zknh", KVM_RISCV_ISA_EXT_ZKNH, true},
> +	{"zkr", KVM_RISCV_ISA_EXT_ZKR, true},
> +	{"zksed", KVM_RISCV_ISA_EXT_ZKSED, true},
> +	{"zksh", KVM_RISCV_ISA_EXT_ZKSH, true},
> +	{"zkt", KVM_RISCV_ISA_EXT_ZKT, true},
> +	{"ztso", KVM_RISCV_ISA_EXT_ZTSO, true},
> +	{"zvbb", KVM_RISCV_ISA_EXT_ZVBB, true},
> +	{"zvbc", KVM_RISCV_ISA_EXT_ZVBC, true},
> +	{"zvfh", KVM_RISCV_ISA_EXT_ZVFH, true},
> +	{"zvfhmin", KVM_RISCV_ISA_EXT_ZVFHMIN, true},
> +	{"zvkb", KVM_RISCV_ISA_EXT_ZVKB, true},
> +	{"zvkg", KVM_RISCV_ISA_EXT_ZVKG, true},
> +	{"zvkned", KVM_RISCV_ISA_EXT_ZVKNED, true},
> +	{"zvknha", KVM_RISCV_ISA_EXT_ZVKNHA, true},
> +	{"zvknhb", KVM_RISCV_ISA_EXT_ZVKNHB, true},
> +	{"zvksed", KVM_RISCV_ISA_EXT_ZVKSED, true},
> +	{"zvksh", KVM_RISCV_ISA_EXT_ZVKSH, true},
> +	{"zvkt", KVM_RISCV_ISA_EXT_ZVKT, true},

nit: I think I would add a 'misa' boolean member instead of 'multi_letter'
and then rework this table to look like this

	{"a",		KVM_RISCV_ISA_EXT_A, .misa = true	},
	{"c",		KVM_RISCV_ISA_EXT_C, .misa = true	},
	{"d",		KVM_RISCV_ISA_EXT_D, .misa = true	},
	{"f",		KVM_RISCV_ISA_EXT_F, .misa = true	},
	{"h",		KVM_RISCV_ISA_EXT_H, .misa = true	},
	{"i",		KVM_RISCV_ISA_EXT_I, .misa = true	},
	{"m",		KVM_RISCV_ISA_EXT_M, .misa = true	},
	{"v",		KVM_RISCV_ISA_EXT_V, .misa = true	},

	/* multi-letter sorted alphabetically */
	{"smnpm",	KVM_RISCV_ISA_EXT_SMNPM,		},
	{"smstateen",	KVM_RISCV_ISA_EXT_SMSTATEEN,		},
	...

The benefit is that only the misa extensions need another field.

>  };
>  
>  static void dump_fdt(const char *dtb_file, void *fdt)
> @@ -129,6 +139,10 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
>  		}
>  
>  		for (i = 0; i < arr_sz; i++) {
> +			/* Skip single-letter extensions since these are taken care */

We should finish the comment with 'of by "whatever takes care of them"'

> +			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, &reg) < 0)
> -- 
> 2.43.0
>

Thanks,
drew

Re: [kvmtool PATCH 08/10] riscv: Include single-letter extensions in isa_info_arr[]

[Anup Patel]

On Sat, Apr 12, 2025 at 6:06 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Mar 26, 2025 at 12:26:42PM +0530, Anup Patel wrote:
> > Currently, the isa_info_arr[] only include multi-letter extensions but
> > the KVM ONE_REG interface covers both single-letter and multi-letter
> > extensions so extend isa_info_arr[] to include single-letter extensions.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  riscv/fdt.c | 138 +++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 76 insertions(+), 62 deletions(-)
> >
> > diff --git a/riscv/fdt.c b/riscv/fdt.c
> > index 251821e..46efb47 100644
> > --- a/riscv/fdt.c
> > +++ b/riscv/fdt.c
> > @@ -12,71 +12,81 @@
> >  struct isa_ext_info {
> >       const char *name;
> >       unsigned long ext_id;
> > +     bool multi_letter;
> >  };
> >
> >  struct isa_ext_info isa_info_arr[] = {
> > -     /* sorted alphabetically */
> > -     {"smnpm", KVM_RISCV_ISA_EXT_SMNPM},
> > -     {"smstateen", KVM_RISCV_ISA_EXT_SMSTATEEN},
> > -     {"ssaia", KVM_RISCV_ISA_EXT_SSAIA},
> > -     {"sscofpmf", KVM_RISCV_ISA_EXT_SSCOFPMF},
> > -     {"ssnpm", KVM_RISCV_ISA_EXT_SSNPM},
> > -     {"sstc", KVM_RISCV_ISA_EXT_SSTC},
> > -     {"svade", KVM_RISCV_ISA_EXT_SVADE},
> > -     {"svadu", KVM_RISCV_ISA_EXT_SVADU},
> > -     {"svinval", KVM_RISCV_ISA_EXT_SVINVAL},
> > -     {"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT},
> > -     {"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT},
> > -     {"svvptc", KVM_RISCV_ISA_EXT_SVVPTC},
> > -     {"zabha", KVM_RISCV_ISA_EXT_ZABHA},
> > -     {"zacas", KVM_RISCV_ISA_EXT_ZACAS},
> > -     {"zawrs", KVM_RISCV_ISA_EXT_ZAWRS},
> > -     {"zba", KVM_RISCV_ISA_EXT_ZBA},
> > -     {"zbb", KVM_RISCV_ISA_EXT_ZBB},
> > -     {"zbc", KVM_RISCV_ISA_EXT_ZBC},
> > -     {"zbkb", KVM_RISCV_ISA_EXT_ZBKB},
> > -     {"zbkc", KVM_RISCV_ISA_EXT_ZBKC},
> > -     {"zbkx", KVM_RISCV_ISA_EXT_ZBKX},
> > -     {"zbs", KVM_RISCV_ISA_EXT_ZBS},
> > -     {"zca", KVM_RISCV_ISA_EXT_ZCA},
> > -     {"zcb", KVM_RISCV_ISA_EXT_ZCB},
> > -     {"zcd", KVM_RISCV_ISA_EXT_ZCD},
> > -     {"zcf", KVM_RISCV_ISA_EXT_ZCF},
> > -     {"zcmop", KVM_RISCV_ISA_EXT_ZCMOP},
> > -     {"zfa", KVM_RISCV_ISA_EXT_ZFA},
> > -     {"zfh", KVM_RISCV_ISA_EXT_ZFH},
> > -     {"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN},
> > -     {"zicbom", KVM_RISCV_ISA_EXT_ZICBOM},
> > -     {"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ},
> > -     {"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE},
> > -     {"zicntr", KVM_RISCV_ISA_EXT_ZICNTR},
> > -     {"zicond", KVM_RISCV_ISA_EXT_ZICOND},
> > -     {"zicsr", KVM_RISCV_ISA_EXT_ZICSR},
> > -     {"zifencei", KVM_RISCV_ISA_EXT_ZIFENCEI},
> > -     {"zihintntl", KVM_RISCV_ISA_EXT_ZIHINTNTL},
> > -     {"zihintpause", KVM_RISCV_ISA_EXT_ZIHINTPAUSE},
> > -     {"zihpm", KVM_RISCV_ISA_EXT_ZIHPM},
> > -     {"zimop", KVM_RISCV_ISA_EXT_ZIMOP},
> > -     {"zknd", KVM_RISCV_ISA_EXT_ZKND},
> > -     {"zkne", KVM_RISCV_ISA_EXT_ZKNE},
> > -     {"zknh", KVM_RISCV_ISA_EXT_ZKNH},
> > -     {"zkr", KVM_RISCV_ISA_EXT_ZKR},
> > -     {"zksed", KVM_RISCV_ISA_EXT_ZKSED},
> > -     {"zksh", KVM_RISCV_ISA_EXT_ZKSH},
> > -     {"zkt", KVM_RISCV_ISA_EXT_ZKT},
> > -     {"ztso", KVM_RISCV_ISA_EXT_ZTSO},
> > -     {"zvbb", KVM_RISCV_ISA_EXT_ZVBB},
> > -     {"zvbc", KVM_RISCV_ISA_EXT_ZVBC},
> > -     {"zvfh", KVM_RISCV_ISA_EXT_ZVFH},
> > -     {"zvfhmin", KVM_RISCV_ISA_EXT_ZVFHMIN},
> > -     {"zvkb", KVM_RISCV_ISA_EXT_ZVKB},
> > -     {"zvkg", KVM_RISCV_ISA_EXT_ZVKG},
> > -     {"zvkned", KVM_RISCV_ISA_EXT_ZVKNED},
> > -     {"zvknha", KVM_RISCV_ISA_EXT_ZVKNHA},
> > -     {"zvknhb", KVM_RISCV_ISA_EXT_ZVKNHB},
> > -     {"zvksed", KVM_RISCV_ISA_EXT_ZVKSED},
> > -     {"zvksh", KVM_RISCV_ISA_EXT_ZVKSH},
> > -     {"zvkt", KVM_RISCV_ISA_EXT_ZVKT},
> > +     /* 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},
> > +     /* multi-letter sorted alphabetically */
> > +     {"smnpm", KVM_RISCV_ISA_EXT_SMNPM, true},
> > +     {"smstateen", KVM_RISCV_ISA_EXT_SMSTATEEN, true},
> > +     {"ssaia", KVM_RISCV_ISA_EXT_SSAIA, true},
> > +     {"sscofpmf", KVM_RISCV_ISA_EXT_SSCOFPMF, true},
> > +     {"ssnpm", KVM_RISCV_ISA_EXT_SSNPM, true},
> > +     {"sstc", KVM_RISCV_ISA_EXT_SSTC, true},
> > +     {"svade", KVM_RISCV_ISA_EXT_SVADE, true},
> > +     {"svadu", KVM_RISCV_ISA_EXT_SVADU, true},
> > +     {"svinval", KVM_RISCV_ISA_EXT_SVINVAL, true},
> > +     {"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT, true},
> > +     {"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT, true},
> > +     {"svvptc", KVM_RISCV_ISA_EXT_SVVPTC, true},
> > +     {"zabha", KVM_RISCV_ISA_EXT_ZABHA, true},
> > +     {"zacas", KVM_RISCV_ISA_EXT_ZACAS, true},
> > +     {"zawrs", KVM_RISCV_ISA_EXT_ZAWRS, true},
> > +     {"zba", KVM_RISCV_ISA_EXT_ZBA, true},
> > +     {"zbb", KVM_RISCV_ISA_EXT_ZBB, true},
> > +     {"zbc", KVM_RISCV_ISA_EXT_ZBC, true},
> > +     {"zbkb", KVM_RISCV_ISA_EXT_ZBKB, true},
> > +     {"zbkc", KVM_RISCV_ISA_EXT_ZBKC, true},
> > +     {"zbkx", KVM_RISCV_ISA_EXT_ZBKX, true},
> > +     {"zbs", KVM_RISCV_ISA_EXT_ZBS, true},
> > +     {"zca", KVM_RISCV_ISA_EXT_ZCA, true},
> > +     {"zcb", KVM_RISCV_ISA_EXT_ZCB, true},
> > +     {"zcd", KVM_RISCV_ISA_EXT_ZCD, true},
> > +     {"zcf", KVM_RISCV_ISA_EXT_ZCF, true},
> > +     {"zcmop", KVM_RISCV_ISA_EXT_ZCMOP, true},
> > +     {"zfa", KVM_RISCV_ISA_EXT_ZFA, true},
> > +     {"zfh", KVM_RISCV_ISA_EXT_ZFH, true},
> > +     {"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN, true},
> > +     {"zicbom", KVM_RISCV_ISA_EXT_ZICBOM, true},
> > +     {"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ, true},
> > +     {"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE, true},
> > +     {"zicntr", KVM_RISCV_ISA_EXT_ZICNTR, true},
> > +     {"zicond", KVM_RISCV_ISA_EXT_ZICOND, true},
> > +     {"zicsr", KVM_RISCV_ISA_EXT_ZICSR, true},
> > +     {"zifencei", KVM_RISCV_ISA_EXT_ZIFENCEI, true},
> > +     {"zihintntl", KVM_RISCV_ISA_EXT_ZIHINTNTL, true},
> > +     {"zihintpause", KVM_RISCV_ISA_EXT_ZIHINTPAUSE, true},
> > +     {"zihpm", KVM_RISCV_ISA_EXT_ZIHPM, true},
> > +     {"zimop", KVM_RISCV_ISA_EXT_ZIMOP, true},
> > +     {"zknd", KVM_RISCV_ISA_EXT_ZKND, true},
> > +     {"zkne", KVM_RISCV_ISA_EXT_ZKNE, true},
> > +     {"zknh", KVM_RISCV_ISA_EXT_ZKNH, true},
> > +     {"zkr", KVM_RISCV_ISA_EXT_ZKR, true},
> > +     {"zksed", KVM_RISCV_ISA_EXT_ZKSED, true},
> > +     {"zksh", KVM_RISCV_ISA_EXT_ZKSH, true},
> > +     {"zkt", KVM_RISCV_ISA_EXT_ZKT, true},
> > +     {"ztso", KVM_RISCV_ISA_EXT_ZTSO, true},
> > +     {"zvbb", KVM_RISCV_ISA_EXT_ZVBB, true},
> > +     {"zvbc", KVM_RISCV_ISA_EXT_ZVBC, true},
> > +     {"zvfh", KVM_RISCV_ISA_EXT_ZVFH, true},
> > +     {"zvfhmin", KVM_RISCV_ISA_EXT_ZVFHMIN, true},
> > +     {"zvkb", KVM_RISCV_ISA_EXT_ZVKB, true},
> > +     {"zvkg", KVM_RISCV_ISA_EXT_ZVKG, true},
> > +     {"zvkned", KVM_RISCV_ISA_EXT_ZVKNED, true},
> > +     {"zvknha", KVM_RISCV_ISA_EXT_ZVKNHA, true},
> > +     {"zvknhb", KVM_RISCV_ISA_EXT_ZVKNHB, true},
> > +     {"zvksed", KVM_RISCV_ISA_EXT_ZVKSED, true},
> > +     {"zvksh", KVM_RISCV_ISA_EXT_ZVKSH, true},
> > +     {"zvkt", KVM_RISCV_ISA_EXT_ZVKT, true},
>
> nit: I think I would add a 'misa' boolean member instead of 'multi_letter'
> and then rework this table to look like this

"misa" is not appropriate because misa CSR is only available
in M-mode.

We do have the notion of "single-letter" extensions
in RISC-V unpriv ISA conventions chapter so let me
use that instead.

>
>         {"a",           KVM_RISCV_ISA_EXT_A, .misa = true       },
>         {"c",           KVM_RISCV_ISA_EXT_C, .misa = true       },
>         {"d",           KVM_RISCV_ISA_EXT_D, .misa = true       },
>         {"f",           KVM_RISCV_ISA_EXT_F, .misa = true       },
>         {"h",           KVM_RISCV_ISA_EXT_H, .misa = true       },
>         {"i",           KVM_RISCV_ISA_EXT_I, .misa = true       },
>         {"m",           KVM_RISCV_ISA_EXT_M, .misa = true       },
>         {"v",           KVM_RISCV_ISA_EXT_V, .misa = true       },
>
>         /* multi-letter sorted alphabetically */
>         {"smnpm",       KVM_RISCV_ISA_EXT_SMNPM,                },
>         {"smstateen",   KVM_RISCV_ISA_EXT_SMSTATEEN,            },
>         ...
>
> The benefit is that only the misa extensions need another field.

See above comment.

>
> >  };
> >
> >  static void dump_fdt(const char *dtb_file, void *fdt)
> > @@ -129,6 +139,10 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
> >               }
> >
> >               for (i = 0; i < arr_sz; i++) {
> > +                     /* Skip single-letter extensions since these are taken care */
>
> We should finish the comment with 'of by "whatever takes care of them"'

Okay, I will update.

>
> > +                     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, &reg) < 0)
> > --
> > 2.43.0
> >
>
> Thanks,
> drew

Regards,
Anup

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

[Anup Patel]

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@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" */
+	{"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},
 	/* multi-letter sorted alphabetically */
-	{"smnpm", KVM_RISCV_ISA_EXT_SMNPM, true},
-	{"smstateen", KVM_RISCV_ISA_EXT_SMSTATEEN, true},
-	{"ssaia", KVM_RISCV_ISA_EXT_SSAIA, true},
-	{"sscofpmf", KVM_RISCV_ISA_EXT_SSCOFPMF, true},
-	{"ssnpm", KVM_RISCV_ISA_EXT_SSNPM, true},
-	{"sstc", KVM_RISCV_ISA_EXT_SSTC, true},
-	{"svade", KVM_RISCV_ISA_EXT_SVADE, true},
-	{"svadu", KVM_RISCV_ISA_EXT_SVADU, true},
-	{"svinval", KVM_RISCV_ISA_EXT_SVINVAL, true},
-	{"svnapot", KVM_RISCV_ISA_EXT_SVNAPOT, true},
-	{"svpbmt", KVM_RISCV_ISA_EXT_SVPBMT, true},
-	{"svvptc", KVM_RISCV_ISA_EXT_SVVPTC, true},
-	{"zabha", KVM_RISCV_ISA_EXT_ZABHA, true},
-	{"zacas", KVM_RISCV_ISA_EXT_ZACAS, true},
-	{"zawrs", KVM_RISCV_ISA_EXT_ZAWRS, true},
-	{"zba", KVM_RISCV_ISA_EXT_ZBA, true},
-	{"zbb", KVM_RISCV_ISA_EXT_ZBB, true},
-	{"zbc", KVM_RISCV_ISA_EXT_ZBC, true},
-	{"zbkb", KVM_RISCV_ISA_EXT_ZBKB, true},
-	{"zbkc", KVM_RISCV_ISA_EXT_ZBKC, true},
-	{"zbkx", KVM_RISCV_ISA_EXT_ZBKX, true},
-	{"zbs", KVM_RISCV_ISA_EXT_ZBS, true},
-	{"zca", KVM_RISCV_ISA_EXT_ZCA, true},
-	{"zcb", KVM_RISCV_ISA_EXT_ZCB, true},
-	{"zcd", KVM_RISCV_ISA_EXT_ZCD, true},
-	{"zcf", KVM_RISCV_ISA_EXT_ZCF, true},
-	{"zcmop", KVM_RISCV_ISA_EXT_ZCMOP, true},
-	{"zfa", KVM_RISCV_ISA_EXT_ZFA, true},
-	{"zfh", KVM_RISCV_ISA_EXT_ZFH, true},
-	{"zfhmin", KVM_RISCV_ISA_EXT_ZFHMIN, true},
-	{"zicbom", KVM_RISCV_ISA_EXT_ZICBOM, true},
-	{"zicboz", KVM_RISCV_ISA_EXT_ZICBOZ, true},
-	{"ziccrse", KVM_RISCV_ISA_EXT_ZICCRSE, true},
-	{"zicntr", KVM_RISCV_ISA_EXT_ZICNTR, true},
-	{"zicond", KVM_RISCV_ISA_EXT_ZICOND, true},
-	{"zicsr", KVM_RISCV_ISA_EXT_ZICSR, true},
-	{"zifencei", KVM_RISCV_ISA_EXT_ZIFENCEI, true},
-	{"zihintntl", KVM_RISCV_ISA_EXT_ZIHINTNTL, true},
-	{"zihintpause", KVM_RISCV_ISA_EXT_ZIHINTPAUSE, true},
-	{"zihpm", KVM_RISCV_ISA_EXT_ZIHPM, true},
-	{"zimop", KVM_RISCV_ISA_EXT_ZIMOP, true},
-	{"zknd", KVM_RISCV_ISA_EXT_ZKND, true},
-	{"zkne", KVM_RISCV_ISA_EXT_ZKNE, true},
-	{"zknh", KVM_RISCV_ISA_EXT_ZKNH, true},
-	{"zkr", KVM_RISCV_ISA_EXT_ZKR, true},
-	{"zksed", KVM_RISCV_ISA_EXT_ZKSED, true},
-	{"zksh", KVM_RISCV_ISA_EXT_ZKSH, true},
-	{"zkt", KVM_RISCV_ISA_EXT_ZKT, true},
-	{"ztso", KVM_RISCV_ISA_EXT_ZTSO, true},
-	{"zvbb", KVM_RISCV_ISA_EXT_ZVBB, true},
-	{"zvbc", KVM_RISCV_ISA_EXT_ZVBC, true},
-	{"zvfh", KVM_RISCV_ISA_EXT_ZVFH, true},
-	{"zvfhmin", KVM_RISCV_ISA_EXT_ZVFHMIN, true},
-	{"zvkb", KVM_RISCV_ISA_EXT_ZVKB, true},
-	{"zvkg", KVM_RISCV_ISA_EXT_ZVKG, true},
-	{"zvkned", KVM_RISCV_ISA_EXT_ZVKNED, true},
-	{"zvknha", KVM_RISCV_ISA_EXT_ZVKNHA, true},
-	{"zvknhb", KVM_RISCV_ISA_EXT_ZVKNHB, true},
-	{"zvksed", KVM_RISCV_ISA_EXT_ZVKSED, true},
-	{"zvksh", KVM_RISCV_ISA_EXT_ZVKSH, true},
-	{"zvkt", KVM_RISCV_ISA_EXT_ZVKT, true},
+	{"smnpm",	KVM_RISCV_ISA_EXT_SMNPM,	true, false},
+	{"smstateen",	KVM_RISCV_ISA_EXT_SMSTATEEN,	true, false},
+	{"ssaia",	KVM_RISCV_ISA_EXT_SSAIA,	true, false},
+	{"sscofpmf",	KVM_RISCV_ISA_EXT_SSCOFPMF,	true, false},
+	{"ssnpm",	KVM_RISCV_ISA_EXT_SSNPM,	true, false},
+	{"sstc",	KVM_RISCV_ISA_EXT_SSTC,		true, false},
+	{"svade",	KVM_RISCV_ISA_EXT_SVADE,	true, false},
+	{"svadu",	KVM_RISCV_ISA_EXT_SVADU,	true, false},
+	{"svinval",	KVM_RISCV_ISA_EXT_SVINVAL,	true, false},
+	{"svnapot",	KVM_RISCV_ISA_EXT_SVNAPOT,	true, false},
+	{"svpbmt",	KVM_RISCV_ISA_EXT_SVPBMT,	true, false},
+	{"svvptc",	KVM_RISCV_ISA_EXT_SVVPTC,	true, false},
+	{"zabha",	KVM_RISCV_ISA_EXT_ZABHA,	true, false},
+	{"zacas",	KVM_RISCV_ISA_EXT_ZACAS,	true, false},
+	{"zawrs",	KVM_RISCV_ISA_EXT_ZAWRS,	true, false},
+	{"zba",		KVM_RISCV_ISA_EXT_ZBA,		true, false},
+	{"zbb",		KVM_RISCV_ISA_EXT_ZBB,		true, false},
+	{"zbc",		KVM_RISCV_ISA_EXT_ZBC,		true, false},
+	{"zbkb",	KVM_RISCV_ISA_EXT_ZBKB,		true, false},
+	{"zbkc",	KVM_RISCV_ISA_EXT_ZBKC,		true, false},
+	{"zbkx",	KVM_RISCV_ISA_EXT_ZBKX,		true, false},
+	{"zbs",		KVM_RISCV_ISA_EXT_ZBS,		true, false},
+	{"zca",		KVM_RISCV_ISA_EXT_ZCA,		true, false},
+	{"zcb",		KVM_RISCV_ISA_EXT_ZCB,		true, false},
+	{"zcd",		KVM_RISCV_ISA_EXT_ZCD,		true, false},
+	{"zcf",		KVM_RISCV_ISA_EXT_ZCF,		true, false},
+	{"zcmop",	KVM_RISCV_ISA_EXT_ZCMOP,	true, false},
+	{"zfa",		KVM_RISCV_ISA_EXT_ZFA,		true, false},
+	{"zfh",		KVM_RISCV_ISA_EXT_ZFH,		true, false},
+	{"zfhmin",	KVM_RISCV_ISA_EXT_ZFHMIN,	true, false},
+	{"zicbom",	KVM_RISCV_ISA_EXT_ZICBOM,	true, false},
+	{"zicboz",	KVM_RISCV_ISA_EXT_ZICBOZ,	true, false},
+	{"ziccrse",	KVM_RISCV_ISA_EXT_ZICCRSE,	true, false},
+	{"zicntr",	KVM_RISCV_ISA_EXT_ZICNTR,	true, false},
+	{"zicond",	KVM_RISCV_ISA_EXT_ZICOND,	true, false},
+	{"zicsr",	KVM_RISCV_ISA_EXT_ZICSR,	true, false},
+	{"zifencei",	KVM_RISCV_ISA_EXT_ZIFENCEI,	true, false},
+	{"zihintntl",	KVM_RISCV_ISA_EXT_ZIHINTNTL,	true, false},
+	{"zihintpause",	KVM_RISCV_ISA_EXT_ZIHINTPAUSE,	true, false},
+	{"zihpm",	KVM_RISCV_ISA_EXT_ZIHPM,	true, false},
+	{"zimop",	KVM_RISCV_ISA_EXT_ZIMOP,	true, false},
+	{"zknd",	KVM_RISCV_ISA_EXT_ZKND,		true, false},
+	{"zkne",	KVM_RISCV_ISA_EXT_ZKNE,		true, false},
+	{"zknh",	KVM_RISCV_ISA_EXT_ZKNH,		true, false},
+	{"zkr",		KVM_RISCV_ISA_EXT_ZKR,		true, false},
+	{"zksed",	KVM_RISCV_ISA_EXT_ZKSED,	true, false},
+	{"zksh",	KVM_RISCV_ISA_EXT_ZKSH,		true, false},
+	{"zkt",		KVM_RISCV_ISA_EXT_ZKT,		true, false},
+	{"ztso",	KVM_RISCV_ISA_EXT_ZTSO,		true, false},
+	{"zvbb",	KVM_RISCV_ISA_EXT_ZVBB,		true, false},
+	{"zvbc",	KVM_RISCV_ISA_EXT_ZVBC,		true, false},
+	{"zvfh",	KVM_RISCV_ISA_EXT_ZVFH,		true, false},
+	{"zvfhmin",	KVM_RISCV_ISA_EXT_ZVFHMIN,	true, false},
+	{"zvkb",	KVM_RISCV_ISA_EXT_ZVKB,		true, false},
+	{"zvkg",	KVM_RISCV_ISA_EXT_ZVKG,		true, false},
+	{"zvkned",	KVM_RISCV_ISA_EXT_ZVKNED,	true, false},
+	{"zvknha",	KVM_RISCV_ISA_EXT_ZVKNHA,	true, false},
+	{"zvknhb",	KVM_RISCV_ISA_EXT_ZVKNHB,	true, false},
+	{"zvksed",	KVM_RISCV_ISA_EXT_ZVKSED,	true, false},
+	{"zvksh",	KVM_RISCV_ISA_EXT_ZVKSH,	true, false},
+	{"zvkt",	KVM_RISCV_ISA_EXT_ZVKT,		true, false},
 };
 
+static bool __isa_ext_disabled(struct kvm *kvm, struct isa_ext_info *info)
+{
+	if (!strncmp(kvm->cfg.arch.cpu_type, "min", 3) &&
+	    !info->min_cpu_included)
+		return true;
+
+	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) &&
+	    !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))
+		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, &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, &reg) < 0)
+				if ((ioctl(vcpu->vcpu_fd, KVM_SET_ONE_REG, &reg) < 0) &&
+				     __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);
 		}
 		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),\
 	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";
 }
 
 bool kvm__arch_cpu_supports_vm(void)
-- 
2.43.0

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

[Andrew Jones]

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@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, &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, &reg) < 0)
> +				if ((ioctl(vcpu->vcpu_fd, KVM_SET_ONE_REG, &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
> 

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

[Anup Patel]

On Sat, Apr 12, 2025 at 6:45 PM Andrew Jones <ajones@ventanamicro.com> 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@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.

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, &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, &reg) < 0)
> > +                             if ((ioctl(vcpu->vcpu_fd, KVM_SET_ONE_REG, &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

Re: [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@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

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

[Anup Patel]

On Thu, Apr 24, 2025 at 6:59 PM Andrew Jones <ajones@ventanamicro.com> 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@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

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

[kvmtool PATCH 10/10] riscv: Allow including extensions in the min CPU type using command-line

[Anup Patel]

It is useful to allow including extensions in the min CPU type on need
basis via command-line. To achieve this, parse extension names as comma
separated values appended to the "min" CPU type using command-line.

For example, to include Sstc and Ssaia in the min CPU type use
"--cpu-type min,sstc,ssaia" command-line option.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 riscv/fdt.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/riscv/fdt.c b/riscv/fdt.c
index 4c018c8..9cefd2f 100644
--- a/riscv/fdt.c
+++ b/riscv/fdt.c
@@ -108,6 +108,20 @@ static bool __isa_ext_warn_disable_failure(struct kvm *kvm, struct isa_ext_info
 	return true;
 }
 
+static void __min_cpu_include(const char *ext, size_t ext_len)
+{
+	struct isa_ext_info *info;
+	unsigned long i;
+
+	for (i = 0; i < ARRAY_SIZE(isa_info_arr); i++) {
+		info = &isa_info_arr[i];
+		if (strlen(info->name) != ext_len)
+			continue;
+		if (!strncmp(ext, info->name, ext_len))
+			info->min_cpu_included = true;
+	}
+}
+
 bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
 {
 	struct isa_ext_info *info = NULL;
@@ -128,16 +142,39 @@ bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
 int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset)
 {
 	struct kvm *kvm = opt->ptr;
+	const char *str, *nstr;
+	int len;
 
-	if ((strncmp(arg, "min", 3) && strncmp(arg, "max", 3)) || strlen(arg) != 3)
+	if ((strncmp(arg, "min", 3) || strlen(arg) < 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))
+	if (!strncmp(arg, "min", 3)) {
 		kvm->cfg.arch.cpu_type = "min";
 
+		str = arg;
+		str += 3;
+		while (*str) {
+			if (*str == ',') {
+				str++;
+				continue;
+			}
+
+			nstr = strchr(str, ',');
+			if (!nstr)
+				nstr = str + strlen(str);
+
+			len = nstr - str;
+			if (len) {
+				__min_cpu_include(str, len);
+				str += len;
+			}
+		}
+	}
+
 	return 0;
 }
 
-- 
2.43.0

Re: [kvmtool PATCH 10/10] riscv: Allow including extensions in the min CPU type using command-line

[Andrew Jones]

On Wed, Mar 26, 2025 at 12:26:44PM +0530, Anup Patel wrote:
> It is useful to allow including extensions in the min CPU type on need
> basis via command-line. To achieve this, parse extension names as comma
> separated values appended to the "min" CPU type using command-line.
> 
> For example, to include Sstc and Ssaia in the min CPU type use
> "--cpu-type min,sstc,ssaia" command-line option.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  riscv/fdt.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/riscv/fdt.c b/riscv/fdt.c
> index 4c018c8..9cefd2f 100644
> --- a/riscv/fdt.c
> +++ b/riscv/fdt.c
> @@ -108,6 +108,20 @@ static bool __isa_ext_warn_disable_failure(struct kvm *kvm, struct isa_ext_info
>  	return true;
>  }
>  
> +static void __min_cpu_include(const char *ext, size_t ext_len)

s/include/enable/

> +{
> +	struct isa_ext_info *info;
> +	unsigned long i;
> +
> +	for (i = 0; i < ARRAY_SIZE(isa_info_arr); i++) {
> +		info = &isa_info_arr[i];
> +		if (strlen(info->name) != ext_len)
> +			continue;
> +		if (!strncmp(ext, info->name, ext_len))

strcmp should be fine here since we already checked length.

> +			info->min_cpu_included = true;
> +	}
> +}
> +
>  bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
>  {
>  	struct isa_ext_info *info = NULL;
> @@ -128,16 +142,39 @@ bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
>  int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset)
>  {
>  	struct kvm *kvm = opt->ptr;
> +	const char *str, *nstr;
> +	int len;
>  
> -	if ((strncmp(arg, "min", 3) && strncmp(arg, "max", 3)) || strlen(arg) != 3)
> +	if ((strncmp(arg, "min", 3) || strlen(arg) < 3) &&

If arg == 'min', then it can't be less than 3 so the '|| strlen(arg) < 3'
is dead code.

> +	    (strncmp(arg, "max", 3) || strlen(arg) != 3))

I think we want

 if (strlen(arg) < 3 ||
     (strlen(arg) == 3 && strcmp(arg, "min") && strcmp(arg, "max")) ||
     strncmp(arg, "min", 3))

>  		die("Invalid CPU type %s\n", arg);
>  
>  	if (!strncmp(arg, "max", 3))
>  		kvm->cfg.arch.cpu_type = "max";
>  
> -	if (!strncmp(arg, "min", 3))
> +	if (!strncmp(arg, "min", 3)) {
>  		kvm->cfg.arch.cpu_type = "min";
>  
> +		str = arg;
> +		str += 3;
> +		while (*str) {
> +			if (*str == ',') {
> +				str++;
> +				continue;
> +			}
> +
> +			nstr = strchr(str, ',');
> +			if (!nstr)
> +				nstr = str + strlen(str);
> +
> +			len = nstr - str;
> +			if (len) {

I think len will always be nonzero since *str isn't \0 and we ate all
consecutive ,'s above. __min_cpu_include() is also safe to call with
len=0, so we could drop this check.

> +				__min_cpu_include(str, len);
> +				str += len;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.43.0
>

Thanks,
drew

Re: [kvmtool PATCH 10/10] riscv: Allow including extensions in the min CPU type using command-line

[Anup Patel]

On Sat, Apr 12, 2025 at 7:15 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Mar 26, 2025 at 12:26:44PM +0530, Anup Patel wrote:
> > It is useful to allow including extensions in the min CPU type on need
> > basis via command-line. To achieve this, parse extension names as comma
> > separated values appended to the "min" CPU type using command-line.
> >
> > For example, to include Sstc and Ssaia in the min CPU type use
> > "--cpu-type min,sstc,ssaia" command-line option.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  riscv/fdt.c | 41 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/riscv/fdt.c b/riscv/fdt.c
> > index 4c018c8..9cefd2f 100644
> > --- a/riscv/fdt.c
> > +++ b/riscv/fdt.c
> > @@ -108,6 +108,20 @@ static bool __isa_ext_warn_disable_failure(struct kvm *kvm, struct isa_ext_info
> >       return true;
> >  }
> >
> > +static void __min_cpu_include(const char *ext, size_t ext_len)
>
> s/include/enable/

Okay, I will update.

>
> > +{
> > +     struct isa_ext_info *info;
> > +     unsigned long i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(isa_info_arr); i++) {
> > +             info = &isa_info_arr[i];
> > +             if (strlen(info->name) != ext_len)
> > +                     continue;
> > +             if (!strncmp(ext, info->name, ext_len))
>
> strcmp should be fine here since we already checked length.

Well, strcmp() does not work here because the "ext" pointer
points to a non-null terminated substring of a larger comma
separated string.

>
> > +                     info->min_cpu_included = true;
> > +     }
> > +}
> > +
> >  bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> >  {
> >       struct isa_ext_info *info = NULL;
> > @@ -128,16 +142,39 @@ bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> >  int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset)
> >  {
> >       struct kvm *kvm = opt->ptr;
> > +     const char *str, *nstr;
> > +     int len;
> >
> > -     if ((strncmp(arg, "min", 3) && strncmp(arg, "max", 3)) || strlen(arg) != 3)
> > +     if ((strncmp(arg, "min", 3) || strlen(arg) < 3) &&
>
> If arg == 'min', then it can't be less than 3 so the '|| strlen(arg) < 3'
> is dead code.
>
> > +         (strncmp(arg, "max", 3) || strlen(arg) != 3))
>
> I think we want
>
>  if (strlen(arg) < 3 ||
>      (strlen(arg) == 3 && strcmp(arg, "min") && strcmp(arg, "max")) ||
>      strncmp(arg, "min", 3))

Nope, for cpu-type = "min" the strlen(arg) can be greater than 3
because of comma separated extensions provided as part of
cpu-type value.

>
> >               die("Invalid CPU type %s\n", arg);
> >
> >       if (!strncmp(arg, "max", 3))
> >               kvm->cfg.arch.cpu_type = "max";
> >
> > -     if (!strncmp(arg, "min", 3))
> > +     if (!strncmp(arg, "min", 3)) {
> >               kvm->cfg.arch.cpu_type = "min";
> >
> > +             str = arg;
> > +             str += 3;
> > +             while (*str) {
> > +                     if (*str == ',') {
> > +                             str++;
> > +                             continue;
> > +                     }
> > +
> > +                     nstr = strchr(str, ',');
> > +                     if (!nstr)
> > +                             nstr = str + strlen(str);
> > +
> > +                     len = nstr - str;
> > +                     if (len) {
>
> I think len will always be nonzero since *str isn't \0 and we ate all
> consecutive ,'s above. __min_cpu_include() is also safe to call with
> len=0, so we could drop this check.

No point in unnecessarily calling __min_cpu_include() for
len == 0. I am sure this extra check won't hurt so I think
we should keep it.

>
> > +                             __min_cpu_include(str, len);
> > +                             str += len;
> > +                     }
> > +             }
> > +     }
> > +
> >       return 0;
> >  }
> >
> > --
> > 2.43.0
> >
>
> Thanks,
> drew

Regards,
Anup

Re: [kvmtool PATCH 10/10] riscv: Allow including extensions in the min CPU type using command-line

[Andrew Jones]

On Thu, Apr 24, 2025 at 07:02:18PM +0530, Anup Patel wrote:
> On Sat, Apr 12, 2025 at 7:15 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >  bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> > >  {
> > >       struct isa_ext_info *info = NULL;
> > > @@ -128,16 +142,39 @@ bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> > >  int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset)
> > >  {
> > >       struct kvm *kvm = opt->ptr;
> > > +     const char *str, *nstr;
> > > +     int len;
> > >
> > > -     if ((strncmp(arg, "min", 3) && strncmp(arg, "max", 3)) || strlen(arg) != 3)
> > > +     if ((strncmp(arg, "min", 3) || strlen(arg) < 3) &&
> >
> > If arg == 'min', then it can't be less than 3 so the '|| strlen(arg) < 3'
> > is dead code.
> >
> > > +         (strncmp(arg, "max", 3) || strlen(arg) != 3))
> >
> > I think we want
> >
> >  if (strlen(arg) < 3 ||
> >      (strlen(arg) == 3 && strcmp(arg, "min") && strcmp(arg, "max")) ||
> >      strncmp(arg, "min", 3))
> 
> Nope, for cpu-type = "min" the strlen(arg) can be greater than 3
> because of comma separated extensions provided as part of
> cpu-type value.

That's what the last condition 'strncmp(arg, "min", 3)' of my proposed
compound-condition is confirming. That condition is only checked for
strlen(arg) > 3.

Thanks,
drew

Re: [kvmtool PATCH 10/10] riscv: Allow including extensions in the min CPU type using command-line

[Anup Patel]

On Thu, Apr 24, 2025 at 7:37 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Apr 24, 2025 at 07:02:18PM +0530, Anup Patel wrote:
> > On Sat, Apr 12, 2025 at 7:15 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > > >  bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> > > >  {
> > > >       struct isa_ext_info *info = NULL;
> > > > @@ -128,16 +142,39 @@ bool riscv__isa_extension_disabled(struct kvm *kvm, unsigned long isa_ext_id)
> > > >  int riscv__cpu_type_parser(const struct option *opt, const char *arg, int unset)
> > > >  {
> > > >       struct kvm *kvm = opt->ptr;
> > > > +     const char *str, *nstr;
> > > > +     int len;
> > > >
> > > > -     if ((strncmp(arg, "min", 3) && strncmp(arg, "max", 3)) || strlen(arg) != 3)
> > > > +     if ((strncmp(arg, "min", 3) || strlen(arg) < 3) &&
> > >
> > > If arg == 'min', then it can't be less than 3 so the '|| strlen(arg) < 3'
> > > is dead code.
> > >
> > > > +         (strncmp(arg, "max", 3) || strlen(arg) != 3))
> > >
> > > I think we want
> > >
> > >  if (strlen(arg) < 3 ||
> > >      (strlen(arg) == 3 && strcmp(arg, "min") && strcmp(arg, "max")) ||
> > >      strncmp(arg, "min", 3))
> >
> > Nope, for cpu-type = "min" the strlen(arg) can be greater than 3
> > because of comma separated extensions provided as part of
> > cpu-type value.
>
> That's what the last condition 'strncmp(arg, "min", 3)' of my proposed
> compound-condition is confirming. That condition is only checked for
> strlen(arg) > 3.
>

This last condition causes "--cpu-type max" to fail.

Regards,
Anup