[PATCH v3 0/9] Add SBI v3.0 PMU enhancements

[PATCH v3 0/9] Add SBI v3.0 PMU enhancements

[Atish Patra]

SBI v3.0 specification[1] added two new improvements to the PMU chaper.
The SBI v3.0 specification is frozen and under public review phase as
per the RISC-V International guidelines. 

1. Added an additional get_event_info function to query event availablity
in bulk instead of individual SBI calls for each event. This helps in
improving the boot time.

2. Raw event width allowed by the platform is widened to have 56 bits
with RAW event v2 as per new clarification in the priv ISA[2].

Apart from implementing these new features, this series improves the gpa
range check in KVM and updates the kvm SBI implementation to SBI v3.0.

The opensbi patches have been merged. This series can be found at [3].

[1] https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/v3.0-rc7/riscv-sbi.pdf 
[2] https://github.com/riscv/riscv-isa-manual/issues/1578
[3] https://github.com/atishp04/linux/tree/b4/pmu_event_info_v3

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Changes in v3:
- Rebased on top of v6.15-rc7 
- Link to v2: https://lore.kernel.org/r/20250115-pmu_event_info-v2-0-84815b70383b@rivosinc.com

Changes in v2:
- Dropped PATCH 2 to be taken during rcX.
- Improved gpa range check validation by introducing a helper function
  and checking the entire range.
- Link to v1: https://lore.kernel.org/r/20241119-pmu_event_info-v1-0-a4f9691421f8@rivosinc.com

---
Atish Patra (9):
      drivers/perf: riscv: Add SBI v3.0 flag
      drivers/perf: riscv: Add raw event v2 support
      RISC-V: KVM: Add support for Raw event v2
      drivers/perf: riscv: Implement PMU event info function
      drivers/perf: riscv: Export PMU event info function
      KVM: Add a helper function to validate vcpu gpa range
      RISC-V: KVM: Use the new gpa range validate helper function
      RISC-V: KVM: Implement get event info function
      RISC-V: KVM: Upgrade the supported SBI version to 3.0

 arch/riscv/include/asm/kvm_vcpu_pmu.h |   3 +
 arch/riscv/include/asm/kvm_vcpu_sbi.h |   2 +-
 arch/riscv/include/asm/sbi.h          |  13 +++
 arch/riscv/kvm/vcpu_pmu.c             |  75 +++++++++++++-
 arch/riscv/kvm/vcpu_sbi_pmu.c         |   3 +
 arch/riscv/kvm/vcpu_sbi_sta.c         |   6 +-
 drivers/perf/riscv_pmu_sbi.c          | 190 +++++++++++++++++++++++++---------
 include/linux/kvm_host.h              |   2 +
 include/linux/perf/riscv_pmu.h        |   2 +
 virt/kvm/kvm_main.c                   |  21 ++++
 10 files changed, 258 insertions(+), 59 deletions(-)
---
base-commit: e32a80927434907f973f38a88cd19d7e51991d24
change-id: 20241018-pmu_event_info-986e21ce6bd3
--
Regards,
Atish patra

[PATCH v3 1/9] drivers/perf: riscv: Add SBI v3.0 flag

[Atish Patra]

There are new PMU related features introduced in SBI v3.0.
1. Raw Event v2 which allows mhpmeventX value to be 56 bit wide.
2. Get Event info function to do a bulk query at one shot.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 drivers/perf/riscv_pmu_sbi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 698de8ddf895..cfd6946fca42 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -63,6 +63,7 @@ PMU_FORMAT_ATTR(event, "config:0-47");
 PMU_FORMAT_ATTR(firmware, "config:62-63");
 
 static bool sbi_v2_available;
+static bool sbi_v3_available;
 static DEFINE_STATIC_KEY_FALSE(sbi_pmu_snapshot_available);
 #define sbi_pmu_snapshot_available() \
 	static_branch_unlikely(&sbi_pmu_snapshot_available)
@@ -1452,6 +1453,9 @@ static int __init pmu_sbi_devinit(void)
 	if (sbi_spec_version >= sbi_mk_version(2, 0))
 		sbi_v2_available = true;
 
+	if (sbi_spec_version >= sbi_mk_version(3, 0))
+		sbi_v3_available = true;
+
 	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_STARTING,
 				      "perf/riscv/pmu:starting",
 				      pmu_sbi_starting_cpu, pmu_sbi_dying_cpu);

-- 
2.43.0

Re: [PATCH v3 1/9] drivers/perf: riscv: Add SBI v3.0 flag

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> There are new PMU related features introduced in SBI v3.0.
> 1. Raw Event v2 which allows mhpmeventX value to be 56 bit wide.
> 2. Get Event info function to do a bulk query at one shot.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  drivers/perf/riscv_pmu_sbi.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 698de8ddf895..cfd6946fca42 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -63,6 +63,7 @@ PMU_FORMAT_ATTR(event, "config:0-47");
>  PMU_FORMAT_ATTR(firmware, "config:62-63");
>
>  static bool sbi_v2_available;
> +static bool sbi_v3_available;
>  static DEFINE_STATIC_KEY_FALSE(sbi_pmu_snapshot_available);
>  #define sbi_pmu_snapshot_available() \
>         static_branch_unlikely(&sbi_pmu_snapshot_available)
> @@ -1452,6 +1453,9 @@ static int __init pmu_sbi_devinit(void)
>         if (sbi_spec_version >= sbi_mk_version(2, 0))
>                 sbi_v2_available = true;
>
> +       if (sbi_spec_version >= sbi_mk_version(3, 0))
> +               sbi_v3_available = true;
> +
>         ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_STARTING,
>                                       "perf/riscv/pmu:starting",
>                                       pmu_sbi_starting_cpu, pmu_sbi_dying_cpu);
>
> --
> 2.43.0
>

[PATCH v3 2/9] drivers/perf: riscv: Add raw event v2 support

[Atish Patra]

SBI v3.0 introduced a new raw event type that allows wider
mhpmeventX width to be programmed via CFG_MATCH.

Use the raw event v2 if SBI v3.0 is available.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/sbi.h |  4 ++++
 drivers/perf/riscv_pmu_sbi.c | 16 ++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 3d250824178b..6ce385a3a7bb 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -160,7 +160,10 @@ struct riscv_pmu_snapshot_data {
 
 #define RISCV_PMU_RAW_EVENT_MASK GENMASK_ULL(47, 0)
 #define RISCV_PMU_PLAT_FW_EVENT_MASK GENMASK_ULL(61, 0)
+/* SBI v3.0 allows extended hpmeventX width value */
+#define RISCV_PMU_RAW_EVENT_V2_MASK GENMASK_ULL(55, 0)
 #define RISCV_PMU_RAW_EVENT_IDX 0x20000
+#define RISCV_PMU_RAW_EVENT_V2_IDX 0x30000
 #define RISCV_PLAT_FW_EVENT	0xFFFF
 
 /** General pmu event codes specified in SBI PMU extension */
@@ -218,6 +221,7 @@ enum sbi_pmu_event_type {
 	SBI_PMU_EVENT_TYPE_HW = 0x0,
 	SBI_PMU_EVENT_TYPE_CACHE = 0x1,
 	SBI_PMU_EVENT_TYPE_RAW = 0x2,
+	SBI_PMU_EVENT_TYPE_RAW_V2 = 0x3,
 	SBI_PMU_EVENT_TYPE_FW = 0xf,
 };
 
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index cfd6946fca42..273ed70098a3 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -59,7 +59,7 @@ asm volatile(ALTERNATIVE(						\
 #define PERF_EVENT_FLAG_USER_ACCESS	BIT(SYSCTL_USER_ACCESS)
 #define PERF_EVENT_FLAG_LEGACY		BIT(SYSCTL_LEGACY)
 
-PMU_FORMAT_ATTR(event, "config:0-47");
+PMU_FORMAT_ATTR(event, "config:0-55");
 PMU_FORMAT_ATTR(firmware, "config:62-63");
 
 static bool sbi_v2_available;
@@ -527,8 +527,10 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 		break;
 	case PERF_TYPE_RAW:
 		/*
-		 * As per SBI specification, the upper 16 bits must be unused
-		 * for a hardware raw event.
+		 * As per SBI v0.3 specification,
+		 *  -- the upper 16 bits must be unused for a hardware raw event.
+		 * As per SBI v3.0 specification,
+		 *  -- the upper 8 bits must be unused for a hardware raw event.
 		 * Bits 63:62 are used to distinguish between raw events
 		 * 00 - Hardware raw event
 		 * 10 - SBI firmware events
@@ -537,8 +539,14 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 
 		switch (config >> 62) {
 		case 0:
+			if (sbi_v3_available) {
+			/* Return error any bits [56-63] is set  as it is not allowed by the spec */
+				if (!(config & ~RISCV_PMU_RAW_EVENT_V2_MASK)) {
+					*econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
+					ret = RISCV_PMU_RAW_EVENT_V2_IDX;
+				}
 			/* Return error any bits [48-63] is set  as it is not allowed by the spec */
-			if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
+			} else if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
 				*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
 				ret = RISCV_PMU_RAW_EVENT_IDX;
 			}

-- 
2.43.0

Re: [PATCH v3 2/9] drivers/perf: riscv: Add raw event v2 support

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> SBI v3.0 introduced a new raw event type that allows wider
> mhpmeventX width to be programmed via CFG_MATCH.
>
> Use the raw event v2 if SBI v3.0 is available.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  arch/riscv/include/asm/sbi.h |  4 ++++
>  drivers/perf/riscv_pmu_sbi.c | 16 ++++++++++++----
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 3d250824178b..6ce385a3a7bb 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -160,7 +160,10 @@ struct riscv_pmu_snapshot_data {
>
>  #define RISCV_PMU_RAW_EVENT_MASK GENMASK_ULL(47, 0)
>  #define RISCV_PMU_PLAT_FW_EVENT_MASK GENMASK_ULL(61, 0)
> +/* SBI v3.0 allows extended hpmeventX width value */
> +#define RISCV_PMU_RAW_EVENT_V2_MASK GENMASK_ULL(55, 0)
>  #define RISCV_PMU_RAW_EVENT_IDX 0x20000
> +#define RISCV_PMU_RAW_EVENT_V2_IDX 0x30000
>  #define RISCV_PLAT_FW_EVENT    0xFFFF
>
>  /** General pmu event codes specified in SBI PMU extension */
> @@ -218,6 +221,7 @@ enum sbi_pmu_event_type {
>         SBI_PMU_EVENT_TYPE_HW = 0x0,
>         SBI_PMU_EVENT_TYPE_CACHE = 0x1,
>         SBI_PMU_EVENT_TYPE_RAW = 0x2,
> +       SBI_PMU_EVENT_TYPE_RAW_V2 = 0x3,
>         SBI_PMU_EVENT_TYPE_FW = 0xf,
>  };
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index cfd6946fca42..273ed70098a3 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -59,7 +59,7 @@ asm volatile(ALTERNATIVE(                                             \
>  #define PERF_EVENT_FLAG_USER_ACCESS    BIT(SYSCTL_USER_ACCESS)
>  #define PERF_EVENT_FLAG_LEGACY         BIT(SYSCTL_LEGACY)
>
> -PMU_FORMAT_ATTR(event, "config:0-47");
> +PMU_FORMAT_ATTR(event, "config:0-55");
>  PMU_FORMAT_ATTR(firmware, "config:62-63");
>
>  static bool sbi_v2_available;
> @@ -527,8 +527,10 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>                 break;
>         case PERF_TYPE_RAW:
>                 /*
> -                * As per SBI specification, the upper 16 bits must be unused
> -                * for a hardware raw event.
> +                * As per SBI v0.3 specification,
> +                *  -- the upper 16 bits must be unused for a hardware raw event.
> +                * As per SBI v3.0 specification,

The text here should be "As-per SBI v2.0 ..."

> +                *  -- the upper 8 bits must be unused for a hardware raw event.
>                  * Bits 63:62 are used to distinguish between raw events
>                  * 00 - Hardware raw event
>                  * 10 - SBI firmware events
> @@ -537,8 +539,14 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>
>                 switch (config >> 62) {
>                 case 0:
> +                       if (sbi_v3_available) {
> +                       /* Return error any bits [56-63] is set  as it is not allowed by the spec */
> +                               if (!(config & ~RISCV_PMU_RAW_EVENT_V2_MASK)) {
> +                                       *econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
> +                                       ret = RISCV_PMU_RAW_EVENT_V2_IDX;
> +                               }
>                         /* Return error any bits [48-63] is set  as it is not allowed by the spec */
> -                       if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
> +                       } else if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
>                                 *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
>                                 ret = RISCV_PMU_RAW_EVENT_IDX;
>                         }
>
> --
> 2.43.0
>

Otherwise, this looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

[PATCH v3 3/9] RISC-V: KVM: Add support for Raw event v2

[Atish Patra]

SBI v3.0 introuced a new raw event type v2 for wider mhpmeventX
programming. Add the support in kvm for that.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/kvm/vcpu_pmu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
index 78ac3216a54d..15d71a7b75ba 100644
--- a/arch/riscv/kvm/vcpu_pmu.c
+++ b/arch/riscv/kvm/vcpu_pmu.c
@@ -60,6 +60,7 @@ static u32 kvm_pmu_get_perf_event_type(unsigned long eidx)
 		type = PERF_TYPE_HW_CACHE;
 		break;
 	case SBI_PMU_EVENT_TYPE_RAW:
+	case SBI_PMU_EVENT_TYPE_RAW_V2:
 	case SBI_PMU_EVENT_TYPE_FW:
 		type = PERF_TYPE_RAW;
 		break;
@@ -128,6 +129,9 @@ static u64 kvm_pmu_get_perf_event_config(unsigned long eidx, uint64_t evt_data)
 	case SBI_PMU_EVENT_TYPE_RAW:
 		config = evt_data & RISCV_PMU_RAW_EVENT_MASK;
 		break;
+	case SBI_PMU_EVENT_TYPE_RAW_V2:
+		config = evt_data & RISCV_PMU_RAW_EVENT_V2_MASK;
+		break;
 	case SBI_PMU_EVENT_TYPE_FW:
 		if (ecode < SBI_PMU_FW_MAX)
 			config = (1ULL << 63) | ecode;

-- 
2.43.0

Re: [PATCH v3 3/9] RISC-V: KVM: Add support for Raw event v2

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> SBI v3.0 introuced a new raw event type v2 for wider mhpmeventX

s/introuced/introduced/

> programming. Add the support in kvm for that.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  arch/riscv/kvm/vcpu_pmu.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 78ac3216a54d..15d71a7b75ba 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -60,6 +60,7 @@ static u32 kvm_pmu_get_perf_event_type(unsigned long eidx)
>                 type = PERF_TYPE_HW_CACHE;
>                 break;
>         case SBI_PMU_EVENT_TYPE_RAW:
> +       case SBI_PMU_EVENT_TYPE_RAW_V2:
>         case SBI_PMU_EVENT_TYPE_FW:
>                 type = PERF_TYPE_RAW;
>                 break;
> @@ -128,6 +129,9 @@ static u64 kvm_pmu_get_perf_event_config(unsigned long eidx, uint64_t evt_data)
>         case SBI_PMU_EVENT_TYPE_RAW:
>                 config = evt_data & RISCV_PMU_RAW_EVENT_MASK;
>                 break;
> +       case SBI_PMU_EVENT_TYPE_RAW_V2:
> +               config = evt_data & RISCV_PMU_RAW_EVENT_V2_MASK;
> +               break;
>         case SBI_PMU_EVENT_TYPE_FW:
>                 if (ecode < SBI_PMU_FW_MAX)
>                         config = (1ULL << 63) | ecode;
>
> --
> 2.43.0
>

Otherwise, it looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

[PATCH v3 4/9] drivers/perf: riscv: Implement PMU event info function

[Atish Patra]

With the new SBI PMU event info function, we can query the availability
of the all standard SBI PMU events at boot time with a single ecall.
This improves the bootime by avoiding making an SBI call for each
standard PMU event. Since this function is defined only in SBI v3.0,
invoke this only if the underlying SBI implementation is v3.0 or higher.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/sbi.h |  9 ++++++
 drivers/perf/riscv_pmu_sbi.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 6ce385a3a7bb..77b6997eb6c5 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -135,6 +135,7 @@ enum sbi_ext_pmu_fid {
 	SBI_EXT_PMU_COUNTER_FW_READ,
 	SBI_EXT_PMU_COUNTER_FW_READ_HI,
 	SBI_EXT_PMU_SNAPSHOT_SET_SHMEM,
+	SBI_EXT_PMU_EVENT_GET_INFO,
 };
 
 union sbi_pmu_ctr_info {
@@ -158,6 +159,14 @@ struct riscv_pmu_snapshot_data {
 	u64 reserved[447];
 };
 
+struct riscv_pmu_event_info {
+	u32 event_idx;
+	u32 output;
+	u64 event_data;
+};
+
+#define RISCV_PMU_EVENT_INFO_OUTPUT_MASK 0x01
+
 #define RISCV_PMU_RAW_EVENT_MASK GENMASK_ULL(47, 0)
 #define RISCV_PMU_PLAT_FW_EVENT_MASK GENMASK_ULL(61, 0)
 /* SBI v3.0 allows extended hpmeventX width value */
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 273ed70098a3..33d8348bf68a 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -299,6 +299,66 @@ static struct sbi_pmu_event_data pmu_cache_event_map[PERF_COUNT_HW_CACHE_MAX]
 	},
 };
 
+static int pmu_sbi_check_event_info(void)
+{
+	int num_events = ARRAY_SIZE(pmu_hw_event_map) + PERF_COUNT_HW_CACHE_MAX *
+			 PERF_COUNT_HW_CACHE_OP_MAX * PERF_COUNT_HW_CACHE_RESULT_MAX;
+	struct riscv_pmu_event_info *event_info_shmem;
+	phys_addr_t base_addr;
+	int i, j, k, result = 0, count = 0;
+	struct sbiret ret;
+
+	event_info_shmem = kcalloc(num_events, sizeof(*event_info_shmem), GFP_KERNEL);
+	if (!event_info_shmem)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++)
+		event_info_shmem[count++].event_idx = pmu_hw_event_map[i].event_idx;
+
+	for (i = 0; i < ARRAY_SIZE(pmu_cache_event_map); i++) {
+		for (j = 0; j < ARRAY_SIZE(pmu_cache_event_map[i]); j++) {
+			for (k = 0; k < ARRAY_SIZE(pmu_cache_event_map[i][j]); k++)
+				event_info_shmem[count++].event_idx =
+							pmu_cache_event_map[i][j][k].event_idx;
+		}
+	}
+
+	base_addr = __pa(event_info_shmem);
+	if (IS_ENABLED(CONFIG_32BIT))
+		ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_EVENT_GET_INFO, lower_32_bits(base_addr),
+				upper_32_bits(base_addr), count, 0, 0, 0);
+	else
+		ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_EVENT_GET_INFO, base_addr, 0,
+				count, 0, 0, 0);
+	if (ret.error) {
+		result = -EOPNOTSUPP;
+		goto free_mem;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++) {
+		if (!(event_info_shmem[i].output & RISCV_PMU_EVENT_INFO_OUTPUT_MASK))
+			pmu_hw_event_map[i].event_idx = -ENOENT;
+	}
+
+	count = ARRAY_SIZE(pmu_hw_event_map);
+
+	for (i = 0; i < ARRAY_SIZE(pmu_cache_event_map); i++) {
+		for (j = 0; j < ARRAY_SIZE(pmu_cache_event_map[i]); j++) {
+			for (k = 0; k < ARRAY_SIZE(pmu_cache_event_map[i][j]); k++) {
+				if (!(event_info_shmem[count].output &
+				      RISCV_PMU_EVENT_INFO_OUTPUT_MASK))
+					pmu_cache_event_map[i][j][k].event_idx = -ENOENT;
+				count++;
+			}
+		}
+	}
+
+free_mem:
+	kfree(event_info_shmem);
+
+	return result;
+}
+
 static void pmu_sbi_check_event(struct sbi_pmu_event_data *edata)
 {
 	struct sbiret ret;
@@ -316,6 +376,14 @@ static void pmu_sbi_check_event(struct sbi_pmu_event_data *edata)
 
 static void pmu_sbi_check_std_events(struct work_struct *work)
 {
+	int ret;
+
+	if (sbi_v3_available) {
+		ret = pmu_sbi_check_event_info();
+		if (!ret)
+			return;
+	}
+
 	for (int i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++)
 		pmu_sbi_check_event(&pmu_hw_event_map[i]);
 

-- 
2.43.0

Re: [PATCH v3 4/9] drivers/perf: riscv: Implement PMU event info function

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> With the new SBI PMU event info function, we can query the availability
> of the all standard SBI PMU events at boot time with a single ecall.
> This improves the bootime by avoiding making an SBI call for each
> standard PMU event. Since this function is defined only in SBI v3.0,
> invoke this only if the underlying SBI implementation is v3.0 or higher.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  arch/riscv/include/asm/sbi.h |  9 ++++++
>  drivers/perf/riscv_pmu_sbi.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 6ce385a3a7bb..77b6997eb6c5 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -135,6 +135,7 @@ enum sbi_ext_pmu_fid {
>         SBI_EXT_PMU_COUNTER_FW_READ,
>         SBI_EXT_PMU_COUNTER_FW_READ_HI,
>         SBI_EXT_PMU_SNAPSHOT_SET_SHMEM,
> +       SBI_EXT_PMU_EVENT_GET_INFO,
>  };
>
>  union sbi_pmu_ctr_info {
> @@ -158,6 +159,14 @@ struct riscv_pmu_snapshot_data {
>         u64 reserved[447];
>  };
>
> +struct riscv_pmu_event_info {
> +       u32 event_idx;
> +       u32 output;
> +       u64 event_data;
> +};
> +
> +#define RISCV_PMU_EVENT_INFO_OUTPUT_MASK 0x01
> +
>  #define RISCV_PMU_RAW_EVENT_MASK GENMASK_ULL(47, 0)
>  #define RISCV_PMU_PLAT_FW_EVENT_MASK GENMASK_ULL(61, 0)
>  /* SBI v3.0 allows extended hpmeventX width value */
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 273ed70098a3..33d8348bf68a 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -299,6 +299,66 @@ static struct sbi_pmu_event_data pmu_cache_event_map[PERF_COUNT_HW_CACHE_MAX]
>         },
>  };
>
> +static int pmu_sbi_check_event_info(void)
> +{
> +       int num_events = ARRAY_SIZE(pmu_hw_event_map) + PERF_COUNT_HW_CACHE_MAX *
> +                        PERF_COUNT_HW_CACHE_OP_MAX * PERF_COUNT_HW_CACHE_RESULT_MAX;
> +       struct riscv_pmu_event_info *event_info_shmem;
> +       phys_addr_t base_addr;
> +       int i, j, k, result = 0, count = 0;
> +       struct sbiret ret;
> +
> +       event_info_shmem = kcalloc(num_events, sizeof(*event_info_shmem), GFP_KERNEL);
> +       if (!event_info_shmem)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++)
> +               event_info_shmem[count++].event_idx = pmu_hw_event_map[i].event_idx;
> +
> +       for (i = 0; i < ARRAY_SIZE(pmu_cache_event_map); i++) {
> +               for (j = 0; j < ARRAY_SIZE(pmu_cache_event_map[i]); j++) {
> +                       for (k = 0; k < ARRAY_SIZE(pmu_cache_event_map[i][j]); k++)
> +                               event_info_shmem[count++].event_idx =
> +                                                       pmu_cache_event_map[i][j][k].event_idx;
> +               }
> +       }
> +
> +       base_addr = __pa(event_info_shmem);
> +       if (IS_ENABLED(CONFIG_32BIT))
> +               ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_EVENT_GET_INFO, lower_32_bits(base_addr),
> +                               upper_32_bits(base_addr), count, 0, 0, 0);
> +       else
> +               ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_EVENT_GET_INFO, base_addr, 0,
> +                               count, 0, 0, 0);
> +       if (ret.error) {
> +               result = -EOPNOTSUPP;
> +               goto free_mem;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++) {
> +               if (!(event_info_shmem[i].output & RISCV_PMU_EVENT_INFO_OUTPUT_MASK))
> +                       pmu_hw_event_map[i].event_idx = -ENOENT;
> +       }
> +
> +       count = ARRAY_SIZE(pmu_hw_event_map);
> +
> +       for (i = 0; i < ARRAY_SIZE(pmu_cache_event_map); i++) {
> +               for (j = 0; j < ARRAY_SIZE(pmu_cache_event_map[i]); j++) {
> +                       for (k = 0; k < ARRAY_SIZE(pmu_cache_event_map[i][j]); k++) {
> +                               if (!(event_info_shmem[count].output &
> +                                     RISCV_PMU_EVENT_INFO_OUTPUT_MASK))
> +                                       pmu_cache_event_map[i][j][k].event_idx = -ENOENT;
> +                               count++;
> +                       }
> +               }
> +       }
> +
> +free_mem:
> +       kfree(event_info_shmem);
> +
> +       return result;
> +}
> +
>  static void pmu_sbi_check_event(struct sbi_pmu_event_data *edata)
>  {
>         struct sbiret ret;
> @@ -316,6 +376,14 @@ static void pmu_sbi_check_event(struct sbi_pmu_event_data *edata)
>
>  static void pmu_sbi_check_std_events(struct work_struct *work)
>  {
> +       int ret;
> +
> +       if (sbi_v3_available) {
> +               ret = pmu_sbi_check_event_info();
> +               if (!ret)
> +                       return;

We should not fall back to the old way if ret != 0. Rather,
the pmu_sbi_check_std_events() should return failure if
pmu_sbi_check_event_info() fails.

> +       }
> +
>         for (int i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++)
>                 pmu_sbi_check_event(&pmu_hw_event_map[i]);
>
>
> --
> 2.43.0
>

Regards,
Anup

[PATCH v3 5/9] drivers/perf: riscv: Export PMU event info function

[Atish Patra]

The event mapping function can be used in event info function to find out
the corresponding SBI PMU event encoding during the get_event_info function
as well. Refactor and export it so that it can be invoked from kvm and
internal driver.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 drivers/perf/riscv_pmu_sbi.c   | 124 ++++++++++++++++++++++-------------------
 include/linux/perf/riscv_pmu.h |   2 +
 2 files changed, 69 insertions(+), 57 deletions(-)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 33d8348bf68a..f5d3db6dba18 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -100,6 +100,7 @@ static unsigned int riscv_pmu_irq;
 /* Cache the available counters in a bitmask */
 static unsigned long cmask;
 
+static int pmu_event_find_cache(u64 config);
 struct sbi_pmu_event_data {
 	union {
 		union {
@@ -411,6 +412,71 @@ static bool pmu_sbi_ctr_is_fw(int cidx)
 	return (info->type == SBI_PMU_CTR_TYPE_FW) ? true : false;
 }
 
+int riscv_pmu_get_event_info(u32 type, u64 config, u64 *econfig)
+{
+	int ret = -ENOENT;
+
+	switch (type) {
+	case PERF_TYPE_HARDWARE:
+		if (config >= PERF_COUNT_HW_MAX)
+			return -EINVAL;
+		ret = pmu_hw_event_map[config].event_idx;
+		break;
+	case PERF_TYPE_HW_CACHE:
+		ret = pmu_event_find_cache(config);
+		break;
+	case PERF_TYPE_RAW:
+		/*
+		 * As per SBI v0.3 specification,
+		 *  -- the upper 16 bits must be unused for a hardware raw event.
+		 * As per SBI v3.0 specification,
+		 *  -- the upper 8 bits must be unused for a hardware raw event.
+		 * Bits 63:62 are used to distinguish between raw events
+		 * 00 - Hardware raw event
+		 * 10 - SBI firmware events
+		 * 11 - Risc-V platform specific firmware event
+		 */
+		switch (config >> 62) {
+		case 0:
+			if (sbi_v3_available) {
+			/* Return error any bits [56-63] is set  as it is not allowed by the spec */
+				if (!(config & ~RISCV_PMU_RAW_EVENT_V2_MASK)) {
+					if (econfig)
+						*econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
+					ret = RISCV_PMU_RAW_EVENT_V2_IDX;
+				}
+			/* Return error any bits [48-63] is set  as it is not allowed by the spec */
+			} else if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
+				if (econfig)
+					*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
+				ret = RISCV_PMU_RAW_EVENT_IDX;
+			}
+			break;
+		case 2:
+			ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
+			break;
+		case 3:
+			/*
+			 * For Risc-V platform specific firmware events
+			 * Event code - 0xFFFF
+			 * Event data - raw event encoding
+			 */
+			ret = SBI_PMU_EVENT_TYPE_FW << 16 | RISCV_PLAT_FW_EVENT;
+			if (econfig)
+				*econfig = config & RISCV_PMU_PLAT_FW_EVENT_MASK;
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(riscv_pmu_get_event_info);
+
 /*
  * Returns the counter width of a programmable counter and number of hardware
  * counters. As we don't support heterogeneous CPUs yet, it is okay to just
@@ -576,7 +642,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 {
 	u32 type = event->attr.type;
 	u64 config = event->attr.config;
-	int ret = -ENOENT;
 
 	/*
 	 * Ensure we are finished checking standard hardware events for
@@ -584,62 +649,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 	 */
 	flush_work(&check_std_events_work);
 
-	switch (type) {
-	case PERF_TYPE_HARDWARE:
-		if (config >= PERF_COUNT_HW_MAX)
-			return -EINVAL;
-		ret = pmu_hw_event_map[event->attr.config].event_idx;
-		break;
-	case PERF_TYPE_HW_CACHE:
-		ret = pmu_event_find_cache(config);
-		break;
-	case PERF_TYPE_RAW:
-		/*
-		 * As per SBI v0.3 specification,
-		 *  -- the upper 16 bits must be unused for a hardware raw event.
-		 * As per SBI v3.0 specification,
-		 *  -- the upper 8 bits must be unused for a hardware raw event.
-		 * Bits 63:62 are used to distinguish between raw events
-		 * 00 - Hardware raw event
-		 * 10 - SBI firmware events
-		 * 11 - Risc-V platform specific firmware event
-		 */
-
-		switch (config >> 62) {
-		case 0:
-			if (sbi_v3_available) {
-			/* Return error any bits [56-63] is set  as it is not allowed by the spec */
-				if (!(config & ~RISCV_PMU_RAW_EVENT_V2_MASK)) {
-					*econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
-					ret = RISCV_PMU_RAW_EVENT_V2_IDX;
-				}
-			/* Return error any bits [48-63] is set  as it is not allowed by the spec */
-			} else if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
-				*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
-				ret = RISCV_PMU_RAW_EVENT_IDX;
-			}
-			break;
-		case 2:
-			ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
-			break;
-		case 3:
-			/*
-			 * For Risc-V platform specific firmware events
-			 * Event code - 0xFFFF
-			 * Event data - raw event encoding
-			 */
-			ret = SBI_PMU_EVENT_TYPE_FW << 16 | RISCV_PLAT_FW_EVENT;
-			*econfig = config & RISCV_PMU_PLAT_FW_EVENT_MASK;
-			break;
-		default:
-			break;
-		}
-		break;
-	default:
-		break;
-	}
-
-	return ret;
+	return riscv_pmu_get_event_info(type, config, econfig);
 }
 
 static void pmu_sbi_snapshot_free(struct riscv_pmu *pmu)
diff --git a/include/linux/perf/riscv_pmu.h b/include/linux/perf/riscv_pmu.h
index 701974639ff2..4a5e3209c473 100644
--- a/include/linux/perf/riscv_pmu.h
+++ b/include/linux/perf/riscv_pmu.h
@@ -91,6 +91,8 @@ struct riscv_pmu *riscv_pmu_alloc(void);
 int riscv_pmu_get_hpm_info(u32 *hw_ctr_width, u32 *num_hw_ctr);
 #endif
 
+int riscv_pmu_get_event_info(u32 type, u64 config, u64 *econfig);
+
 #endif /* CONFIG_RISCV_PMU */
 
 #endif /* _RISCV_PMU_H */

-- 
2.43.0

Re: [PATCH v3 5/9] drivers/perf: riscv: Export PMU event info function

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> The event mapping function can be used in event info function to find out
> the corresponding SBI PMU event encoding during the get_event_info function
> as well. Refactor and export it so that it can be invoked from kvm and
> internal driver.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  drivers/perf/riscv_pmu_sbi.c   | 124 ++++++++++++++++++++++-------------------
>  include/linux/perf/riscv_pmu.h |   2 +
>  2 files changed, 69 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 33d8348bf68a..f5d3db6dba18 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -100,6 +100,7 @@ static unsigned int riscv_pmu_irq;
>  /* Cache the available counters in a bitmask */
>  static unsigned long cmask;
>
> +static int pmu_event_find_cache(u64 config);
>  struct sbi_pmu_event_data {
>         union {
>                 union {
> @@ -411,6 +412,71 @@ static bool pmu_sbi_ctr_is_fw(int cidx)
>         return (info->type == SBI_PMU_CTR_TYPE_FW) ? true : false;
>  }
>
> +int riscv_pmu_get_event_info(u32 type, u64 config, u64 *econfig)
> +{
> +       int ret = -ENOENT;
> +
> +       switch (type) {
> +       case PERF_TYPE_HARDWARE:
> +               if (config >= PERF_COUNT_HW_MAX)
> +                       return -EINVAL;
> +               ret = pmu_hw_event_map[config].event_idx;
> +               break;
> +       case PERF_TYPE_HW_CACHE:
> +               ret = pmu_event_find_cache(config);
> +               break;
> +       case PERF_TYPE_RAW:
> +               /*
> +                * As per SBI v0.3 specification,
> +                *  -- the upper 16 bits must be unused for a hardware raw event.
> +                * As per SBI v3.0 specification,
> +                *  -- the upper 8 bits must be unused for a hardware raw event.
> +                * Bits 63:62 are used to distinguish between raw events
> +                * 00 - Hardware raw event
> +                * 10 - SBI firmware events
> +                * 11 - Risc-V platform specific firmware event
> +                */
> +               switch (config >> 62) {
> +               case 0:
> +                       if (sbi_v3_available) {
> +                       /* Return error any bits [56-63] is set  as it is not allowed by the spec */
> +                               if (!(config & ~RISCV_PMU_RAW_EVENT_V2_MASK)) {
> +                                       if (econfig)
> +                                               *econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
> +                                       ret = RISCV_PMU_RAW_EVENT_V2_IDX;
> +                               }
> +                       /* Return error any bits [48-63] is set  as it is not allowed by the spec */
> +                       } else if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
> +                               if (econfig)
> +                                       *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> +                               ret = RISCV_PMU_RAW_EVENT_IDX;
> +                       }
> +                       break;
> +               case 2:
> +                       ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
> +                       break;
> +               case 3:
> +                       /*
> +                        * For Risc-V platform specific firmware events
> +                        * Event code - 0xFFFF
> +                        * Event data - raw event encoding
> +                        */
> +                       ret = SBI_PMU_EVENT_TYPE_FW << 16 | RISCV_PLAT_FW_EVENT;
> +                       if (econfig)
> +                               *econfig = config & RISCV_PMU_PLAT_FW_EVENT_MASK;
> +                       break;
> +               default:
> +                       break;
> +               }
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(riscv_pmu_get_event_info);
> +
>  /*
>   * Returns the counter width of a programmable counter and number of hardware
>   * counters. As we don't support heterogeneous CPUs yet, it is okay to just
> @@ -576,7 +642,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>  {
>         u32 type = event->attr.type;
>         u64 config = event->attr.config;
> -       int ret = -ENOENT;
>
>         /*
>          * Ensure we are finished checking standard hardware events for
> @@ -584,62 +649,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>          */
>         flush_work(&check_std_events_work);
>
> -       switch (type) {
> -       case PERF_TYPE_HARDWARE:
> -               if (config >= PERF_COUNT_HW_MAX)
> -                       return -EINVAL;
> -               ret = pmu_hw_event_map[event->attr.config].event_idx;
> -               break;
> -       case PERF_TYPE_HW_CACHE:
> -               ret = pmu_event_find_cache(config);
> -               break;
> -       case PERF_TYPE_RAW:
> -               /*
> -                * As per SBI v0.3 specification,
> -                *  -- the upper 16 bits must be unused for a hardware raw event.
> -                * As per SBI v3.0 specification,
> -                *  -- the upper 8 bits must be unused for a hardware raw event.
> -                * Bits 63:62 are used to distinguish between raw events
> -                * 00 - Hardware raw event
> -                * 10 - SBI firmware events
> -                * 11 - Risc-V platform specific firmware event
> -                */
> -
> -               switch (config >> 62) {
> -               case 0:
> -                       if (sbi_v3_available) {
> -                       /* Return error any bits [56-63] is set  as it is not allowed by the spec */
> -                               if (!(config & ~RISCV_PMU_RAW_EVENT_V2_MASK)) {
> -                                       *econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
> -                                       ret = RISCV_PMU_RAW_EVENT_V2_IDX;
> -                               }
> -                       /* Return error any bits [48-63] is set  as it is not allowed by the spec */
> -                       } else if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
> -                               *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> -                               ret = RISCV_PMU_RAW_EVENT_IDX;
> -                       }
> -                       break;
> -               case 2:
> -                       ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
> -                       break;
> -               case 3:
> -                       /*
> -                        * For Risc-V platform specific firmware events
> -                        * Event code - 0xFFFF
> -                        * Event data - raw event encoding
> -                        */
> -                       ret = SBI_PMU_EVENT_TYPE_FW << 16 | RISCV_PLAT_FW_EVENT;
> -                       *econfig = config & RISCV_PMU_PLAT_FW_EVENT_MASK;
> -                       break;
> -               default:
> -                       break;
> -               }
> -               break;
> -       default:
> -               break;
> -       }
> -
> -       return ret;
> +       return riscv_pmu_get_event_info(type, config, econfig);
>  }
>
>  static void pmu_sbi_snapshot_free(struct riscv_pmu *pmu)
> diff --git a/include/linux/perf/riscv_pmu.h b/include/linux/perf/riscv_pmu.h
> index 701974639ff2..4a5e3209c473 100644
> --- a/include/linux/perf/riscv_pmu.h
> +++ b/include/linux/perf/riscv_pmu.h
> @@ -91,6 +91,8 @@ struct riscv_pmu *riscv_pmu_alloc(void);
>  int riscv_pmu_get_hpm_info(u32 *hw_ctr_width, u32 *num_hw_ctr);
>  #endif
>
> +int riscv_pmu_get_event_info(u32 type, u64 config, u64 *econfig);
> +
>  #endif /* CONFIG_RISCV_PMU */

We will see compile/link errors for users of riscv_pmu_get_event_info()
if CONFIG_RISCV_PMU_SBI is not defined. Am I missing anything

>
>  #endif /* _RISCV_PMU_H */
>
> --
> 2.43.0
>

Regards,
Anup

[PATCH v3 6/9] KVM: Add a helper function to validate vcpu gpa range

[Atish Patra]

The arch specific code may need to validate a gpa range if it is a shared
memory between the host and the guest. Currently, there are few places
where it is used in RISC-V implementation. Given the nature of the function
it may be used for other architectures. Hence, a common helper function
is added.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 include/linux/kvm_host.h |  2 ++
 virt/kvm/kvm_main.c      | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 291d49b9bf05..adda61cc4072 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1383,6 +1383,8 @@ static inline int kvm_vcpu_map_readonly(struct kvm_vcpu *vcpu, gpa_t gpa,
 
 unsigned long kvm_vcpu_gfn_to_hva(struct kvm_vcpu *vcpu, gfn_t gfn);
 unsigned long kvm_vcpu_gfn_to_hva_prot(struct kvm_vcpu *vcpu, gfn_t gfn, bool *writable);
+int kvm_vcpu_validate_gpa_range(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned long len,
+				bool write_access);
 int kvm_vcpu_read_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, void *data, int offset,
 			     int len);
 int kvm_vcpu_read_guest_atomic(struct kvm_vcpu *vcpu, gpa_t gpa, void *data,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e85b33a92624..3f52f5571fa6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3301,6 +3301,27 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest);
 
+int kvm_vcpu_validate_gpa_range(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned long len,
+				bool write_access)
+{
+	gfn_t gfn = gpa >> PAGE_SHIFT;
+	int seg;
+	int offset = offset_in_page(gpa);
+	bool writable = false;
+	unsigned long hva;
+
+	while ((seg = next_segment(len, offset)) != 0) {
+		hva = kvm_vcpu_gfn_to_hva_prot(vcpu, gfn, &writable);
+		if (kvm_is_error_hva(hva) || (writable ^ write_access))
+			return -EPERM;
+		offset = 0;
+		len -= seg;
+		++gfn;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kvm_vcpu_validate_gpa_range);
+
 static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
 				       struct gfn_to_hva_cache *ghc,
 				       gpa_t gpa, unsigned long len)

-- 
2.43.0

Re: [PATCH v3 6/9] KVM: Add a helper function to validate vcpu gpa range

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> The arch specific code may need to validate a gpa range if it is a shared
> memory between the host and the guest. Currently, there are few places
> where it is used in RISC-V implementation. Given the nature of the function
> it may be used for other architectures. Hence, a common helper function
> is added.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  include/linux/kvm_host.h |  2 ++
>  virt/kvm/kvm_main.c      | 21 +++++++++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 291d49b9bf05..adda61cc4072 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1383,6 +1383,8 @@ static inline int kvm_vcpu_map_readonly(struct kvm_vcpu *vcpu, gpa_t gpa,
>
>  unsigned long kvm_vcpu_gfn_to_hva(struct kvm_vcpu *vcpu, gfn_t gfn);
>  unsigned long kvm_vcpu_gfn_to_hva_prot(struct kvm_vcpu *vcpu, gfn_t gfn, bool *writable);
> +int kvm_vcpu_validate_gpa_range(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned long len,
> +                               bool write_access);
>  int kvm_vcpu_read_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, void *data, int offset,
>                              int len);
>  int kvm_vcpu_read_guest_atomic(struct kvm_vcpu *vcpu, gpa_t gpa, void *data,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e85b33a92624..3f52f5571fa6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3301,6 +3301,27 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest);
>
> +int kvm_vcpu_validate_gpa_range(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned long len,
> +                               bool write_access)
> +{
> +       gfn_t gfn = gpa >> PAGE_SHIFT;
> +       int seg;
> +       int offset = offset_in_page(gpa);
> +       bool writable = false;
> +       unsigned long hva;

Nit: rearrange the variables in a inverted pyramid shape.

> +
> +       while ((seg = next_segment(len, offset)) != 0) {
> +               hva = kvm_vcpu_gfn_to_hva_prot(vcpu, gfn, &writable);
> +               if (kvm_is_error_hva(hva) || (writable ^ write_access))
> +                       return -EPERM;
> +               offset = 0;
> +               len -= seg;
> +               ++gfn;
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_vcpu_validate_gpa_range);
> +
>  static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
>                                        struct gfn_to_hva_cache *ghc,
>                                        gpa_t gpa, unsigned long len)
>
> --
> 2.43.0
>

Otherwise, looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

Re: [PATCH v3 6/9] KVM: Add a helper function to validate vcpu gpa range

[Anup Patel]

Hi Paolo,

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> The arch specific code may need to validate a gpa range if it is a shared
> memory between the host and the guest. Currently, there are few places
> where it is used in RISC-V implementation. Given the nature of the function
> it may be used for other architectures. Hence, a common helper function
> is added.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  include/linux/kvm_host.h |  2 ++
>  virt/kvm/kvm_main.c      | 21 +++++++++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 291d49b9bf05..adda61cc4072 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1383,6 +1383,8 @@ static inline int kvm_vcpu_map_readonly(struct kvm_vcpu *vcpu, gpa_t gpa,
>
>  unsigned long kvm_vcpu_gfn_to_hva(struct kvm_vcpu *vcpu, gfn_t gfn);
>  unsigned long kvm_vcpu_gfn_to_hva_prot(struct kvm_vcpu *vcpu, gfn_t gfn, bool *writable);
> +int kvm_vcpu_validate_gpa_range(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned long len,
> +                               bool write_access);
>  int kvm_vcpu_read_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn, void *data, int offset,
>                              int len);
>  int kvm_vcpu_read_guest_atomic(struct kvm_vcpu *vcpu, gpa_t gpa, void *data,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e85b33a92624..3f52f5571fa6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3301,6 +3301,27 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest);
>
> +int kvm_vcpu_validate_gpa_range(struct kvm_vcpu *vcpu, gpa_t gpa, unsigned long len,
> +                               bool write_access)
> +{
> +       gfn_t gfn = gpa >> PAGE_SHIFT;
> +       int seg;
> +       int offset = offset_in_page(gpa);
> +       bool writable = false;
> +       unsigned long hva;
> +
> +       while ((seg = next_segment(len, offset)) != 0) {
> +               hva = kvm_vcpu_gfn_to_hva_prot(vcpu, gfn, &writable);
> +               if (kvm_is_error_hva(hva) || (writable ^ write_access))
> +                       return -EPERM;
> +               offset = 0;
> +               len -= seg;
> +               ++gfn;
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_vcpu_validate_gpa_range);
> +

Can you please review this common helper since it is in virt/kvm ?

>  static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
>                                        struct gfn_to_hva_cache *ghc,
>                                        gpa_t gpa, unsigned long len)
>
> --
> 2.43.0
>

Regards,
Anup

[PATCH v3 7/9] RISC-V: KVM: Use the new gpa range validate helper function

[Atish Patra]

Remove the duplicate code and use the new helper function to validate
the shared memory gpa address.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/kvm/vcpu_pmu.c     | 5 +----
 arch/riscv/kvm/vcpu_sbi_sta.c | 6 ++----
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
index 15d71a7b75ba..163bd4403fd0 100644
--- a/arch/riscv/kvm/vcpu_pmu.c
+++ b/arch/riscv/kvm/vcpu_pmu.c
@@ -409,8 +409,6 @@ int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long s
 	int snapshot_area_size = sizeof(struct riscv_pmu_snapshot_data);
 	int sbiret = 0;
 	gpa_t saddr;
-	unsigned long hva;
-	bool writable;
 
 	if (!kvpmu || flags) {
 		sbiret = SBI_ERR_INVALID_PARAM;
@@ -432,8 +430,7 @@ int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long s
 		goto out;
 	}
 
-	hva = kvm_vcpu_gfn_to_hva_prot(vcpu, saddr >> PAGE_SHIFT, &writable);
-	if (kvm_is_error_hva(hva) || !writable) {
+	if (kvm_vcpu_validate_gpa_range(vcpu, saddr, PAGE_SIZE, true)) {
 		sbiret = SBI_ERR_INVALID_ADDRESS;
 		goto out;
 	}
diff --git a/arch/riscv/kvm/vcpu_sbi_sta.c b/arch/riscv/kvm/vcpu_sbi_sta.c
index 5f35427114c1..67dfb613df6a 100644
--- a/arch/riscv/kvm/vcpu_sbi_sta.c
+++ b/arch/riscv/kvm/vcpu_sbi_sta.c
@@ -85,8 +85,6 @@ static int kvm_sbi_sta_steal_time_set_shmem(struct kvm_vcpu *vcpu)
 	unsigned long shmem_phys_hi = cp->a1;
 	u32 flags = cp->a2;
 	struct sbi_sta_struct zero_sta = {0};
-	unsigned long hva;
-	bool writable;
 	gpa_t shmem;
 	int ret;
 
@@ -111,8 +109,8 @@ static int kvm_sbi_sta_steal_time_set_shmem(struct kvm_vcpu *vcpu)
 			return SBI_ERR_INVALID_ADDRESS;
 	}
 
-	hva = kvm_vcpu_gfn_to_hva_prot(vcpu, shmem >> PAGE_SHIFT, &writable);
-	if (kvm_is_error_hva(hva) || !writable)
+	/* The spec requires the shmem to be 64-byte aligned. */
+	if (kvm_vcpu_validate_gpa_range(vcpu, shmem, 64, true))
 		return SBI_ERR_INVALID_ADDRESS;
 
 	ret = kvm_vcpu_write_guest(vcpu, shmem, &zero_sta, sizeof(zero_sta));

-- 
2.43.0

Re: [PATCH v3 7/9] RISC-V: KVM: Use the new gpa range validate helper function

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> Remove the duplicate code and use the new helper function to validate
> the shared memory gpa address.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/kvm/vcpu_pmu.c     | 5 +----
>  arch/riscv/kvm/vcpu_sbi_sta.c | 6 ++----
>  2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 15d71a7b75ba..163bd4403fd0 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -409,8 +409,6 @@ int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long s
>         int snapshot_area_size = sizeof(struct riscv_pmu_snapshot_data);
>         int sbiret = 0;
>         gpa_t saddr;
> -       unsigned long hva;
> -       bool writable;
>
>         if (!kvpmu || flags) {
>                 sbiret = SBI_ERR_INVALID_PARAM;
> @@ -432,8 +430,7 @@ int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long s
>                 goto out;
>         }
>
> -       hva = kvm_vcpu_gfn_to_hva_prot(vcpu, saddr >> PAGE_SHIFT, &writable);
> -       if (kvm_is_error_hva(hva) || !writable) {
> +       if (kvm_vcpu_validate_gpa_range(vcpu, saddr, PAGE_SIZE, true)) {
>                 sbiret = SBI_ERR_INVALID_ADDRESS;
>                 goto out;
>         }
> diff --git a/arch/riscv/kvm/vcpu_sbi_sta.c b/arch/riscv/kvm/vcpu_sbi_sta.c
> index 5f35427114c1..67dfb613df6a 100644
> --- a/arch/riscv/kvm/vcpu_sbi_sta.c
> +++ b/arch/riscv/kvm/vcpu_sbi_sta.c
> @@ -85,8 +85,6 @@ static int kvm_sbi_sta_steal_time_set_shmem(struct kvm_vcpu *vcpu)
>         unsigned long shmem_phys_hi = cp->a1;
>         u32 flags = cp->a2;
>         struct sbi_sta_struct zero_sta = {0};
> -       unsigned long hva;
> -       bool writable;
>         gpa_t shmem;
>         int ret;
>
> @@ -111,8 +109,8 @@ static int kvm_sbi_sta_steal_time_set_shmem(struct kvm_vcpu *vcpu)
>                         return SBI_ERR_INVALID_ADDRESS;
>         }
>
> -       hva = kvm_vcpu_gfn_to_hva_prot(vcpu, shmem >> PAGE_SHIFT, &writable);
> -       if (kvm_is_error_hva(hva) || !writable)
> +       /* The spec requires the shmem to be 64-byte aligned. */
> +       if (kvm_vcpu_validate_gpa_range(vcpu, shmem, 64, true))
>                 return SBI_ERR_INVALID_ADDRESS;
>
>         ret = kvm_vcpu_write_guest(vcpu, shmem, &zero_sta, sizeof(zero_sta));
>
> --
> 2.43.0
>

[PATCH v3 8/9] RISC-V: KVM: Implement get event info function

[Atish Patra]

The new get_event_info funciton allows the guest to query the presence
of multiple events with single SBI call. Currently, the perf driver
in linux guest invokes it for all the standard SBI PMU events. Support
the SBI function implementation in KVM as well.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/kvm_vcpu_pmu.h |  3 ++
 arch/riscv/kvm/vcpu_pmu.c             | 66 +++++++++++++++++++++++++++++++++++
 arch/riscv/kvm/vcpu_sbi_pmu.c         |  3 ++
 3 files changed, 72 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_vcpu_pmu.h b/arch/riscv/include/asm/kvm_vcpu_pmu.h
index 1d85b6617508..9a930afc8f57 100644
--- a/arch/riscv/include/asm/kvm_vcpu_pmu.h
+++ b/arch/riscv/include/asm/kvm_vcpu_pmu.h
@@ -98,6 +98,9 @@ void kvm_riscv_vcpu_pmu_init(struct kvm_vcpu *vcpu);
 int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long saddr_low,
 				      unsigned long saddr_high, unsigned long flags,
 				      struct kvm_vcpu_sbi_return *retdata);
+int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low,
+				  unsigned long saddr_high, unsigned long num_events,
+				  unsigned long flags, struct kvm_vcpu_sbi_return *retdata);
 void kvm_riscv_vcpu_pmu_deinit(struct kvm_vcpu *vcpu);
 void kvm_riscv_vcpu_pmu_reset(struct kvm_vcpu *vcpu);
 
diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
index 163bd4403fd0..70a6bdfc42f5 100644
--- a/arch/riscv/kvm/vcpu_pmu.c
+++ b/arch/riscv/kvm/vcpu_pmu.c
@@ -453,6 +453,72 @@ int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long s
 	return 0;
 }
 
+int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low,
+				  unsigned long saddr_high, unsigned long num_events,
+				  unsigned long flags, struct kvm_vcpu_sbi_return *retdata)
+{
+	struct riscv_pmu_event_info *einfo;
+	int shmem_size = num_events * sizeof(*einfo);
+	gpa_t shmem;
+	u32 eidx, etype;
+	u64 econfig;
+	int ret;
+
+	if (flags != 0 || (saddr_low & (SZ_16 - 1))) {
+		ret = SBI_ERR_INVALID_PARAM;
+		goto out;
+	}
+
+	shmem = saddr_low;
+	if (saddr_high != 0) {
+		if (IS_ENABLED(CONFIG_32BIT)) {
+			shmem |= ((gpa_t)saddr_high << 32);
+		} else {
+			ret = SBI_ERR_INVALID_ADDRESS;
+			goto out;
+		}
+	}
+
+	if (kvm_vcpu_validate_gpa_range(vcpu, shmem, shmem_size, true)) {
+		ret = SBI_ERR_INVALID_ADDRESS;
+		goto out;
+	}
+
+	einfo = kzalloc(shmem_size, GFP_KERNEL);
+	if (!einfo)
+		return -ENOMEM;
+
+	ret = kvm_vcpu_read_guest(vcpu, shmem, einfo, shmem_size);
+	if (ret) {
+		ret = SBI_ERR_FAILURE;
+		goto free_mem;
+	}
+
+	for (int i = 0; i < num_events; i++) {
+		eidx = einfo[i].event_idx;
+		etype = kvm_pmu_get_perf_event_type(eidx);
+		econfig = kvm_pmu_get_perf_event_config(eidx, einfo[i].event_data);
+		ret = riscv_pmu_get_event_info(etype, econfig, NULL);
+		if (ret > 0)
+			einfo[i].output = 1;
+		else
+			einfo[i].output = 0;
+	}
+
+	kvm_vcpu_write_guest(vcpu, shmem, einfo, shmem_size);
+	if (ret) {
+		ret = SBI_ERR_FAILURE;
+		goto free_mem;
+	}
+
+free_mem:
+	kfree(einfo);
+out:
+	retdata->err_val = ret;
+
+	return 0;
+}
+
 int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu,
 				struct kvm_vcpu_sbi_return *retdata)
 {
diff --git a/arch/riscv/kvm/vcpu_sbi_pmu.c b/arch/riscv/kvm/vcpu_sbi_pmu.c
index e4be34e03e83..a020d979d179 100644
--- a/arch/riscv/kvm/vcpu_sbi_pmu.c
+++ b/arch/riscv/kvm/vcpu_sbi_pmu.c
@@ -73,6 +73,9 @@ static int kvm_sbi_ext_pmu_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	case SBI_EXT_PMU_SNAPSHOT_SET_SHMEM:
 		ret = kvm_riscv_vcpu_pmu_snapshot_set_shmem(vcpu, cp->a0, cp->a1, cp->a2, retdata);
 		break;
+	case SBI_EXT_PMU_EVENT_GET_INFO:
+		ret = kvm_riscv_vcpu_pmu_event_info(vcpu, cp->a0, cp->a1, cp->a2, cp->a3, retdata);
+		break;
 	default:
 		retdata->err_val = SBI_ERR_NOT_SUPPORTED;
 	}

-- 
2.43.0

Re: [PATCH v3 8/9] RISC-V: KVM: Implement get event info function

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> The new get_event_info funciton allows the guest to query the presence
> of multiple events with single SBI call. Currently, the perf driver
> in linux guest invokes it for all the standard SBI PMU events. Support
> the SBI function implementation in KVM as well.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/include/asm/kvm_vcpu_pmu.h |  3 ++
>  arch/riscv/kvm/vcpu_pmu.c             | 66 +++++++++++++++++++++++++++++++++++
>  arch/riscv/kvm/vcpu_sbi_pmu.c         |  3 ++
>  3 files changed, 72 insertions(+)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_pmu.h b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> index 1d85b6617508..9a930afc8f57 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_pmu.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> @@ -98,6 +98,9 @@ void kvm_riscv_vcpu_pmu_init(struct kvm_vcpu *vcpu);
>  int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long saddr_low,
>                                       unsigned long saddr_high, unsigned long flags,
>                                       struct kvm_vcpu_sbi_return *retdata);
> +int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low,
> +                                 unsigned long saddr_high, unsigned long num_events,
> +                                 unsigned long flags, struct kvm_vcpu_sbi_return *retdata);
>  void kvm_riscv_vcpu_pmu_deinit(struct kvm_vcpu *vcpu);
>  void kvm_riscv_vcpu_pmu_reset(struct kvm_vcpu *vcpu);
>
> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 163bd4403fd0..70a6bdfc42f5 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -453,6 +453,72 @@ int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long s
>         return 0;
>  }
>
> +int kvm_riscv_vcpu_pmu_event_info(struct kvm_vcpu *vcpu, unsigned long saddr_low,
> +                                 unsigned long saddr_high, unsigned long num_events,
> +                                 unsigned long flags, struct kvm_vcpu_sbi_return *retdata)
> +{
> +       struct riscv_pmu_event_info *einfo;
> +       int shmem_size = num_events * sizeof(*einfo);
> +       gpa_t shmem;
> +       u32 eidx, etype;
> +       u64 econfig;
> +       int ret;
> +
> +       if (flags != 0 || (saddr_low & (SZ_16 - 1))) {
> +               ret = SBI_ERR_INVALID_PARAM;
> +               goto out;
> +       }
> +
> +       shmem = saddr_low;
> +       if (saddr_high != 0) {
> +               if (IS_ENABLED(CONFIG_32BIT)) {
> +                       shmem |= ((gpa_t)saddr_high << 32);
> +               } else {
> +                       ret = SBI_ERR_INVALID_ADDRESS;
> +                       goto out;
> +               }
> +       }
> +
> +       if (kvm_vcpu_validate_gpa_range(vcpu, shmem, shmem_size, true)) {
> +               ret = SBI_ERR_INVALID_ADDRESS;
> +               goto out;
> +       }
> +
> +       einfo = kzalloc(shmem_size, GFP_KERNEL);
> +       if (!einfo)
> +               return -ENOMEM;
> +
> +       ret = kvm_vcpu_read_guest(vcpu, shmem, einfo, shmem_size);
> +       if (ret) {
> +               ret = SBI_ERR_FAILURE;
> +               goto free_mem;
> +       }
> +
> +       for (int i = 0; i < num_events; i++) {
> +               eidx = einfo[i].event_idx;
> +               etype = kvm_pmu_get_perf_event_type(eidx);
> +               econfig = kvm_pmu_get_perf_event_config(eidx, einfo[i].event_data);
> +               ret = riscv_pmu_get_event_info(etype, econfig, NULL);
> +               if (ret > 0)
> +                       einfo[i].output = 1;
> +               else
> +                       einfo[i].output = 0;
> +       }
> +
> +       kvm_vcpu_write_guest(vcpu, shmem, einfo, shmem_size);
> +       if (ret) {
> +               ret = SBI_ERR_FAILURE;
> +               goto free_mem;
> +       }
> +
> +free_mem:
> +       kfree(einfo);
> +out:
> +       retdata->err_val = ret;
> +
> +       return 0;
> +}
> +
>  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu,
>                                 struct kvm_vcpu_sbi_return *retdata)
>  {
> diff --git a/arch/riscv/kvm/vcpu_sbi_pmu.c b/arch/riscv/kvm/vcpu_sbi_pmu.c
> index e4be34e03e83..a020d979d179 100644
> --- a/arch/riscv/kvm/vcpu_sbi_pmu.c
> +++ b/arch/riscv/kvm/vcpu_sbi_pmu.c
> @@ -73,6 +73,9 @@ static int kvm_sbi_ext_pmu_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
>         case SBI_EXT_PMU_SNAPSHOT_SET_SHMEM:
>                 ret = kvm_riscv_vcpu_pmu_snapshot_set_shmem(vcpu, cp->a0, cp->a1, cp->a2, retdata);
>                 break;
> +       case SBI_EXT_PMU_EVENT_GET_INFO:
> +               ret = kvm_riscv_vcpu_pmu_event_info(vcpu, cp->a0, cp->a1, cp->a2, cp->a3, retdata);
> +               break;
>         default:
>                 retdata->err_val = SBI_ERR_NOT_SUPPORTED;
>         }
>
> --
> 2.43.0
>

[PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Atish Patra]

Upgrade the SBI version to v3.0 so that corresponding features
can be enabled in the guest.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index 4ed6203cdd30..194299e0ab0e 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -11,7 +11,7 @@
 
 #define KVM_SBI_IMPID 3
 
-#define KVM_SBI_VERSION_MAJOR 2
+#define KVM_SBI_VERSION_MAJOR 3
 #define KVM_SBI_VERSION_MINOR 0
 
 enum kvm_riscv_sbi_ext_status {

-- 
2.43.0

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Radim Krčmář]

2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
> Upgrade the SBI version to v3.0 so that corresponding features
> can be enabled in the guest.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> -#define KVM_SBI_VERSION_MAJOR 2
> +#define KVM_SBI_VERSION_MAJOR 3

I think it's time to add versioning to KVM SBI implementation.
Userspace should be able to select the desired SBI version and KVM would
tell the guest that newer features are not supported.

We could somewhat get away with the userspace_sbi patch I posted,
because userspace would at least be in control of the SBI version, but
it would still be incorrect without a KVM enforcement, because a
misbehaving guest could use features that should not be supported.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Atish Patra]

On 5/23/25 6:31 AM, Radim Krčmář wrote:
> 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
>> Upgrade the SBI version to v3.0 so that corresponding features
>> can be enabled in the guest.
>>
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> ---
>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>> -#define KVM_SBI_VERSION_MAJOR 2
>> +#define KVM_SBI_VERSION_MAJOR 3
> I think it's time to add versioning to KVM SBI implementation.
> Userspace should be able to select the desired SBI version and KVM would
> tell the guest that newer features are not supported.

We can achieve that through onereg interface by disabling individual SBI 
extensions.
We can extend the existing onereg interface to disable a specific SBI 
version directly
instead of individual ones to save those IOCTL as well.

> We could somewhat get away with the userspace_sbi patch I posted,
> because userspace would at least be in control of the SBI version, but
> it would still be incorrect without a KVM enforcement, because a
> misbehaving guest could use features that should not be supported.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Radim Krčmář]

2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
> On 5/23/25 6:31 AM, Radim Krčmář wrote:
>> 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
>>> Upgrade the SBI version to v3.0 so that corresponding features
>>> can be enabled in the guest.
>>>
>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>> ---
>>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>>> -#define KVM_SBI_VERSION_MAJOR 2
>>> +#define KVM_SBI_VERSION_MAJOR 3
>> I think it's time to add versioning to KVM SBI implementation.
>> Userspace should be able to select the desired SBI version and KVM would
>> tell the guest that newer features are not supported.
>
> We can achieve that through onereg interface by disabling individual SBI 
> extensions.
> We can extend the existing onereg interface to disable a specific SBI 
> version directly
> instead of individual ones to save those IOCTL as well.

Yes, I am all in favor of letting userspace provide all values in the
BASE extension.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Andrew Jones]

On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
> 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
> > On 5/23/25 6:31 AM, Radim Krčmář wrote:
> >> 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
> >>> Upgrade the SBI version to v3.0 so that corresponding features
> >>> can be enabled in the guest.
> >>>
> >>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>> ---
> >>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> >>> -#define KVM_SBI_VERSION_MAJOR 2
> >>> +#define KVM_SBI_VERSION_MAJOR 3
> >> I think it's time to add versioning to KVM SBI implementation.
> >> Userspace should be able to select the desired SBI version and KVM would
> >> tell the guest that newer features are not supported.

We need new code for this, but it's a good idea.

> >
> > We can achieve that through onereg interface by disabling individual SBI 
> > extensions.
> > We can extend the existing onereg interface to disable a specific SBI 
> > version directly
> > instead of individual ones to save those IOCTL as well.
> 
> Yes, I am all in favor of letting userspace provide all values in the
> BASE extension.

This is covered by your recent patch that provides userspace_sbi.
With that, userspace can disable all extensions that aren't
supported by a given spec version, disable BASE and then provide
a BASE that advertises the version it wants. The new code is needed
for extensions that userspace still wants KVM to accelerate, but then
KVM needs to be informed it should deny all functions not included in
the selected spec version.

Thanks,
drew

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Atish Patra]

On 5/26/25 4:13 AM, Andrew Jones wrote:
> On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
>> 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
>>> On 5/23/25 6:31 AM, Radim Krčmář wrote:
>>>> 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
>>>>> Upgrade the SBI version to v3.0 so that corresponding features
>>>>> can be enabled in the guest.
>>>>>
>>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>>> ---
>>>>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>>>>> -#define KVM_SBI_VERSION_MAJOR 2
>>>>> +#define KVM_SBI_VERSION_MAJOR 3
>>>> I think it's time to add versioning to KVM SBI implementation.
>>>> Userspace should be able to select the desired SBI version and KVM would
>>>> tell the guest that newer features are not supported.
> 
> We need new code for this, but it's a good idea.
> 
>>>
>>> We can achieve that through onereg interface by disabling individual SBI
>>> extensions.
>>> We can extend the existing onereg interface to disable a specific SBI
>>> version directly
>>> instead of individual ones to save those IOCTL as well.
>>
>> Yes, I am all in favor of letting userspace provide all values in the
>> BASE extension.
> 

We already support vendorid/archid/impid through one reg. I think we 
just need to add the SBI version support to that so that user space can 
set it.

> This is covered by your recent patch that provides userspace_sbi.

Why do we need to invent new IOCTL for this ? Once the user space sets 
the SBI version, KVM can enforce it.

> With that, userspace can disable all extensions that aren't
> supported by a given spec version, disable BASE and then provide
> a BASE that advertises the version it wants. The new code is needed
> for extensions that userspace still wants KVM to accelerate, but then
> KVM needs to be informed it should deny all functions not included in
> the selected spec version.
> 
> Thanks,
> drew
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Andrew Jones]

On Wed, May 28, 2025 at 07:16:11AM -0700, Atish Patra wrote:
> On 5/26/25 4:13 AM, Andrew Jones wrote:
> > On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
> > > 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
> > > > On 5/23/25 6:31 AM, Radim Krčmář wrote:
> > > > > 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
> > > > > > Upgrade the SBI version to v3.0 so that corresponding features
> > > > > > can be enabled in the guest.
> > > > > > 
> > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > > ---
> > > > > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > > -#define KVM_SBI_VERSION_MAJOR 2
> > > > > > +#define KVM_SBI_VERSION_MAJOR 3
> > > > > I think it's time to add versioning to KVM SBI implementation.
> > > > > Userspace should be able to select the desired SBI version and KVM would
> > > > > tell the guest that newer features are not supported.
> > 
> > We need new code for this, but it's a good idea.
> > 
> > > > 
> > > > We can achieve that through onereg interface by disabling individual SBI
> > > > extensions.
> > > > We can extend the existing onereg interface to disable a specific SBI
> > > > version directly
> > > > instead of individual ones to save those IOCTL as well.
> > > 
> > > Yes, I am all in favor of letting userspace provide all values in the
> > > BASE extension.
> > 
> 
> We already support vendorid/archid/impid through one reg. I think we just
> need to add the SBI version support to that so that user space can set it.
> 
> > This is covered by your recent patch that provides userspace_sbi.
> 
> Why do we need to invent new IOCTL for this ? Once the user space sets the
> SBI version, KVM can enforce it.

If an SBI spec version provides an extension that can be emulated by
userspace, then userspace could choose to advertise that spec version,
implement a BASE probe function that advertises the extension, and
implement the extension, even if the KVM version running is older
and unaware of it. But, in order to do that, we need KVM to exit to
userspace for all unknown SBI calls and to allow BASE to be overridden
by userspace. The new KVM CAP ioctl allows opting into that new behavior.

The old KVM with new VMM configuration isn't totally far-fetched. While
host kernels tend to get updated regularly to include security fixes,
enterprise kernels tend to stop adding features at some point in order
to maximize stability. While enterprise VMMs would also eventually stop
adding features, enterprise consumers are always free to use their own
VMMs (at their own risk). So, there's a real chance we could have
deployments with older, stable KVM where users want to enable later SBI
extensions, and, in some cases, that should be possible by just updating
the VMM -- but only if KVM is only acting as an SBI implementation
accelerator and not as a userspace SBI implementation gatekeeper.

Thanks,
drew

> 
> > With that, userspace can disable all extensions that aren't
> > supported by a given spec version, disable BASE and then provide
> > a BASE that advertises the version it wants. The new code is needed
> > for extensions that userspace still wants KVM to accelerate, but then
> > KVM needs to be informed it should deny all functions not included in
> > the selected spec version.
> > 
> > Thanks,
> > drew
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Atish Patra]

<Removing Palmer's rivos email address to avoid bouncing>

On 5/28/25 8:09 AM, Andrew Jones wrote:
> On Wed, May 28, 2025 at 07:16:11AM -0700, Atish Patra wrote:
>> On 5/26/25 4:13 AM, Andrew Jones wrote:
>>> On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
>>>> 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
>>>>> On 5/23/25 6:31 AM, Radim Krčmář wrote:
>>>>>> 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
>>>>>>> Upgrade the SBI version to v3.0 so that corresponding features
>>>>>>> can be enabled in the guest.
>>>>>>>
>>>>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>>>>> ---
>>>>>>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>>>>>>> -#define KVM_SBI_VERSION_MAJOR 2
>>>>>>> +#define KVM_SBI_VERSION_MAJOR 3
>>>>>> I think it's time to add versioning to KVM SBI implementation.
>>>>>> Userspace should be able to select the desired SBI version and KVM would
>>>>>> tell the guest that newer features are not supported.
>>> We need new code for this, but it's a good idea.
>>>
>>>>> We can achieve that through onereg interface by disabling individual SBI
>>>>> extensions.
>>>>> We can extend the existing onereg interface to disable a specific SBI
>>>>> version directly
>>>>> instead of individual ones to save those IOCTL as well.
>>>> Yes, I am all in favor of letting userspace provide all values in the
>>>> BASE extension.
>> We already support vendorid/archid/impid through one reg. I think we just
>> need to add the SBI version support to that so that user space can set it.
>>
>>> This is covered by your recent patch that provides userspace_sbi.
>> Why do we need to invent new IOCTL for this ? Once the user space sets the
>> SBI version, KVM can enforce it.
> If an SBI spec version provides an extension that can be emulated by
> userspace, then userspace could choose to advertise that spec version,
> implement a BASE probe function that advertises the extension, and
> implement the extension, even if the KVM version running is older
> and unaware of it. But, in order to do that, we need KVM to exit to
> userspace for all unknown SBI calls and to allow BASE to be overridden
You mean only the version field in BASE - Correct ?

We already support vendorid/archid/impid through one reg. I don't see the
point of overriding SBI implementation ID & version.

> by userspace. The new KVM CAP ioctl allows opting into that new behavior.

But why we need a new IOCTL for that ? We can achieve that with existing
one reg interface with improvements.

> The old KVM with new VMM configuration isn't totally far-fetched. While
> host kernels tend to get updated regularly to include security fixes,
> enterprise kernels tend to stop adding features at some point in order
> to maximize stability. While enterprise VMMs would also eventually stop
> adding features, enterprise consumers are always free to use their own
> VMMs (at their own risk). So, there's a real chance we could have

I think we are years away from that happening (if it happens). My 
suggestion was not to
try to build a world where no body lives ;). When we get to that 
scenario, the default KVM
shipped will have many extension implemented. So there won't be much 
advantage to
reimplement them in the user space. We can also take an informed 
decision at that time
if the current selective forwarding approach is better or we need to 
blindly forward any
unknown SBI calls to the user space.

> deployments with older, stable KVM where users want to enable later SBI
> extensions, and, in some cases, that should be possible by just updating
> the VMM -- but only if KVM is only acting as an SBI implementation
> accelerator and not as a userspace SBI implementation gatekeeper.

But some of the SBI extensions are so fundamental that it must be 
implemented in KVM
for various reasons pointed by Anup on other thread.

> Thanks,
> drew
>
>>> With that, userspace can disable all extensions that aren't
>>> supported by a given spec version, disable BASE and then provide
>>> a BASE that advertises the version it wants. The new code is needed
>>> for extensions that userspace still wants KVM to accelerate, but then
>>> KVM needs to be informed it should deny all functions not included in
>>> the selected spec version.
>>>
>>> Thanks,
>>> drew
>>>
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Atish Patra]

On 5/28/25 12:21 PM, Atish Patra wrote:
> <Removing Palmer's rivos email address to avoid bouncing>
> 
> On 5/28/25 8:09 AM, Andrew Jones wrote:
>> On Wed, May 28, 2025 at 07:16:11AM -0700, Atish Patra wrote:
>>> On 5/26/25 4:13 AM, Andrew Jones wrote:
>>>> On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
>>>>> 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
>>>>>> On 5/23/25 6:31 AM, Radim Krčmář wrote:
>>>>>>> 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
>>>>>>>> Upgrade the SBI version to v3.0 so that corresponding features
>>>>>>>> can be enabled in the guest.
>>>>>>>>
>>>>>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>>>>>> ---
>>>>>>>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/ 
>>>>>>>> include/asm/kvm_vcpu_sbi.h
>>>>>>>> -#define KVM_SBI_VERSION_MAJOR 2
>>>>>>>> +#define KVM_SBI_VERSION_MAJOR 3
>>>>>>> I think it's time to add versioning to KVM SBI implementation.
>>>>>>> Userspace should be able to select the desired SBI version and 
>>>>>>> KVM would
>>>>>>> tell the guest that newer features are not supported.
>>>> We need new code for this, but it's a good idea.
>>>>
>>>>>> We can achieve that through onereg interface by disabling 
>>>>>> individual SBI
>>>>>> extensions.
>>>>>> We can extend the existing onereg interface to disable a specific SBI
>>>>>> version directly
>>>>>> instead of individual ones to save those IOCTL as well.
>>>>> Yes, I am all in favor of letting userspace provide all values in the
>>>>> BASE extension.
>>> We already support vendorid/archid/impid through one reg. I think we 
>>> just
>>> need to add the SBI version support to that so that user space can 
>>> set it.
>>>
>>>> This is covered by your recent patch that provides userspace_sbi.
>>> Why do we need to invent new IOCTL for this ? Once the user space 
>>> sets the
>>> SBI version, KVM can enforce it.
>> If an SBI spec version provides an extension that can be emulated by
>> userspace, then userspace could choose to advertise that spec version,
>> implement a BASE probe function that advertises the extension, and
>> implement the extension, even if the KVM version running is older
>> and unaware of it. But, in order to do that, we need KVM to exit to
>> userspace for all unknown SBI calls and to allow BASE to be overridden
> You mean only the version field in BASE - Correct ?
> 
> We already support vendorid/archid/impid through one reg. I don't see the
> point of overriding SBI implementation ID & version.
> 
>> by userspace. The new KVM CAP ioctl allows opting into that new behavior.
> 
> But why we need a new IOCTL for that ? We can achieve that with existing
> one reg interface with improvements.
> 
>> The old KVM with new VMM configuration isn't totally far-fetched. While
>> host kernels tend to get updated regularly to include security fixes,
>> enterprise kernels tend to stop adding features at some point in order
>> to maximize stability. While enterprise VMMs would also eventually stop
>> adding features, enterprise consumers are always free to use their own
>> VMMs (at their own risk). So, there's a real chance we could have
> 
> I think we are years away from that happening (if it happens). My 
> suggestion was not to
> try to build a world where no body lives ;). When we get to that 

We also support KVM as a kernel module. So it is relatively easier to 
update the RISC-V KVM module for enterprise consumers.

> scenario, the default KVM
> shipped will have many extension implemented. So there won't be much 
> advantage to
> reimplement them in the user space. We can also take an informed 
> decision at that time
> if the current selective forwarding approach is better or we need to 
> blindly forward any
> unknown SBI calls to the user space.
> 
>> deployments with older, stable KVM where users want to enable later SBI
>> extensions, and, in some cases, that should be possible by just updating
>> the VMM -- but only if KVM is only acting as an SBI implementation
>> accelerator and not as a userspace SBI implementation gatekeeper.
> 
> But some of the SBI extensions are so fundamental that it must be 
> implemented in KVM
> for various reasons pointed by Anup on other thread.
> 
>> Thanks,
>> drew
>>
>>>> With that, userspace can disable all extensions that aren't
>>>> supported by a given spec version, disable BASE and then provide
>>>> a BASE that advertises the version it wants. The new code is needed
>>>> for extensions that userspace still wants KVM to accelerate, but then
>>>> KVM needs to be informed it should deny all functions not included in
>>>> the selected spec version.
>>>>
>>>> Thanks,
>>>> drew
>>>>
>>>> _______________________________________________
>>>> linux-riscv mailing list
>>>> linux-riscv@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Radim Krčmář]

I originally gave up on the idea, but I feel kinda bad for Drew now, so
trying again:

2025-05-28T12:21:59-07:00, Atish Patra <atish.patra@linux.dev>:
> On 5/28/25 8:09 AM, Andrew Jones wrote:
>> On Wed, May 28, 2025 at 07:16:11AM -0700, Atish Patra wrote:
>>> On 5/26/25 4:13 AM, Andrew Jones wrote:
>>>> On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
>>>>> 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
>>>>>> On 5/23/25 6:31 AM, Radim Krčmář wrote:
>>>>>>> 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
>>>>>>>> Upgrade the SBI version to v3.0 so that corresponding features
>>>>>>>> can be enabled in the guest.
>>>>>>>>
>>>>>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>>>>>> ---
>>>>>>>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>>>>>>>> -#define KVM_SBI_VERSION_MAJOR 2
>>>>>>>> +#define KVM_SBI_VERSION_MAJOR 3
>>>>>>> I think it's time to add versioning to KVM SBI implementation.
>>>>>>> Userspace should be able to select the desired SBI version and KVM would
>>>>>>> tell the guest that newer features are not supported.
>>>> We need new code for this, but it's a good idea.
>>>>
>>>>>> We can achieve that through onereg interface by disabling individual SBI
>>>>>> extensions.
>>>>>> We can extend the existing onereg interface to disable a specific SBI
>>>>>> version directly
>>>>>> instead of individual ones to save those IOCTL as well.
>>>>> Yes, I am all in favor of letting userspace provide all values in the
>>>>> BASE extension.
>>> We already support vendorid/archid/impid through one reg. I think we just
>>> need to add the SBI version support to that so that user space can set it.
>>>
>>>> This is covered by your recent patch that provides userspace_sbi.
>>> Why do we need to invent new IOCTL for this ? Once the user space sets the
>>> SBI version, KVM can enforce it.
>> If an SBI spec version provides an extension that can be emulated by
>> userspace, then userspace could choose to advertise that spec version,
>> implement a BASE probe function that advertises the extension, and
>> implement the extension, even if the KVM version running is older
>> and unaware of it. But, in order to do that, we need KVM to exit to
>> userspace for all unknown SBI calls and to allow BASE to be overridden
> You mean only the version field in BASE - Correct ?

No, "BASE probe function" is the sbi_probe_extension() ecall.

>> by userspace. The new KVM CAP ioctl allows opting into that new behavior.
>
> But why we need a new IOCTL for that ? We can achieve that with existing
> one reg interface with improvements.

It's an existing IOCTL with a new data payload, but I can easily use
ONE_REG if you want to do everything through that.

KVM doesn't really need any other IOCTL than ONE_REGs, it's just
sometimes more reasonable to use a different IOCTL, like ENABLE_CAP.

>> The old KVM with new VMM configuration isn't totally far-fetched. While
>> host kernels tend to get updated regularly to include security fixes,
>> enterprise kernels tend to stop adding features at some point in order
>> to maximize stability. While enterprise VMMs would also eventually stop
>> adding features, enterprise consumers are always free to use their own
>> VMMs (at their own risk). So, there's a real chance we could have
>
> I think we are years away from that happening (if it happens). My 
> suggestion was not to
> try to build a world where no body lives ;). When we get to that 
> scenario, the default KVM
> shipped will have many extension implemented. So there won't be much 
> advantage to
> reimplement them in the user space. We can also take an informed 
> decision at that time
> if the current selective forwarding approach is better

Please don't repeat the design of SUSP/SRST/DBCN.
Seeing them is one of the reasons why I proposed the new interface.

"Blindly" forwarding DBCN to userspace is even a minor optimization. :)

>                                                        or we need to 
> blindly forward any
> unknown SBI calls to the user space.

Yes, KVM has to do what userpace configures it to do.

I don't think that implementing unsupported SBI extensions in KVM is
important -- they should not be a hot path.

>> deployments with older, stable KVM where users want to enable later SBI
>> extensions, and, in some cases, that should be possible by just updating
>> the VMM -- but only if KVM is only acting as an SBI implementation
>> accelerator and not as a userspace SBI implementation gatekeeper.
>
> But some of the SBI extensions are so fundamental that it must be 
> implemented in KVM
> for various reasons pointed by Anup on other thread.

No, SBI does not have to be implemented in KVM at all.

We do have a deep disagreement on what is virtualization and the role of
KVM in it.  I think that userspace wants a generic ISA accelerator.

Even if userspace wants SBI for the M-mode interface, security minded
userspace aims for as little kernel code as possible.
Userspace might want to accelerate some SBI extension in KVM, but it
should not be KVM who decides what userspace wants.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Atish Patra]

On 5/29/25 3:24 AM, Radim Krčmář wrote:
> I originally gave up on the idea, but I feel kinda bad for Drew now, so
> trying again:

I am sorry if some of my replies came across in the wrong way. That was 
never
the intention.


> 2025-05-28T12:21:59-07:00, Atish Patra <atish.patra@linux.dev>:
>> On 5/28/25 8:09 AM, Andrew Jones wrote:
>>> On Wed, May 28, 2025 at 07:16:11AM -0700, Atish Patra wrote:
>>>> On 5/26/25 4:13 AM, Andrew Jones wrote:
>>>>> On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
>>>>>> 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
>>>>>>> On 5/23/25 6:31 AM, Radim Krčmář wrote:
>>>>>>>> 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
>>>>>>>>> Upgrade the SBI version to v3.0 so that corresponding features
>>>>>>>>> can be enabled in the guest.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>>>>>>> ---
>>>>>>>>> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
>>>>>>>>> -#define KVM_SBI_VERSION_MAJOR 2
>>>>>>>>> +#define KVM_SBI_VERSION_MAJOR 3
>>>>>>>> I think it's time to add versioning to KVM SBI implementation.
>>>>>>>> Userspace should be able to select the desired SBI version and KVM would
>>>>>>>> tell the guest that newer features are not supported.
>>>>> We need new code for this, but it's a good idea.
>>>>>
>>>>>>> We can achieve that through onereg interface by disabling individual SBI
>>>>>>> extensions.
>>>>>>> We can extend the existing onereg interface to disable a specific SBI
>>>>>>> version directly
>>>>>>> instead of individual ones to save those IOCTL as well.
>>>>>> Yes, I am all in favor of letting userspace provide all values in the
>>>>>> BASE extension.
>>>> We already support vendorid/archid/impid through one reg. I think we just
>>>> need to add the SBI version support to that so that user space can set it.
>>>>
>>>>> This is covered by your recent patch that provides userspace_sbi.
>>>> Why do we need to invent new IOCTL for this ? Once the user space sets the
>>>> SBI version, KVM can enforce it.
>>> If an SBI spec version provides an extension that can be emulated by
>>> userspace, then userspace could choose to advertise that spec version,
>>> implement a BASE probe function that advertises the extension, and
>>> implement the extension, even if the KVM version running is older
>>> and unaware of it. But, in order to do that, we need KVM to exit to
>>> userspace for all unknown SBI calls and to allow BASE to be overridden
>> You mean only the version field in BASE - Correct ?
> No, "BASE probe function" is the sbi_probe_extension() ecall.
>
>>> by userspace. The new KVM CAP ioctl allows opting into that new behavior.
>> But why we need a new IOCTL for that ? We can achieve that with existing
>> one reg interface with improvements.
> It's an existing IOCTL with a new data payload, but I can easily use
> ONE_REG if you want to do everything through that.
>
> KVM doesn't really need any other IOCTL than ONE_REGs, it's just
> sometimes more reasonable to use a different IOCTL, like ENABLE_CAP.
>
>>> The old KVM with new VMM configuration isn't totally far-fetched. While
>>> host kernels tend to get updated regularly to include security fixes,
>>> enterprise kernels tend to stop adding features at some point in order
>>> to maximize stability. While enterprise VMMs would also eventually stop
>>> adding features, enterprise consumers are always free to use their own
>>> VMMs (at their own risk). So, there's a real chance we could have
>> I think we are years away from that happening (if it happens). My
>> suggestion was not to
>> try to build a world where no body lives ;). When we get to that
>> scenario, the default KVM
>> shipped will have many extension implemented. So there won't be much
>> advantage to
>> reimplement them in the user space. We can also take an informed
>> decision at that time
>> if the current selective forwarding approach is better
> Please don't repeat the design of SUSP/SRST/DBCN.
> Seeing them is one of the reasons why I proposed the new interface.
>
> "Blindly" forwarding DBCN to userspace is even a minor optimization. :)
>
>>                                                         or we need to
>> blindly forward any
>> unknown SBI calls to the user space.
> Yes, KVM has to do what userpace configures it to do.
>
> I don't think that implementing unsupported SBI extensions in KVM is
> important -- they should not be a hot path.
>
>>> deployments with older, stable KVM where users want to enable later SBI
>>> extensions, and, in some cases, that should be possible by just updating
>>> the VMM -- but only if KVM is only acting as an SBI implementation
>>> accelerator and not as a userspace SBI implementation gatekeeper.
>> But some of the SBI extensions are so fundamental that it must be
>> implemented in KVM
>> for various reasons pointed by Anup on other thread.
> No, SBI does not have to be implemented in KVM at all.
>
> We do have a deep disagreement on what is virtualization and the role of
> KVM in it.  I think that userspace wants a generic ISA accelerator.

I think the disagreement is the role of SBI in KVM virtualization rather 
than
a generic virtualization and the role of KVM in it. I completely agree 
that KVM should act as an accelerator and defer the control to the user 
space in most of the cases
such e.g I/O operations or system related functionalities. However, SBI 
specification solves
much wider problems than those. Broadly we can categorize SBI 
functionalities into the following
areas

1. Bridging ISA GAP
2. Higher Privilege Assistance
3. Virtualization
4. Platform abstraction
5. Confidential computing

For #1, #3 and #5, I believe user space shouldn't be involved in 
implementation
some of them are in hot path as well. For #4 and #2, there are some 
opportunities which
can be implemented in user space depending on the exact need. I am still 
not clear what is the exact
motivation /right now/ to pursue such a path. May be I missed something.
As per my understanding from our discussion threads, there are two use 
cases possible

1. userspace wants to update more states in HSM. What are the states 
user space should care about scounteren (fixed already in usptream) ?
2. VMM vs KVM version difference - this may be true in the future 
depending on the speed of RISC-V virtualization adoption in the industry.
But we are definitely not there yet. Please let me know if I 
misunderstood any use cases.

> Even if userspace wants SBI for the M-mode interface, security minded
This is probably a 3rd one ? Why we want M-mode interface in the user 
space ?
> userspace aims for as little kernel code as possible.

We trust VMM code more than KVM code ?

> Userspace might want to accelerate some SBI extension in KVM, but it
> should not be KVM who decides what userspace wants.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Andrew Jones]

On Thu, May 29, 2025 at 11:44:38AM -0700, Atish Patra wrote:
> 
> On 5/29/25 3:24 AM, Radim Krčmář wrote:
> > I originally gave up on the idea, but I feel kinda bad for Drew now, so
> > trying again:
> 
> I am sorry if some of my replies came across in the wrong way. That was
> never
> the intention.

Not at all. Radim only meant that I was defending his patches, even though
he wasn't :-)

> 
> 
> > 2025-05-28T12:21:59-07:00, Atish Patra <atish.patra@linux.dev>:
> > > On 5/28/25 8:09 AM, Andrew Jones wrote:
> > > > On Wed, May 28, 2025 at 07:16:11AM -0700, Atish Patra wrote:
> > > > > On 5/26/25 4:13 AM, Andrew Jones wrote:
> > > > > > On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
> > > > > > > 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
> > > > > > > > On 5/23/25 6:31 AM, Radim Krčmář wrote:
> > > > > > > > > 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
> > > > > > > > > > Upgrade the SBI version to v3.0 so that corresponding features
> > > > > > > > > > can be enabled in the guest.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > > > > > > ---
> > > > > > > > > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > > > > > > -#define KVM_SBI_VERSION_MAJOR 2
> > > > > > > > > > +#define KVM_SBI_VERSION_MAJOR 3
> > > > > > > > > I think it's time to add versioning to KVM SBI implementation.
> > > > > > > > > Userspace should be able to select the desired SBI version and KVM would
> > > > > > > > > tell the guest that newer features are not supported.
> > > > > > We need new code for this, but it's a good idea.
> > > > > > 
> > > > > > > > We can achieve that through onereg interface by disabling individual SBI
> > > > > > > > extensions.
> > > > > > > > We can extend the existing onereg interface to disable a specific SBI
> > > > > > > > version directly
> > > > > > > > instead of individual ones to save those IOCTL as well.
> > > > > > > Yes, I am all in favor of letting userspace provide all values in the
> > > > > > > BASE extension.
> > > > > We already support vendorid/archid/impid through one reg. I think we just
> > > > > need to add the SBI version support to that so that user space can set it.
> > > > > 
> > > > > > This is covered by your recent patch that provides userspace_sbi.
> > > > > Why do we need to invent new IOCTL for this ? Once the user space sets the
> > > > > SBI version, KVM can enforce it.
> > > > If an SBI spec version provides an extension that can be emulated by
> > > > userspace, then userspace could choose to advertise that spec version,
> > > > implement a BASE probe function that advertises the extension, and
> > > > implement the extension, even if the KVM version running is older
> > > > and unaware of it. But, in order to do that, we need KVM to exit to
> > > > userspace for all unknown SBI calls and to allow BASE to be overridden
> > > You mean only the version field in BASE - Correct ?
> > No, "BASE probe function" is the sbi_probe_extension() ecall.
> > 
> > > > by userspace. The new KVM CAP ioctl allows opting into that new behavior.
> > > But why we need a new IOCTL for that ? We can achieve that with existing
> > > one reg interface with improvements.
> > It's an existing IOCTL with a new data payload, but I can easily use
> > ONE_REG if you want to do everything through that.
> > 
> > KVM doesn't really need any other IOCTL than ONE_REGs, it's just
> > sometimes more reasonable to use a different IOCTL, like ENABLE_CAP.
> > 
> > > > The old KVM with new VMM configuration isn't totally far-fetched. While
> > > > host kernels tend to get updated regularly to include security fixes,
> > > > enterprise kernels tend to stop adding features at some point in order
> > > > to maximize stability. While enterprise VMMs would also eventually stop
> > > > adding features, enterprise consumers are always free to use their own
> > > > VMMs (at their own risk). So, there's a real chance we could have
> > > I think we are years away from that happening (if it happens). My
> > > suggestion was not to
> > > try to build a world where no body lives ;). When we get to that
> > > scenario, the default KVM
> > > shipped will have many extension implemented. So there won't be much
> > > advantage to
> > > reimplement them in the user space. We can also take an informed
> > > decision at that time
> > > if the current selective forwarding approach is better
> > Please don't repeat the design of SUSP/SRST/DBCN.
> > Seeing them is one of the reasons why I proposed the new interface.
> > 
> > "Blindly" forwarding DBCN to userspace is even a minor optimization. :)
> > 
> > >                                                         or we need to
> > > blindly forward any
> > > unknown SBI calls to the user space.
> > Yes, KVM has to do what userpace configures it to do.
> > 
> > I don't think that implementing unsupported SBI extensions in KVM is
> > important -- they should not be a hot path.
> > 
> > > > deployments with older, stable KVM where users want to enable later SBI
> > > > extensions, and, in some cases, that should be possible by just updating
> > > > the VMM -- but only if KVM is only acting as an SBI implementation
> > > > accelerator and not as a userspace SBI implementation gatekeeper.
> > > But some of the SBI extensions are so fundamental that it must be
> > > implemented in KVM
> > > for various reasons pointed by Anup on other thread.
> > No, SBI does not have to be implemented in KVM at all.
> > 
> > We do have a deep disagreement on what is virtualization and the role of
> > KVM in it.  I think that userspace wants a generic ISA accelerator.
> 
> I think the disagreement is the role of SBI in KVM virtualization rather
> than
> a generic virtualization and the role of KVM in it. I completely agree that
> KVM should act as an accelerator and defer the control to the user space in
> most of the cases
> such e.g I/O operations or system related functionalities. However, SBI
> specification solves
> much wider problems than those. Broadly we can categorize SBI
> functionalities into the following
> areas
> 
> 1. Bridging ISA GAP
> 2. Higher Privilege Assistance
> 3. Virtualization
> 4. Platform abstraction
> 5. Confidential computing
> 
> For #1, #3 and #5, I believe user space shouldn't be involved in
> implementation
> some of them are in hot path as well.

IMO, userspace should still be in control of whether or not it's involved
in #1, #3, and #5. It may make little sense for it to be involved, but the
choice should still be its.

> For #4 and #2, there are some
> opportunities which
> can be implemented in user space depending on the exact need. I am still not
> clear what is the exact
> motivation /right now/ to pursue such a path. May be I missed something.
> As per my understanding from our discussion threads, there are two use cases
> possible
> 
> 1. userspace wants to update more states in HSM. What are the states user
> space should care about scounteren (fixed already in usptream) ?
> 2. VMM vs KVM version difference - this may be true in the future depending
> on the speed of RISC-V virtualization adoption in the industry.
> But we are definitely not there yet. Please let me know if I misunderstood
> any use cases.

That's what I'm aware of as well, but I see giving userspace back full
control of what gets accelerated by KVM, and what doesn't, as a fix, which
is why I wouldn't want to delay it any longer.

> 
> > Even if userspace wants SBI for the M-mode interface, security minded
> This is probably a 3rd one ? Why we want M-mode interface in the user space
> ?
> > userspace aims for as little kernel code as possible.
> 
> We trust VMM code more than KVM code ?

We should be skeptical of both, which is why we'd rather put as much code
in userspace as possible. Insecure/faulty userspace will hopefully have
exploits/bugs contained to the single process. An insecure/faulty KVM
means the host is compromised/crashed. On x86, Google put a lot of effort
into moving instruction emulation out of KVM for security concerns[1]. In
general, if it's not a hot path and there's a way to do it in userspace,
then it should be done in userspace (or at least there should be an
option to use userspace -- each use case can choose what's best for
itself).

[1] https://www.linux-kvm.org/images/3/3d/01x02-Steve_Rutherford-Performant_Security_Hardening_of_KVM.pdf

Thanks,
drew

> 
> > Userspace might want to accelerate some SBI extension in KVM, but it
> > should not be KVM who decides what userspace wants.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Anup Patel]

On Fri, May 30, 2025 at 12:44 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, May 29, 2025 at 11:44:38AM -0700, Atish Patra wrote:
> >
> > On 5/29/25 3:24 AM, Radim Krčmář wrote:
> > > I originally gave up on the idea, but I feel kinda bad for Drew now, so
> > > trying again:
> >
> > I am sorry if some of my replies came across in the wrong way. That was
> > never
> > the intention.
>
> Not at all. Radim only meant that I was defending his patches, even though
> he wasn't :-)
>
> >
> >
> > > 2025-05-28T12:21:59-07:00, Atish Patra <atish.patra@linux.dev>:
> > > > On 5/28/25 8:09 AM, Andrew Jones wrote:
> > > > > On Wed, May 28, 2025 at 07:16:11AM -0700, Atish Patra wrote:
> > > > > > On 5/26/25 4:13 AM, Andrew Jones wrote:
> > > > > > > On Mon, May 26, 2025 at 11:00:30AM +0200, Radim Krčmář wrote:
> > > > > > > > 2025-05-23T10:16:11-07:00, Atish Patra <atish.patra@linux.dev>:
> > > > > > > > > On 5/23/25 6:31 AM, Radim Krčmář wrote:
> > > > > > > > > > 2025-05-22T12:03:43-07:00, Atish Patra <atishp@rivosinc.com>:
> > > > > > > > > > > Upgrade the SBI version to v3.0 so that corresponding features
> > > > > > > > > > > can be enabled in the guest.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > > > > > > > > ---
> > > > > > > > > > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > > > > > > > -#define KVM_SBI_VERSION_MAJOR 2
> > > > > > > > > > > +#define KVM_SBI_VERSION_MAJOR 3
> > > > > > > > > > I think it's time to add versioning to KVM SBI implementation.
> > > > > > > > > > Userspace should be able to select the desired SBI version and KVM would
> > > > > > > > > > tell the guest that newer features are not supported.
> > > > > > > We need new code for this, but it's a good idea.
> > > > > > >
> > > > > > > > > We can achieve that through onereg interface by disabling individual SBI
> > > > > > > > > extensions.
> > > > > > > > > We can extend the existing onereg interface to disable a specific SBI
> > > > > > > > > version directly
> > > > > > > > > instead of individual ones to save those IOCTL as well.
> > > > > > > > Yes, I am all in favor of letting userspace provide all values in the
> > > > > > > > BASE extension.
> > > > > > We already support vendorid/archid/impid through one reg. I think we just
> > > > > > need to add the SBI version support to that so that user space can set it.
> > > > > >
> > > > > > > This is covered by your recent patch that provides userspace_sbi.
> > > > > > Why do we need to invent new IOCTL for this ? Once the user space sets the
> > > > > > SBI version, KVM can enforce it.
> > > > > If an SBI spec version provides an extension that can be emulated by
> > > > > userspace, then userspace could choose to advertise that spec version,
> > > > > implement a BASE probe function that advertises the extension, and
> > > > > implement the extension, even if the KVM version running is older
> > > > > and unaware of it. But, in order to do that, we need KVM to exit to
> > > > > userspace for all unknown SBI calls and to allow BASE to be overridden
> > > > You mean only the version field in BASE - Correct ?
> > > No, "BASE probe function" is the sbi_probe_extension() ecall.
> > >
> > > > > by userspace. The new KVM CAP ioctl allows opting into that new behavior.
> > > > But why we need a new IOCTL for that ? We can achieve that with existing
> > > > one reg interface with improvements.
> > > It's an existing IOCTL with a new data payload, but I can easily use
> > > ONE_REG if you want to do everything through that.
> > >
> > > KVM doesn't really need any other IOCTL than ONE_REGs, it's just
> > > sometimes more reasonable to use a different IOCTL, like ENABLE_CAP.
> > >
> > > > > The old KVM with new VMM configuration isn't totally far-fetched. While
> > > > > host kernels tend to get updated regularly to include security fixes,
> > > > > enterprise kernels tend to stop adding features at some point in order
> > > > > to maximize stability. While enterprise VMMs would also eventually stop
> > > > > adding features, enterprise consumers are always free to use their own
> > > > > VMMs (at their own risk). So, there's a real chance we could have
> > > > I think we are years away from that happening (if it happens). My
> > > > suggestion was not to
> > > > try to build a world where no body lives ;). When we get to that
> > > > scenario, the default KVM
> > > > shipped will have many extension implemented. So there won't be much
> > > > advantage to
> > > > reimplement them in the user space. We can also take an informed
> > > > decision at that time
> > > > if the current selective forwarding approach is better
> > > Please don't repeat the design of SUSP/SRST/DBCN.
> > > Seeing them is one of the reasons why I proposed the new interface.
> > >
> > > "Blindly" forwarding DBCN to userspace is even a minor optimization. :)
> > >
> > > >                                                         or we need to
> > > > blindly forward any
> > > > unknown SBI calls to the user space.
> > > Yes, KVM has to do what userpace configures it to do.
> > >
> > > I don't think that implementing unsupported SBI extensions in KVM is
> > > important -- they should not be a hot path.
> > >
> > > > > deployments with older, stable KVM where users want to enable later SBI
> > > > > extensions, and, in some cases, that should be possible by just updating
> > > > > the VMM -- but only if KVM is only acting as an SBI implementation
> > > > > accelerator and not as a userspace SBI implementation gatekeeper.
> > > > But some of the SBI extensions are so fundamental that it must be
> > > > implemented in KVM
> > > > for various reasons pointed by Anup on other thread.
> > > No, SBI does not have to be implemented in KVM at all.
> > >
> > > We do have a deep disagreement on what is virtualization and the role of
> > > KVM in it.  I think that userspace wants a generic ISA accelerator.
> >
> > I think the disagreement is the role of SBI in KVM virtualization rather
> > than
> > a generic virtualization and the role of KVM in it. I completely agree that
> > KVM should act as an accelerator and defer the control to the user space in
> > most of the cases
> > such e.g I/O operations or system related functionalities. However, SBI
> > specification solves
> > much wider problems than those. Broadly we can categorize SBI
> > functionalities into the following
> > areas
> >
> > 1. Bridging ISA GAP
> > 2. Higher Privilege Assistance
> > 3. Virtualization
> > 4. Platform abstraction
> > 5. Confidential computing
> >
> > For #1, #3 and #5, I believe user space shouldn't be involved in
> > implementation
> > some of them are in hot path as well.
>
> IMO, userspace should still be in control of whether or not it's involved
> in #1, #3, and #5. It may make little sense for it to be involved, but the
> choice should still be its.
>
> > For #4 and #2, there are some
> > opportunities which
> > can be implemented in user space depending on the exact need. I am still not
> > clear what is the exact
> > motivation /right now/ to pursue such a path. May be I missed something.
> > As per my understanding from our discussion threads, there are two use cases
> > possible
> >
> > 1. userspace wants to update more states in HSM. What are the states user
> > space should care about scounteren (fixed already in usptream) ?
> > 2. VMM vs KVM version difference - this may be true in the future depending
> > on the speed of RISC-V virtualization adoption in the industry.
> > But we are definitely not there yet. Please let me know if I misunderstood
> > any use cases.
>
> That's what I'm aware of as well, but I see giving userspace back full
> control of what gets accelerated by KVM, and what doesn't, as a fix, which
> is why I wouldn't want to delay it any longer.
>
> >
> > > Even if userspace wants SBI for the M-mode interface, security minded
> > This is probably a 3rd one ? Why we want M-mode interface in the user space
> > ?
> > > userspace aims for as little kernel code as possible.
> >
> > We trust VMM code more than KVM code ?
>
> We should be skeptical of both, which is why we'd rather put as much code
> in userspace as possible. Insecure/faulty userspace will hopefully have
> exploits/bugs contained to the single process. An insecure/faulty KVM
> means the host is compromised/crashed. On x86, Google put a lot of effort
> into moving instruction emulation out of KVM for security concerns[1]. In
> general, if it's not a hot path and there's a way to do it in userspace,
> then it should be done in userspace (or at least there should be an
> option to use userspace -- each use case can choose what's best for
> itself).
>
> [1] https://www.linux-kvm.org/images/3/3d/01x02-Steve_Rutherford-Performant_Security_Hardening_of_KVM.pdf
>

We are already forwarding a few of the category #2 and all
of category #4 SBI extensions to KVM user space which are
not in critical or hot-path.

Majority of SBI extensions in categories #1, #2, #3, and #5
provide critical per-VCPU functionality and many of these
are also in hotpath (such as #1, #3, and #5) hence
implemented in kernel space.

Further, KVM user space lacks required functionality (CSRs,
instructions, or ioctls) to implement many critical SBI extensions
in user space so blanket forwarding of all SBI extensions to
KVM user space is not going to fly.
(Note: Previously, I have already provided many examples)

In short, a hybrid approach (current implementation) is the
best thing where only non-critical and non-hotpath SBI
extensions (few of them) are forwarded to KVM user space
while critical / hot-path SBI extensions (majority of them)
are in kernel space.

Regards,
Anup

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Radim Krčmář]

2025-05-29T11:44:38-07:00, Atish Patra <atish.patra@linux.dev>:
> On 5/29/25 3:24 AM, Radim Krčmář wrote:
>> I originally gave up on the idea, but I feel kinda bad for Drew now, so
>> trying again:
>
> I am sorry if some of my replies came across in the wrong way. That was 
> never
> the intention.

I didn't mean to accuse you, my apologies.  I agree with Drew's
positions, so to expand on a question that wasn't touched in his mail:

>> Even if userspace wants SBI for the M-mode interface, security minded
> This is probably a 3rd one ? Why we want M-mode interface in the user 
> space ?

It is about turning KVM into an ISA accelerator.

A guest thinks it is running in S/HS-mode.
The ecall instruction traps to M-mode.  RISC-V H extension doesn't
accelerate M-mode, so we have to emulate the trap in software.

The ISA doesn't say that M-mode means SBI.  We try really hard to have
SBI on all RISC-V, but I think KVM is taking it a bit too far.

We can discuss how best to describe SBI, so userspace can choose to
accelerate the M-mode in KVM, but I think that the ability to emulate
M-mode in userspace should be provided.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Atish Patra]

On 5/30/25 4:09 AM, Radim Krčmář wrote:
> 2025-05-29T11:44:38-07:00, Atish Patra <atish.patra@linux.dev>:
>> On 5/29/25 3:24 AM, Radim Krčmář wrote:
>>> I originally gave up on the idea, but I feel kinda bad for Drew now, so
>>> trying again:
>> I am sorry if some of my replies came across in the wrong way. That was
>> never
>> the intention.
> I didn't mean to accuse you, my apologies.  I agree with Drew's
> positions, so to expand on a question that wasn't touched in his mail:
>
>>> Even if userspace wants SBI for the M-mode interface, security minded
>> This is probably a 3rd one ? Why we want M-mode interface in the user
>> space ?
> It is about turning KVM into an ISA accelerator.
>
> A guest thinks it is running in S/HS-mode.
> The ecall instruction traps to M-mode.  RISC-V H extension doesn't
> accelerate M-mode, so we have to emulate the trap in software.
We don't need to accelerate M-mode. That's the beauty of the RISC-V H 
extension.
The ISA is designed in such a way that the SBI is the interface between 
the supervisor environment (VS/HS)
and the supervisor execution environment (HS/M).


>
> The ISA doesn't say that M-mode means SBI.  We try really hard to have
> SBI on all RISC-V, but I think KVM is taking it a bit too far.
>
> We can discuss how best to describe SBI, so userspace can choose to
> accelerate the M-mode in KVM, but I think that the ability to emulate
> M-mode in userspace should be provided.
I am still trying to understand the advantages of emulating the M-mode 
in the user space.
Can you please elaborate ?
I am assuming you are not hinting Nested virtualization which can be 
achieved with existing
ISA provided mechanisms and accelerated by SBI NACL.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Radim Krčmář]

2025-05-30T12:29:30-07:00, Atish Patra <atish.patra@linux.dev>:
> On 5/30/25 4:09 AM, Radim Krčmář wrote:
>> 2025-05-29T11:44:38-07:00, Atish Patra <atish.patra@linux.dev>:
>>> On 5/29/25 3:24 AM, Radim Krčmář wrote:
>>>> I originally gave up on the idea, but I feel kinda bad for Drew now, so
>>>> trying again:
>>> I am sorry if some of my replies came across in the wrong way. That was
>>> never
>>> the intention.
>> I didn't mean to accuse you, my apologies.  I agree with Drew's
>> positions, so to expand on a question that wasn't touched in his mail:
>>
>>>> Even if userspace wants SBI for the M-mode interface, security minded
>>> This is probably a 3rd one ? Why we want M-mode interface in the user
>>> space ?
>> It is about turning KVM into an ISA accelerator.
>>
>> A guest thinks it is running in S/HS-mode.
>> The ecall instruction traps to M-mode.  RISC-V H extension doesn't
>> accelerate M-mode, so we have to emulate the trap in software.
> We don't need to accelerate M-mode. That's the beauty of the RISC-V H 
> extension.

(It is a gap to me. :])

> The ISA is designed in such a way that the SBI is the interface between 
> the supervisor environment (VS/HS)
> and the supervisor execution environment (HS/M).

The ISA says nothing about the implementation of said interface.

Returning 42 in x21 as a response to an ecall with 0x10 in a7 and 0x3 in
a6 is perfectly valid RISC-V implementation that KVM currently cannot
virtualize.

>> The ISA doesn't say that M-mode means SBI.  We try really hard to have
>> SBI on all RISC-V, but I think KVM is taking it a bit too far.
>>
>> We can discuss how best to describe SBI, so userspace can choose to
>> accelerate the M-mode in KVM, but I think that the ability to emulate
>> M-mode in userspace should be provided.
> I am still trying to understand the advantages of emulating the M-mode 
> in the user space.
> Can you please elaborate ?

This thread already has a lot of them, so to avoid repeating them, I
have to go into quite niche use-cases:
When developing M-mode software on RISC-V (when RISC-V has more useful
implementations than QEMU), a developer might want to accelerate the
S/U-modes in KVM.
It is also simpler to implement an old SBI interface (especially with
bugs/quirks) if virtualization just executes the old M-mode binary.

Why must KVM prevent userspace from virtualizing RISC-V?

> I am assuming you are not hinting Nested virtualization which can be 
> achieved with existing
> ISA provided mechanisms and accelerated by SBI NACL.

Right, I am talking about virtualization of RISC-V, because I don't have
a crystal ball to figure out what users will want.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Atish Patra]

On 6/3/25 4:40 AM, Radim Krčmář wrote:
> 2025-05-30T12:29:30-07:00, Atish Patra <atish.patra@linux.dev>:
>> On 5/30/25 4:09 AM, Radim Krčmář wrote:
>>> 2025-05-29T11:44:38-07:00, Atish Patra <atish.patra@linux.dev>:
>>>> On 5/29/25 3:24 AM, Radim Krčmář wrote:
>>>>> I originally gave up on the idea, but I feel kinda bad for Drew now, so
>>>>> trying again:
>>>> I am sorry if some of my replies came across in the wrong way. That was
>>>> never
>>>> the intention.
>>> I didn't mean to accuse you, my apologies.  I agree with Drew's
>>> positions, so to expand on a question that wasn't touched in his mail:
>>>
>>>>> Even if userspace wants SBI for the M-mode interface, security minded
>>>> This is probably a 3rd one ? Why we want M-mode interface in the user
>>>> space ?
>>> It is about turning KVM into an ISA accelerator.
>>>
>>> A guest thinks it is running in S/HS-mode.
>>> The ecall instruction traps to M-mode.  RISC-V H extension doesn't
>>> accelerate M-mode, so we have to emulate the trap in software.
>> We don't need to accelerate M-mode. That's the beauty of the RISC-V H
>> extension.
> (It is a gap to me. :])
RISC-V H extension is designed to virtualize S-mode and U-mode. Not M-mode.
I don't think retrofitting M-mode virtualization has absolutely any 
benefit. It has
many challenges that will probably result in poor performance. It can be 
a hobby project
but I am not sure if it can be adopted in production.

Are there any similar use cases in other ISAs ? Does anybody support 
virtualizaing EL3 in ARM64 ?

>> The ISA is designed in such a way that the SBI is the interface between
>> the supervisor environment (VS/HS)
>> and the supervisor execution environment (HS/M).
> The ISA says nothing about the implementation of said interface.
>
> Returning 42 in x21 as a response to an ecall with 0x10 in a7 and 0x3 in
> a6 is perfectly valid RISC-V implementation that KVM currently cannot
> virtualize.

If the concern is only supporting an older version of SBI version, we 
can support that with onereg
interface today. I think I already agreed on that earlier in this thread 
and revise this series to have
it ready for review.


>>> The ISA doesn't say that M-mode means SBI.  We try really hard to have
>>> SBI on all RISC-V, but I think KVM is taking it a bit too far.
>>>
>>> We can discuss how best to describe SBI, so userspace can choose to
>>> accelerate the M-mode in KVM, but I think that the ability to emulate
>>> M-mode in userspace should be provided.
>> I am still trying to understand the advantages of emulating the M-mode
>> in the user space.
>> Can you please elaborate ?
> This thread already has a lot of them, so to avoid repeating them, I
> have to go into quite niche use-cases:
> When developing M-mode software on RISC-V (when RISC-V has more useful
> implementations than QEMU), a developer might want to accelerate the
> S/U-modes in KVM.
> It is also simpler to implement an old SBI interface (especially with
> bugs/quirks) if virtualization just executes the old M-mode binary.
>
> Why must KVM prevent userspace from virtualizing RISC-V?

If there is a valid use case that can be put into production or
if you have any prototype that it has better performance then we can 
have it.
In absence of either, isn't it better to spend our energy on things that 
actually matter
right now and improve RISC-V virtualization performance rather than 
something that
may or may not be possible in the very far future.

>> I am assuming you are not hinting Nested virtualization which can be
>> achieved with existing
>> ISA provided mechanisms and accelerated by SBI NACL.
> Right, I am talking about virtualization of RISC-V, because I don't have
> a crystal ball to figure out what users will want.

Re: [PATCH v3 9/9] RISC-V: KVM: Upgrade the supported SBI version to 3.0

[Anup Patel]

On Fri, May 23, 2025 at 12:33 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> Upgrade the SBI version to v3.0 so that corresponding features
> can be enabled in the guest.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Extending the ONE_REG interface to allow KVM user-space select
SBI version can be done as a separate series.

For this series, we can go ahead with this patch.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index 4ed6203cdd30..194299e0ab0e 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -11,7 +11,7 @@
>
>  #define KVM_SBI_IMPID 3
>
> -#define KVM_SBI_VERSION_MAJOR 2
> +#define KVM_SBI_VERSION_MAJOR 3
>  #define KVM_SBI_VERSION_MINOR 0
>
>  enum kvm_riscv_sbi_ext_status {
>
> --
> 2.43.0
>