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

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

[Atish Patra]

SBI v3.0 specification[1] added two new improvements to the PMU chaper.

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 adds a fix
in firmware event mapping and updates the kvm SBI implementation to
SBI v3.0.

The opensbi patches can be found at [3]. This series can be found at [4].

[1] https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/vv3.0-rc2/riscv-sbi.pdf
[2] https://github.com/riscv/riscv-isa-manual/issues/1578
[3] https://github.com/atishp04/opensbi/tree/b4/pmu_event_info
[4] https://github.com/atishp04/linux/tree/b4/pmu_event_info

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (8):
      drivers/perf: riscv: Add SBI v3.0 flag
      drivers/perf: riscv: Fix Platform firmware event data
      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
      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          |  12 +++
 arch/riscv/kvm/vcpu_pmu.c             |  71 +++++++++++++
 arch/riscv/kvm/vcpu_sbi_pmu.c         |   3 +
 drivers/perf/riscv_pmu_sbi.c          | 184 +++++++++++++++++++++++++---------
 include/linux/perf/riscv_pmu.h        |   2 +
 7 files changed, 228 insertions(+), 49 deletions(-)
---
base-commit: acb481ddd977ab669128bab61024d05e7dc1654f
change-id: 20241018-pmu_event_info-986e21ce6bd3
--
Regards,
Atish patra

[PATCH 1/8] 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 391ca1422cae..cb98efa9b106 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)
@@ -1450,6 +1451,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.34.1

[PATCH 2/8] drivers/perf: riscv: Fix Platform firmware event data

[Atish Patra]

Platform firmware event data field is allowed to be 62 bits for
Linux as uppper most two bits are reserved to indicate SBI fw or
platform specific firmware events.
However, the event data field is masked as per the hardware raw
event mask which is not correct.

Fix the platform firmware event data field with proper mask.

Fixes: f0c9363db2dd ("perf/riscv-sbi: Add platform specific firmware event handling")

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

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 98f631b051db..9be38b05f4ad 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -158,6 +158,7 @@ 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)
 #define RISCV_PMU_RAW_EVENT_IDX 0x20000
 #define RISCV_PLAT_FW_EVENT	0xFFFF
 
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index cb98efa9b106..50cbdbf66bb7 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -508,7 +508,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 {
 	u32 type = event->attr.type;
 	u64 config = event->attr.config;
-	u64 raw_config_val;
 	int ret;
 
 	/*
@@ -529,21 +528,20 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 	case PERF_TYPE_RAW:
 		/*
 		 * As per SBI specification, the upper 16 bits must be unused
-		 * for a raw event.
+		 * 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
 		 */
-		raw_config_val = config & RISCV_PMU_RAW_EVENT_MASK;
+
 		switch (config >> 62) {
 		case 0:
 			ret = RISCV_PMU_RAW_EVENT_IDX;
-			*econfig = raw_config_val;
+			*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
 			break;
 		case 2:
-			ret = (raw_config_val & 0xFFFF) |
-				(SBI_PMU_EVENT_TYPE_FW << 16);
+			ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
 			break;
 		case 3:
 			/*
@@ -552,7 +550,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 			 * Event data - raw event encoding
 			 */
 			ret = SBI_PMU_EVENT_TYPE_FW << 16 | RISCV_PLAT_FW_EVENT;
-			*econfig = raw_config_val;
+			*econfig = config & RISCV_PMU_PLAT_FW_EVENT_MASK;
 			break;
 		}
 		break;

-- 
2.34.1

[PATCH 3/8] 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 | 18 ++++++++++++------
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 9be38b05f4ad..3ee9bfa5e77c 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -159,7 +159,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 */
@@ -217,6 +220,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 50cbdbf66bb7..f0e845ff6b79 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,18 +527,24 @@ 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
 		 * 11 - Risc-V platform specific firmware event
 		 */
-
 		switch (config >> 62) {
 		case 0:
-			ret = RISCV_PMU_RAW_EVENT_IDX;
-			*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
+			if (sbi_v3_available) {
+				*econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
+				ret = RISCV_PMU_RAW_EVENT_V2_IDX;
+			} else {
+				*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);

-- 
2.34.1

[PATCH 4/8] 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 2707a51b082c..efd66835c2b8 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.34.1

[PATCH 5/8] 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 |  7 +++++
 drivers/perf/riscv_pmu_sbi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 3ee9bfa5e77c..c04f64fbc01d 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -134,6 +134,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 {
@@ -157,6 +158,12 @@ struct riscv_pmu_snapshot_data {
 	u64 reserved[447];
 };
 
+struct riscv_pmu_event_info {
+	u32 event_idx;
+	u32 output;
+	u64 event_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 */
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index f0e845ff6b79..2a6527cc9d97 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 {
@@ -299,6 +300,68 @@ 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 = (struct riscv_pmu_event_info *)
+			   kcalloc(num_events, sizeof(*event_info_shmem), GFP_KERNEL);
+	if (!event_info_shmem) {
+		pr_err("Can not allocate memory for event info query\n");
+		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 (int j = 0; j < ARRAY_SIZE(pmu_cache_event_map[i]); j++) {
+			for (int 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;
+	}
+	/* Do we need some barriers here or priv mode transition will ensure that */
+	for (i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++) {
+		if (!(event_info_shmem[i].output & 0x01))
+			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 & 0x01))
+					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 +379,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.34.1

[PATCH 6/8] 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   | 111 ++++++++++++++++++++++-------------------
 include/linux/perf/riscv_pmu.h |   2 +
 2 files changed, 62 insertions(+), 51 deletions(-)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 2a6527cc9d97..8ddd094c82ad 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -414,6 +414,65 @@ 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) {
+				if (econfig)
+					*econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
+				ret = RISCV_PMU_RAW_EVENT_V2_IDX;
+			} else {
+				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;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(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
@@ -579,7 +638,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;
 
 	/*
 	 * Ensure we are finished checking standard hardware events for
@@ -587,56 +645,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) {
-				*econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
-				ret = RISCV_PMU_RAW_EVENT_V2_IDX;
-			} else {
-				*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;
-		}
-		break;
-	default:
-		ret = -ENOENT;
-		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.34.1

[PATCH 7/8] 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             | 67 +++++++++++++++++++++++++++++++++++
 arch/riscv/kvm/vcpu_sbi_pmu.c         |  3 ++
 3 files changed, 73 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 efd66835c2b8..a30f7ec31479 100644
--- a/arch/riscv/kvm/vcpu_pmu.c
+++ b/arch/riscv/kvm/vcpu_pmu.c
@@ -456,6 +456,73 @@ 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)
+{
+	unsigned long hva;
+	struct riscv_pmu_event_info *einfo;
+	int shmem_size = num_events * sizeof(*einfo);
+	bool writable;
+	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;
+		}
+	}
+
+	hva = kvm_vcpu_gfn_to_hva_prot(vcpu, shmem >> PAGE_SHIFT, &writable);
+	if (kvm_is_error_hva(hva) || !writable) {
+		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;
+	}
+
+	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.34.1

[PATCH 8/8] 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 b96705258cf9..239457b864d7 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.34.1

Re: [PATCH 2/8] drivers/perf: riscv: Fix Platform firmware event data

[Alexandre Ghiti]

Hi Atish,

On 19/11/2024 21:29, Atish Patra wrote:
> Platform firmware event data field is allowed to be 62 bits for
> Linux as uppper most two bits are reserved to indicate SBI fw or
> platform specific firmware events.
> However, the event data field is masked as per the hardware raw
> event mask which is not correct.
>
> Fix the platform firmware event data field with proper mask.
>
> Fixes: f0c9363db2dd ("perf/riscv-sbi: Add platform specific firmware event handling")
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>   arch/riscv/include/asm/sbi.h |  1 +
>   drivers/perf/riscv_pmu_sbi.c | 12 +++++-------
>   2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 98f631b051db..9be38b05f4ad 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -158,6 +158,7 @@ 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)
>   #define RISCV_PMU_RAW_EVENT_IDX 0x20000
>   #define RISCV_PLAT_FW_EVENT	0xFFFF
>   
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index cb98efa9b106..50cbdbf66bb7 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -508,7 +508,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>   {
>   	u32 type = event->attr.type;
>   	u64 config = event->attr.config;
> -	u64 raw_config_val;
>   	int ret;
>   
>   	/*
> @@ -529,21 +528,20 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>   	case PERF_TYPE_RAW:
>   		/*
>   		 * As per SBI specification, the upper 16 bits must be unused
> -		 * for a raw event.
> +		 * 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
>   		 */
> -		raw_config_val = config & RISCV_PMU_RAW_EVENT_MASK;
> +
>   		switch (config >> 62) {
>   		case 0:
>   			ret = RISCV_PMU_RAW_EVENT_IDX;
> -			*econfig = raw_config_val;
> +			*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
>   			break;
>   		case 2:
> -			ret = (raw_config_val & 0xFFFF) |
> -				(SBI_PMU_EVENT_TYPE_FW << 16);
> +			ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
>   			break;
>   		case 3:
>   			/*
> @@ -552,7 +550,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>   			 * Event data - raw event encoding
>   			 */
>   			ret = SBI_PMU_EVENT_TYPE_FW << 16 | RISCV_PLAT_FW_EVENT;
> -			*econfig = raw_config_val;
> +			*econfig = config & RISCV_PMU_PLAT_FW_EVENT_MASK;
>   			break;
>   		}
>   		break;
>

It seems independent from the other patches, so I guess we should take 
it for 6.13 rcX.

Let me know if that's not the case.

Thanks,

Alex

Re: [PATCH 2/8] drivers/perf: riscv: Fix Platform firmware event data

[Atish Kumar Patra]

On Thu, Nov 28, 2024 at 5:10 AM Alexandre Ghiti <alex@ghiti.fr> wrote:
>
> Hi Atish,
>
> On 19/11/2024 21:29, Atish Patra wrote:
> > Platform firmware event data field is allowed to be 62 bits for
> > Linux as uppper most two bits are reserved to indicate SBI fw or
> > platform specific firmware events.
> > However, the event data field is masked as per the hardware raw
> > event mask which is not correct.
> >
> > Fix the platform firmware event data field with proper mask.
> >
> > Fixes: f0c9363db2dd ("perf/riscv-sbi: Add platform specific firmware event handling")
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >   arch/riscv/include/asm/sbi.h |  1 +
> >   drivers/perf/riscv_pmu_sbi.c | 12 +++++-------
> >   2 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 98f631b051db..9be38b05f4ad 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -158,6 +158,7 @@ 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)
> >   #define RISCV_PMU_RAW_EVENT_IDX 0x20000
> >   #define RISCV_PLAT_FW_EVENT 0xFFFF
> >
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index cb98efa9b106..50cbdbf66bb7 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -508,7 +508,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >   {
> >       u32 type = event->attr.type;
> >       u64 config = event->attr.config;
> > -     u64 raw_config_val;
> >       int ret;
> >
> >       /*
> > @@ -529,21 +528,20 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >       case PERF_TYPE_RAW:
> >               /*
> >                * As per SBI specification, the upper 16 bits must be unused
> > -              * for a raw event.
> > +              * 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
> >                */
> > -             raw_config_val = config & RISCV_PMU_RAW_EVENT_MASK;
> > +
> >               switch (config >> 62) {
> >               case 0:
> >                       ret = RISCV_PMU_RAW_EVENT_IDX;
> > -                     *econfig = raw_config_val;
> > +                     *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> >                       break;
> >               case 2:
> > -                     ret = (raw_config_val & 0xFFFF) |
> > -                             (SBI_PMU_EVENT_TYPE_FW << 16);
> > +                     ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
> >                       break;
> >               case 3:
> >                       /*
> > @@ -552,7 +550,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >                        * Event data - raw event encoding
> >                        */
> >                       ret = SBI_PMU_EVENT_TYPE_FW << 16 | RISCV_PLAT_FW_EVENT;
> > -                     *econfig = raw_config_val;
> > +                     *econfig = config & RISCV_PMU_PLAT_FW_EVENT_MASK;
> >                       break;
> >               }
> >               break;
> >
>
> It seems independent from the other patches, so I guess we should take
> it for 6.13 rcX.
>

Yes. This patch doesn't have any SBI v3.0 dependencies. I will send
this patch separately
so that it can be applied for 6.13 rcX

> Let me know if that's not the case.
>
> Thanks,
>
> Alex
>

Re: [PATCH 3/8] drivers/perf: riscv: Add raw event v2 support

[Samuel Holland]

Hi Atish,

On 2024-11-19 2:29 PM, Atish Patra 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 | 18 ++++++++++++------
>  2 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 9be38b05f4ad..3ee9bfa5e77c 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -159,7 +159,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 */
> @@ -217,6 +220,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 50cbdbf66bb7..f0e845ff6b79 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,18 +527,24 @@ 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
>  		 * 11 - Risc-V platform specific firmware event
>  		 */
> -
>  		switch (config >> 62) {
>  		case 0:
> -			ret = RISCV_PMU_RAW_EVENT_IDX;
> -			*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> +			if (sbi_v3_available) {
> +				*econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
> +				ret = RISCV_PMU_RAW_EVENT_V2_IDX;
> +			} else {
> +				*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> +				ret = RISCV_PMU_RAW_EVENT_IDX;

Shouldn't we check to see if any of bits 48-55 are set and return an error,
instead of silently requesting the wrong event?

Regards,
Samuel

> +			}
>  			break;
>  		case 2:
>  			ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
> 

Re: [PATCH 5/8] drivers/perf: riscv: Implement PMU event info function

[Samuel Holland]

Hi Atish,

On 2024-11-19 2:29 PM, Atish Patra 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 |  7 +++++
>  drivers/perf/riscv_pmu_sbi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 3ee9bfa5e77c..c04f64fbc01d 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -134,6 +134,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 {
> @@ -157,6 +158,12 @@ struct riscv_pmu_snapshot_data {
>  	u64 reserved[447];
>  };
>  
> +struct riscv_pmu_event_info {
> +	u32 event_idx;
> +	u32 output;
> +	u64 event_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 */
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index f0e845ff6b79..2a6527cc9d97 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);

This new declaration does not appear to be used.

>  struct sbi_pmu_event_data {
>  	union {
>  		union {
> @@ -299,6 +300,68 @@ 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 = (struct riscv_pmu_event_info *)
> +			   kcalloc(num_events, sizeof(*event_info_shmem), GFP_KERNEL);

Please drop the unnecessary cast.

> +	if (!event_info_shmem) {
> +		pr_err("Can not allocate memory for event info query\n");

Usually there's no need to print an error for allocation failure, since the
allocator already warns. And this isn't really an error, since we can (and do)
fall back to the existing way of checking for events.

> +		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 (int j = 0; j < ARRAY_SIZE(pmu_cache_event_map[i]); j++) {
> +			for (int 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;
> +	}
> +	/* Do we need some barriers here or priv mode transition will ensure that */

No barrier is needed -- the SBI implementation is running on the same hart, so
coherency isn't even a consideration.

> +	for (i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++) {
> +		if (!(event_info_shmem[i].output & 0x01))

This bit mask should probably use a macro.

> +			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 & 0x01))

Same comment applies here.

Regards,
Samuel

> +					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 +379,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]);
>  
> 

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

[Samuel Holland]

Hi Atish,

On 2024-11-19 2:29 PM, Atish Patra 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>
> ---
>  arch/riscv/include/asm/kvm_vcpu_pmu.h |  3 ++
>  arch/riscv/kvm/vcpu_pmu.c             | 67 +++++++++++++++++++++++++++++++++++
>  arch/riscv/kvm/vcpu_sbi_pmu.c         |  3 ++
>  3 files changed, 73 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 efd66835c2b8..a30f7ec31479 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -456,6 +456,73 @@ 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)
> +{
> +	unsigned long hva;
> +	struct riscv_pmu_event_info *einfo;
> +	int shmem_size = num_events * sizeof(*einfo);
> +	bool writable;
> +	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;
> +		}
> +	}
> +
> +	hva = kvm_vcpu_gfn_to_hva_prot(vcpu, shmem >> PAGE_SHIFT, &writable);
> +	if (kvm_is_error_hva(hva) || !writable) {
> +		ret = SBI_ERR_INVALID_ADDRESS;

This only checks the first page if the address crosses a page boundary. Maybe
that is okay since kvm_vcpu_read_guest()/kvm_vcpu_write_guest() will fail if a
later page is inaccessible?

> +		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;

This also needs to write `output` in the else case to indicate that the event is
not supported; the spec does not require the caller to initialize bit 0 of
output to zero.

Regards,
Samuel

> +	}
> +
> +	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;
>  	}
> 

Re: [PATCH 3/8] drivers/perf: riscv: Add raw event v2 support

[Atish Kumar Patra]

On Mon, Dec 2, 2024 at 2:37 PM Samuel Holland <samuel.holland@sifive.com> wrote:
>
> Hi Atish,
>
> On 2024-11-19 2:29 PM, Atish Patra 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 | 18 ++++++++++++------
> >  2 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 9be38b05f4ad..3ee9bfa5e77c 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -159,7 +159,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 */
> > @@ -217,6 +220,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 50cbdbf66bb7..f0e845ff6b79 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,18 +527,24 @@ 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
> >                * 11 - Risc-V platform specific firmware event
> >                */
> > -
> >               switch (config >> 62) {
> >               case 0:
> > -                     ret = RISCV_PMU_RAW_EVENT_IDX;
> > -                     *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> > +                     if (sbi_v3_available) {
> > +                             *econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
> > +                             ret = RISCV_PMU_RAW_EVENT_V2_IDX;
> > +                     } else {
> > +                             *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> > +                             ret = RISCV_PMU_RAW_EVENT_IDX;
>
> Shouldn't we check to see if any of bits 48-55 are set and return an error,
> instead of silently requesting the wrong event?
>

We can. I did not add it originally as we can't do much validation for
the raw events for anyways.
If the encoding is not supported the user will get the error anyways
as it can't find a counter.
We will just save 1 SBI call if the kernel doesn't allow requesting an
event if bits 48-55 are set.

> Regards,
> Samuel
>
> > +                     }
> >                       break;
> >               case 2:
> >                       ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
> >
>

Re: [PATCH 3/8] drivers/perf: riscv: Add raw event v2 support

[Samuel Holland]

Hi Atish,

On 2024-12-02 6:15 PM, Atish Kumar Patra wrote:
> On Mon, Dec 2, 2024 at 2:37 PM Samuel Holland <samuel.holland@sifive.com> wrote:
>> On 2024-11-19 2:29 PM, Atish Patra 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 | 18 ++++++++++++------
>>>  2 files changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
>>> index 9be38b05f4ad..3ee9bfa5e77c 100644
>>> --- a/arch/riscv/include/asm/sbi.h
>>> +++ b/arch/riscv/include/asm/sbi.h
>>> @@ -159,7 +159,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 */
>>> @@ -217,6 +220,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 50cbdbf66bb7..f0e845ff6b79 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,18 +527,24 @@ 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
>>>                * 11 - Risc-V platform specific firmware event
>>>                */
>>> -
>>>               switch (config >> 62) {
>>>               case 0:
>>> -                     ret = RISCV_PMU_RAW_EVENT_IDX;
>>> -                     *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
>>> +                     if (sbi_v3_available) {
>>> +                             *econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
>>> +                             ret = RISCV_PMU_RAW_EVENT_V2_IDX;
>>> +                     } else {
>>> +                             *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
>>> +                             ret = RISCV_PMU_RAW_EVENT_IDX;
>>
>> Shouldn't we check to see if any of bits 48-55 are set and return an error,
>> instead of silently requesting the wrong event?
>>
> 
> We can. I did not add it originally as we can't do much validation for
> the raw events for anyways.
> If the encoding is not supported the user will get the error anyways
> as it can't find a counter.
> We will just save 1 SBI call if the kernel doesn't allow requesting an
> event if bits 48-55 are set.

The scenario I'm concerned about is where masking off bits 48-55 results in a
valid, supported encoding for a different event. For example, in the HPM event
encoding scheme used by Rocket and inherited by SiFive cores, bits 8-55 are a
bitmap. So masking off some of those bits will exclude some events, but will not
create an invalid encoding. This could be very confusing for users.

Regards,
Samuel

Re: [PATCH 3/8] drivers/perf: riscv: Add raw event v2 support

[Atish Kumar Patra]

On Mon, Dec 2, 2024 at 6:39 PM Samuel Holland <samuel.holland@sifive.com> wrote:
>
> Hi Atish,
>
> On 2024-12-02 6:15 PM, Atish Kumar Patra wrote:
> > On Mon, Dec 2, 2024 at 2:37 PM Samuel Holland <samuel.holland@sifive.com> wrote:
> >> On 2024-11-19 2:29 PM, Atish Patra 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 | 18 ++++++++++++------
> >>>  2 files changed, 16 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> >>> index 9be38b05f4ad..3ee9bfa5e77c 100644
> >>> --- a/arch/riscv/include/asm/sbi.h
> >>> +++ b/arch/riscv/include/asm/sbi.h
> >>> @@ -159,7 +159,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 */
> >>> @@ -217,6 +220,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 50cbdbf66bb7..f0e845ff6b79 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,18 +527,24 @@ 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
> >>>                * 11 - Risc-V platform specific firmware event
> >>>                */
> >>> -
> >>>               switch (config >> 62) {
> >>>               case 0:
> >>> -                     ret = RISCV_PMU_RAW_EVENT_IDX;
> >>> -                     *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> >>> +                     if (sbi_v3_available) {
> >>> +                             *econfig = config & RISCV_PMU_RAW_EVENT_V2_MASK;
> >>> +                             ret = RISCV_PMU_RAW_EVENT_V2_IDX;
> >>> +                     } else {
> >>> +                             *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> >>> +                             ret = RISCV_PMU_RAW_EVENT_IDX;
> >>
> >> Shouldn't we check to see if any of bits 48-55 are set and return an error,
> >> instead of silently requesting the wrong event?
> >>
> >
> > We can. I did not add it originally as we can't do much validation for
> > the raw events for anyways.
> > If the encoding is not supported the user will get the error anyways
> > as it can't find a counter.
> > We will just save 1 SBI call if the kernel doesn't allow requesting an
> > event if bits 48-55 are set.
>
> The scenario I'm concerned about is where masking off bits 48-55 results in a
> valid, supported encoding for a different event. For example, in the HPM event
> encoding scheme used by Rocket and inherited by SiFive cores, bits 8-55 are a
> bitmap. So masking off some of those bits will exclude some events, but will not
> create an invalid encoding. This could be very confusing for users.
>

Ahh yes. That is problematic if the vendor implements that type of
event encoding.
I will send the fix patch with an error if bits 48-55 are set.

> Regards,
> Samuel
>

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

[Atish Kumar Patra]

On Mon, Dec 2, 2024 at 3:02 PM Samuel Holland <samuel.holland@sifive.com> wrote:
>
> Hi Atish,
>
> On 2024-11-19 2:29 PM, Atish Patra 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>
> > ---
> >  arch/riscv/include/asm/kvm_vcpu_pmu.h |  3 ++
> >  arch/riscv/kvm/vcpu_pmu.c             | 67 +++++++++++++++++++++++++++++++++++
> >  arch/riscv/kvm/vcpu_sbi_pmu.c         |  3 ++
> >  3 files changed, 73 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 efd66835c2b8..a30f7ec31479 100644
> > --- a/arch/riscv/kvm/vcpu_pmu.c
> > +++ b/arch/riscv/kvm/vcpu_pmu.c
> > @@ -456,6 +456,73 @@ 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)
> > +{
> > +     unsigned long hva;
> > +     struct riscv_pmu_event_info *einfo;
> > +     int shmem_size = num_events * sizeof(*einfo);
> > +     bool writable;
> > +     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;
> > +             }
> > +     }
> > +
> > +     hva = kvm_vcpu_gfn_to_hva_prot(vcpu, shmem >> PAGE_SHIFT, &writable);
> > +     if (kvm_is_error_hva(hva) || !writable) {
> > +             ret = SBI_ERR_INVALID_ADDRESS;
>
> This only checks the first page if the address crosses a page boundary. Maybe
> that is okay since kvm_vcpu_read_guest()/kvm_vcpu_write_guest() will fail if a
> later page is inaccessible?
>

That's my assumption as well.
We can invoke kvm_vcpu_gfn_to_hva_prot in a loop to validate all the
pages starting with shmem though.
The difference would be the error code returned
(SBI_ERR_INVALID_ADDRESS vs SBI_ERR_FAILURE)
which the specification allows anyways.

Let me know if you think KVM must validate the entire range. I can add in v2.

> > +             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;
>
> This also needs to write `output` in the else case to indicate that the event is
> not supported; the spec does not require the caller to initialize bit 0 of
> output to zero.
>

Sure. Added.

> Regards,
> Samuel
>
> > +     }
> > +
> > +     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;
> >       }
> >
>

Re: [PATCH 5/8] drivers/perf: riscv: Implement PMU event info function

[Atish Kumar Patra]

On Mon, Dec 2, 2024 at 2:49 PM Samuel Holland <samuel.holland@sifive.com> wrote:
>
> Hi Atish,
>
> On 2024-11-19 2:29 PM, Atish Patra 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 |  7 +++++
> >  drivers/perf/riscv_pmu_sbi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 78 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 3ee9bfa5e77c..c04f64fbc01d 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -134,6 +134,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 {
> > @@ -157,6 +158,12 @@ struct riscv_pmu_snapshot_data {
> >       u64 reserved[447];
> >  };
> >
> > +struct riscv_pmu_event_info {
> > +     u32 event_idx;
> > +     u32 output;
> > +     u64 event_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 */
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index f0e845ff6b79..2a6527cc9d97 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);
>
> This new declaration does not appear to be used.
>

This is a forward declaration as  pmu_event_find_cache but it should
be in the next patch instead of this patch.
I have moved it to that patch.

> >  struct sbi_pmu_event_data {
> >       union {
> >               union {
> > @@ -299,6 +300,68 @@ 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 = (struct riscv_pmu_event_info *)
> > +                        kcalloc(num_events, sizeof(*event_info_shmem), GFP_KERNEL);
>
> Please drop the unnecessary cast.
>

Done.

> > +     if (!event_info_shmem) {
> > +             pr_err("Can not allocate memory for event info query\n");
>
> Usually there's no need to print an error for allocation failure, since the
> allocator already warns. And this isn't really an error, since we can (and do)
> fall back to the existing way of checking for events.
>

Fixed.

> > +             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 (int j = 0; j < ARRAY_SIZE(pmu_cache_event_map[i]); j++) {
> > +                     for (int 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;
> > +     }
> > +     /* Do we need some barriers here or priv mode transition will ensure that */
>
> No barrier is needed -- the SBI implementation is running on the same hart, so
> coherency isn't even a consideration.
>
> > +     for (i = 0; i < ARRAY_SIZE(pmu_hw_event_map); i++) {
> > +             if (!(event_info_shmem[i].output & 0x01))
>
> This bit mask should probably use a macro.
>
> > +                     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 & 0x01))
>
> Same comment applies here.
>

Done.


> Regards,
> Samuel
>
> > +                                     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 +379,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]);
> >
> >
>

Re: [PATCH 0/8] Add SBI v3.0 PMU enhancements

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 19 Nov 2024 12:29:48 -0800 you wrote:
> SBI v3.0 specification[1] added two new improvements to the PMU chaper.
> 
> 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].
> 
> [...]

Here is the summary with links:
  - [1/8] drivers/perf: riscv: Add SBI v3.0 flag
    (no matching commit)
  - [2/8] drivers/perf: riscv: Fix Platform firmware event data
    https://git.kernel.org/riscv/c/fc58db9aeb15
  - [3/8] drivers/perf: riscv: Add raw event v2 support
    (no matching commit)
  - [4/8] RISC-V: KVM: Add support for Raw event v2
    (no matching commit)
  - [5/8] drivers/perf: riscv: Implement PMU event info function
    (no matching commit)
  - [6/8] drivers/perf: riscv: Export PMU event info function
    (no matching commit)
  - [7/8] RISC-V: KVM: Implement get event info function
    (no matching commit)
  - [8/8] RISC-V: KVM: Upgrade the supported SBI version to 3.0
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html