2024-03-29 09:28:04

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 00/11] riscv: support Sdtrig extension hcontext/scontext CSRs

riscv-debug-spec [1] Chapter 5: Sdtrig extension introduces
trigger CSRs which can cause a breakpoint exception,
entry into Debug Mode, or a trace action without having to
execute a special instruction.

The focus in the following patches is on the two CSRs from
the Sdtrig extension: hcontext and scontext.

These two CSRs are optional according to the spec, apart from
the Smstateen extension [2], which has bit 57 to control the
accessbility of the hcontext/scontext CSRs.
We also introduce dt-binding in the CPU DTS for the existence of
the CSRs in situations where the Smstaten extension is not available.

The hcontext/scontext CSRs can help to raise triggers with the
textra32/textra64 CSRs set up correctly. (Chapter 5.7.17/ 5.7.18 [1])

Therefore, as part of Linux awareness debugging.
We propose the scontext CSR be filled by the Linux PID,
And the hcontext CSR be filled with a self-maintained Guest OS ID.

The reason for using the self-maintained Guest OS ID instead of VMID is
that VMID might change over time, and the user setting up the trigger
might enter the previous value, invoking the wrong VM for debugging.

The tests have been done on QEMU with Sdtrig CSRs
(mcontext/hcontext/scontext implemented) [3] boot on virt machine
and also run the Guest OS as virt machine with KVM enabled,
the two hcontext/scontext CSRs can be written correctly.

This patch series is based on v6.9-rc1.

Link: https://github.com/riscv/riscv-debug-spec/releases/download/ar20231208/riscv-debug-stable.pdf [1]
Link: https://github.com/riscvarchive/riscv-state-enable/releases/download/v1.0.0/Smstateen.pdf [2]
Link: https://github.com/sifive/qemu/tree/dev/maxh/sdtrig_ISA [3]

Signed-off-by: Max Hsu <[email protected]>
---
Max Hsu (7):
dt-bindings: riscv: Add Sdtrig ISA extension
dt-bindings: riscv: Add Sdtrig optional CSRs existence on DT
riscv: Add ISA extension parsing for Sdtrig
riscv: Add Sdtrig CSRs definition, Smstateen bit to access Sdtrig CSRs
riscv: cpufeature: Add Sdtrig optional CSRs checks
riscv: suspend: add Smstateen CSRs save/restore
riscv: Add task switch support for scontext CSR

Yong-Xuan Wang (4):
riscv: KVM: Add Sdtrig Extension Support for Guest/VM
riscv: KVM: Add scontext to ONE_REG
riscv: KVM: Add hcontext support
KVM: riscv: selftests: Add Sdtrig Extension to get-reg-list test

Documentation/devicetree/bindings/riscv/cpus.yaml | 18 +++
.../devicetree/bindings/riscv/extensions.yaml | 7 +
arch/riscv/include/asm/csr.h | 6 +
arch/riscv/include/asm/hwcap.h | 1 +
arch/riscv/include/asm/kvm_host.h | 14 ++
arch/riscv/include/asm/kvm_vcpu_debug.h | 24 +++
arch/riscv/include/asm/suspend.h | 7 +
arch/riscv/include/asm/switch_to.h | 15 ++
arch/riscv/include/uapi/asm/kvm.h | 9 ++
arch/riscv/kernel/cpufeature.c | 162 +++++++++++++++++++++
arch/riscv/kernel/suspend.c | 25 ++++
arch/riscv/kvm/Makefile | 1 +
arch/riscv/kvm/main.c | 4 +
arch/riscv/kvm/vcpu.c | 8 +
arch/riscv/kvm/vcpu_debug.c | 107 ++++++++++++++
arch/riscv/kvm/vcpu_onereg.c | 63 +++++++-
arch/riscv/kvm/vm.c | 4 +
tools/testing/selftests/kvm/riscv/get-reg-list.c | 27 ++++
18 files changed, 500 insertions(+), 2 deletions(-)
---
base-commit: 317c7bc0ef035d8ebfc3e55c5dde0566fd5fb171
change-id: 20240329-dev-maxh-lin-452-6-9-c6209e6db67f

Best regards,
--
Max Hsu <[email protected]>



2024-03-29 09:28:23

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 01/11] dt-bindings: riscv: Add Sdtrig ISA extension

As riscv-debug-spec [1] Chapter 5 introduce Sdtrig extension.

Add an entry for the Sdtrig extension to the riscv,isa-extensions property.

Link: https://github.com/riscv/riscv-debug-spec/releases/download/ar20231208/riscv-debug-stable.pdf [1]
Signed-off-by: Max Hsu <[email protected]>
---
Documentation/devicetree/bindings/riscv/extensions.yaml | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index 468c646247aa..47d82cd35ca7 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -121,6 +121,13 @@ properties:
version of the privileged ISA specification.

# multi-letter extensions, sorted alphanumerically
+ - const: sdtrig
+ description: |
+ The standard Sdtrig extension for introduce trigger CSRs for
+ cause a breakpoint exception, entry into Debug Mode,
+ or trace action as frozen at commit 359bedc ("Freeze Candidate")
+ of riscv-debug-spec
+
- const: smaia
description: |
The standard Smaia supervisor-level extension for the advanced

--
2.43.2


2024-03-29 09:29:03

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 03/11] riscv: Add ISA extension parsing for Sdtrig

Add ISA extension parsing for Sdtrig as introduced
in riscv-debug-spec [1] Chapter 5

Link: https://github.com/riscv/riscv-debug-spec/releases/download/ar20231208/riscv-debug-stable.pdf [1]
Signed-off-by: Max Hsu <[email protected]>
---
arch/riscv/include/asm/hwcap.h | 1 +
arch/riscv/kernel/cpufeature.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index e17d0078a651..9f8d780fce35 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -81,6 +81,7 @@
#define RISCV_ISA_EXT_ZTSO 72
#define RISCV_ISA_EXT_ZACAS 73
#define RISCV_ISA_EXT_XANDESPMU 74
+#define RISCV_ISA_EXT_SDTRIG 75

#define RISCV_ISA_EXT_XLINUXENVCFG 127

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 3ed2359eae35..080c06b76f53 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -296,6 +296,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
+ __RISCV_ISA_EXT_DATA(sdtrig, RISCV_ISA_EXT_SDTRIG),
__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),

--
2.43.2


2024-03-29 09:29:37

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 02/11] dt-bindings: riscv: Add Sdtrig optional CSRs existence on DT

The mcontext/hcontext/scontext CSRs are optional in the Sdtrig extension,
to prevent RW operations to the missing CSRs, which will cause
illegal instructions.

As a solution, we have proposed the dt format for these CSRs.

Signed-off-by: Max Hsu <[email protected]>
---
Documentation/devicetree/bindings/riscv/cpus.yaml | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index d87dd50f1a4b..c713a48c5025 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -137,6 +137,24 @@ properties:
DMIPS/MHz, relative to highest capacity-dmips-mhz
in the system.

+ debug:
+ type: object
+ properties:
+ compatible:
+ const: riscv,debug-v1.0.0
+ trigger-module:
+ type: object
+ description: |
+ An indication set of optional CSR existence from
+ riscv-debug-spec Sdtrig extension
+ properties:
+ mcontext-present:
+ type: boolean
+ hcontext-present:
+ type: boolean
+ scontext-present:
+ type: boolean
+
anyOf:
- required:
- riscv,isa

--
2.43.2


2024-03-29 09:29:48

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 04/11] riscv: Add Sdtrig CSRs definition, Smstateen bit to access Sdtrig CSRs

Add hcontext/scontext CSRs definition to csr.h

As riscv-state-enable [1] Smstateen extension spec:
Sdtrig CSRs: hcontext/scontext availability are controlled by
bit 57 of Smstateen CSRs.

Link: https://github.com/riscvarchive/riscv-state-enable/releases/download/v1.0.0/Smstateen.pdf [1]
Signed-off-by: Max Hsu <[email protected]>
---
arch/riscv/include/asm/csr.h | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 2468c55933cd..308ae795dc82 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -204,6 +204,8 @@
#define ENVCFG_FIOM _AC(0x1, UL)

/* Smstateen bits */
+#define SMSTATEEN0_HSCONTEXT_SHIFT 57
+#define SMSTATEEN0_HSCONTEXT (_ULL(1) << SMSTATEEN0_HSCONTEXT_SHIFT)
#define SMSTATEEN0_AIA_IMSIC_SHIFT 58
#define SMSTATEEN0_AIA_IMSIC (_ULL(1) << SMSTATEEN0_AIA_IMSIC_SHIFT)
#define SMSTATEEN0_AIA_SHIFT 59
@@ -480,6 +482,10 @@
#define IE_TIE (_AC(0x1, UL) << RV_IRQ_TIMER)
#define IE_EIE (_AC(0x1, UL) << RV_IRQ_EXT)

+/* riscv-debug-spec: Sdtrig extension */
+#define CSR_SCONTEXT 0x5a8
+#define CSR_HCONTEXT 0x6a8
+
#ifndef __ASSEMBLY__

#define csr_swap(csr, val) \

--
2.43.2


2024-03-29 09:30:05

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 05/11] riscv: cpufeature: Add Sdtrig optional CSRs checks

Sdtrig extension introduce two optional CSRs [hcontext/scontext],
that will be storing PID/Guest OS ID for the debug feature.

The availability of these two CSRs will be determined by
DTS and Smstateen extension [h/s]stateen0 CSR bit 57.

If all CPUs hcontext/scontext checks are satisfied, it will enable the
use_hcontext/use_scontext static branch.

Signed-off-by: Max Hsu <[email protected]>
---
arch/riscv/include/asm/switch_to.h | 6 ++
arch/riscv/kernel/cpufeature.c | 161 +++++++++++++++++++++++++++++++++++++
2 files changed, 167 insertions(+)

diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
index 7efdb0584d47..07432550ed54 100644
--- a/arch/riscv/include/asm/switch_to.h
+++ b/arch/riscv/include/asm/switch_to.h
@@ -69,6 +69,12 @@ static __always_inline bool has_fpu(void) { return false; }
#define __switch_to_fpu(__prev, __next) do { } while (0)
#endif

+DECLARE_STATIC_KEY_FALSE(use_scontext);
+static __always_inline bool has_scontext(void)
+{
+ return static_branch_likely(&use_scontext);
+}
+
extern struct task_struct *__switch_to(struct task_struct *,
struct task_struct *);

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 080c06b76f53..44ff84b920af 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -35,6 +35,19 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly;
/* Per-cpu ISA extensions. */
struct riscv_isainfo hart_isa[NR_CPUS];

+atomic_t hcontext_disable;
+atomic_t scontext_disable;
+
+DEFINE_STATIC_KEY_FALSE_RO(use_hcontext);
+EXPORT_SYMBOL(use_hcontext);
+
+DEFINE_STATIC_KEY_FALSE_RO(use_scontext);
+EXPORT_SYMBOL(use_scontext);
+
+/* Record the maximum number that the hcontext CSR allowed to hold */
+atomic_long_t hcontext_id_share;
+EXPORT_SYMBOL(hcontext_id_share);
+
/**
* riscv_isa_extension_base() - Get base extension word
*
@@ -719,6 +732,154 @@ unsigned long riscv_get_elf_hwcap(void)
return hwcap;
}

+static void __init sdtrig_percpu_csrs_check(void *data)
+{
+ struct device_node *node;
+ struct device_node *debug_node;
+ struct device_node *trigger_module;
+
+ unsigned int cpu = smp_processor_id();
+
+ /*
+ * Expect every cpu node has the [h/s]context-present property
+ * otherwise, jump to sdtrig_csrs_disable_all to disable all access to
+ * [h/s]context CSRs
+ */
+ node = of_cpu_device_node_get(cpu);
+ if (!node)
+ goto sdtrig_csrs_disable_all;
+
+ debug_node = of_get_compatible_child(node, "riscv,debug-v1.0.0");
+ of_node_put(node);
+
+ if (!debug_node)
+ goto sdtrig_csrs_disable_all;
+
+ trigger_module = of_get_child_by_name(debug_node, "trigger-module");
+ of_node_put(debug_node);
+
+ if (!trigger_module)
+ goto sdtrig_csrs_disable_all;
+
+ if (!(IS_ENABLED(CONFIG_KVM) &&
+ of_property_read_bool(trigger_module, "hcontext-present")))
+ atomic_inc(&hcontext_disable);
+
+ if (!of_property_read_bool(trigger_module, "scontext-present"))
+ atomic_inc(&scontext_disable);
+
+ of_node_put(trigger_module);
+
+ /*
+ * Before access to hcontext/scontext CSRs, if the smstateen
+ * extension is present, the accessibility will be controlled
+ * by the hstateen0[H]/sstateen0 CSRs.
+ */
+ if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_SMSTATEEN)) {
+ u64 hstateen_bit, sstateen_bit;
+
+ if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_h)) {
+#if __riscv_xlen > 32
+ csr_set(CSR_HSTATEEN0, SMSTATEEN0_HSCONTEXT);
+ hstateen_bit = csr_read(CSR_HSTATEEN0);
+#else
+ csr_set(CSR_HSTATEEN0H, SMSTATEEN0_HSCONTEXT >> 32);
+ hstateen_bit = csr_read(CSR_HSTATEEN0H) << 32;
+#endif
+ if (!(hstateen_bit & SMSTATEEN0_HSCONTEXT))
+ goto sdtrig_csrs_disable_all;
+
+ } else {
+ if (IS_ENABLED(CONFIG_KVM))
+ atomic_inc(&hcontext_disable);
+
+ /*
+ * In RV32, the smstateen extension doesn't provide
+ * high 32 bits of sstateen0 CSR which represent
+ * accessibility for scontext CSR;
+ * The decision is left on whether the dts has the
+ * property to access the scontext CSR.
+ */
+#if __riscv_xlen > 32
+ csr_set(CSR_SSTATEEN0, SMSTATEEN0_HSCONTEXT);
+ sstateen_bit = csr_read(CSR_SSTATEEN0);
+
+ if (!(sstateen_bit & SMSTATEEN0_HSCONTEXT))
+ atomic_inc(&scontext_disable);
+#endif
+ }
+ }
+
+ /*
+ * The code can only access hcontext/scontext CSRs if:
+ * The cpu dts node have [h/s]context-present;
+ * If Smstateen extension is presented, then the accessibility bit
+ * toward hcontext/scontext CSRs is enabled; Or the Smstateen extension
+ * isn't available, thus the access won't be blocked by it.
+ *
+ * With writing 1 to the every bit of these CSRs, we retrieve the
+ * maximum bits that is available on the CSRs. and decide
+ * whether it's suit for its context recording operation.
+ */
+ if (IS_ENABLED(CONFIG_KVM) &&
+ !atomic_read(&hcontext_disable)) {
+ unsigned long hcontext_available_bits = 0;
+
+ csr_write(CSR_HCONTEXT, -1UL);
+ hcontext_available_bits = csr_swap(CSR_HCONTEXT, hcontext_available_bits);
+
+ /* hcontext CSR is required by at least 1 bit */
+ if (hcontext_available_bits)
+ atomic_long_and(hcontext_available_bits, &hcontext_id_share);
+ else
+ atomic_inc(&hcontext_disable);
+ }
+
+ if (!atomic_read(&scontext_disable)) {
+ unsigned long scontext_available_bits = 0;
+
+ csr_write(CSR_SCONTEXT, -1UL);
+ scontext_available_bits = csr_swap(CSR_SCONTEXT, scontext_available_bits);
+
+ /* scontext CSR is required by at least the sizeof pid_t */
+ if (scontext_available_bits < ((1UL << (sizeof(pid_t) << 3)) - 1))
+ atomic_inc(&scontext_disable);
+ }
+
+ return;
+
+sdtrig_csrs_disable_all:
+ if (IS_ENABLED(CONFIG_KVM))
+ atomic_inc(&hcontext_disable);
+
+ atomic_inc(&scontext_disable);
+}
+
+static int __init sdtrig_enable_csrs_fill(void)
+{
+ if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_SDTRIG)) {
+ atomic_long_set(&hcontext_id_share, -1UL);
+
+ /* check every CPUs sdtrig extension optional CSRs */
+ sdtrig_percpu_csrs_check(NULL);
+ smp_call_function(sdtrig_percpu_csrs_check, NULL, 1);
+
+ if (IS_ENABLED(CONFIG_KVM) &&
+ !atomic_read(&hcontext_disable)) {
+ pr_info("riscv-sdtrig: Writing 'GuestOS ID' to hcontext CSR is enabled\n");
+ static_branch_enable(&use_hcontext);
+ }
+
+ if (!atomic_read(&scontext_disable)) {
+ pr_info("riscv-sdtrig: Writing 'PID' to scontext CSR is enabled\n");
+ static_branch_enable(&use_scontext);
+ }
+ }
+ return 0;
+}
+
+arch_initcall(sdtrig_enable_csrs_fill);
+
void riscv_user_isa_enable(void)
{
if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))

--
2.43.2


2024-03-29 09:30:29

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 06/11] riscv: suspend: add Smstateen CSRs save/restore

From Smstateen extension: the values of the [h/s]stateen CSRs will
be lost when entering a non-retentive idle state.
Therefore, these CSRs values need to be restored to ensure that
the corresponding functionality remains enabled.

Signed-off-by: Max Hsu <[email protected]>
---
arch/riscv/include/asm/suspend.h | 6 ++++++
arch/riscv/kernel/suspend.c | 18 ++++++++++++++++++
2 files changed, 24 insertions(+)

diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
index 4718096fa5e3..2ecace073869 100644
--- a/arch/riscv/include/asm/suspend.h
+++ b/arch/riscv/include/asm/suspend.h
@@ -17,6 +17,12 @@ struct suspend_context {
unsigned long envcfg;
unsigned long tvec;
unsigned long ie;
+#if __riscv_xlen < 64
+ unsigned long hstateen0h;
+#endif
+ unsigned long hstateen0;
+ unsigned long sstateen0;
+
#ifdef CONFIG_MMU
unsigned long satp;
#endif
diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
index 8a327b485b90..a086da222872 100644
--- a/arch/riscv/kernel/suspend.c
+++ b/arch/riscv/kernel/suspend.c
@@ -19,6 +19,15 @@ void suspend_save_csrs(struct suspend_context *context)
context->envcfg = csr_read(CSR_ENVCFG);
context->tvec = csr_read(CSR_TVEC);
context->ie = csr_read(CSR_IE);
+ if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN)) {
+ if (riscv_has_extension_unlikely(RISCV_ISA_EXT_h)) {
+#if __riscv_xlen < 64
+ context->hstateen0h = csr_read(CSR_HSTATEEN0H);
+#endif
+ context->hstateen0 = csr_read(CSR_HSTATEEN0);
+ }
+ context->sstateen0 = csr_read(CSR_SSTATEEN0);
+ }

/*
* No need to save/restore IP CSR (i.e. MIP or SIP) because:
@@ -42,6 +51,15 @@ void suspend_restore_csrs(struct suspend_context *context)
csr_write(CSR_ENVCFG, context->envcfg);
csr_write(CSR_TVEC, context->tvec);
csr_write(CSR_IE, context->ie);
+ if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN)) {
+ if (riscv_has_extension_unlikely(RISCV_ISA_EXT_h)) {
+#if __riscv_xlen < 64
+ csr_write(CSR_HSTATEEN0H, context->hstateen0h);
+#endif
+ csr_write(CSR_HSTATEEN0, context->hstateen0);
+ }
+ csr_write(CSR_SSTATEEN0, context->sstateen0);
+ }

#ifdef CONFIG_MMU
csr_write(CSR_SATP, context->satp);

--
2.43.2


2024-03-29 09:30:45

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 07/11] riscv: Add task switch support for scontext CSR

Write the next task PID to the scontext CSR if the use_scontext
static branch is enabled by the detection of the cpufeature.c

The scontext CSR needs to be saved and restored when entering
a non-retentive idle state so that when resuming the CPU,
the task's PID on the scontext CSR will be correct.

Co-developed-by: Nick Hu <[email protected]>
Signed-off-by: Nick Hu <[email protected]>
Signed-off-by: Max Hsu <[email protected]>
---
arch/riscv/include/asm/suspend.h | 1 +
arch/riscv/include/asm/switch_to.h | 9 +++++++++
arch/riscv/kernel/suspend.c | 7 +++++++
3 files changed, 17 insertions(+)

diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
index 2ecace073869..5021cad7e815 100644
--- a/arch/riscv/include/asm/suspend.h
+++ b/arch/riscv/include/asm/suspend.h
@@ -13,6 +13,7 @@ struct suspend_context {
/* Saved and restored by low-level functions */
struct pt_regs regs;
/* Saved and restored by high-level functions */
+ unsigned long scontext;
unsigned long scratch;
unsigned long envcfg;
unsigned long tvec;
diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
index 07432550ed54..289cd6b60978 100644
--- a/arch/riscv/include/asm/switch_to.h
+++ b/arch/riscv/include/asm/switch_to.h
@@ -8,6 +8,7 @@

#include <linux/jump_label.h>
#include <linux/sched/task_stack.h>
+#include <linux/pid.h>
#include <asm/vector.h>
#include <asm/cpufeature.h>
#include <asm/processor.h>
@@ -75,6 +76,12 @@ static __always_inline bool has_scontext(void)
return static_branch_likely(&use_scontext);
}

+static __always_inline void __switch_to_scontext(struct task_struct *__prev,
+ struct task_struct *__next)
+{
+ csr_write(CSR_SCONTEXT, task_pid_nr(__next));
+}
+
extern struct task_struct *__switch_to(struct task_struct *,
struct task_struct *);

@@ -86,6 +93,8 @@ do { \
__switch_to_fpu(__prev, __next); \
if (has_vector()) \
__switch_to_vector(__prev, __next); \
+ if (has_scontext()) \
+ __switch_to_scontext(__prev, __next); \
((last) = __switch_to(__prev, __next)); \
} while (0)

diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
index a086da222872..6b403a1f75c3 100644
--- a/arch/riscv/kernel/suspend.c
+++ b/arch/riscv/kernel/suspend.c
@@ -11,9 +11,13 @@
#include <asm/csr.h>
#include <asm/sbi.h>
#include <asm/suspend.h>
+#include <asm/switch_to.h>

void suspend_save_csrs(struct suspend_context *context)
{
+ if (has_scontext())
+ context->scontext = csr_read(CSR_SCONTEXT);
+
context->scratch = csr_read(CSR_SCRATCH);
if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
context->envcfg = csr_read(CSR_ENVCFG);
@@ -46,6 +50,9 @@ void suspend_save_csrs(struct suspend_context *context)

void suspend_restore_csrs(struct suspend_context *context)
{
+ if (has_scontext())
+ csr_write(CSR_SCONTEXT, context->scontext);
+
csr_write(CSR_SCRATCH, context->scratch);
if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
csr_write(CSR_ENVCFG, context->envcfg);

--
2.43.2


2024-03-29 09:31:08

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 08/11] riscv: KVM: Add Sdtrig Extension Support for Guest/VM

From: Yong-Xuan Wang <[email protected]>

We extend the KVM ISA extension ONE_REG interface to allow VMM
tools to detect and enable Sdtrig extension for Guest/VM. We
also save/restore the scontext CSR for guest VCPUs and set the
HSCONTEXT bit in hstateen0 CSR if the scontext CSR is available
for Guest/VM when the Smstateen extension is present.

Signed-off-by: Yong-Xuan Wang <[email protected]>
Co-developed-by: Max Hsu <[email protected]>
Signed-off-by: Max Hsu <[email protected]>
---
arch/riscv/include/asm/kvm_host.h | 11 +++++++++++
arch/riscv/include/asm/kvm_vcpu_debug.h | 17 +++++++++++++++++
arch/riscv/include/uapi/asm/kvm.h | 1 +
arch/riscv/kvm/Makefile | 1 +
arch/riscv/kvm/vcpu.c | 8 ++++++++
arch/riscv/kvm/vcpu_debug.c | 29 +++++++++++++++++++++++++++++
arch/riscv/kvm/vcpu_onereg.c | 1 +
7 files changed, 68 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index 484d04a92fa6..d495279d99e1 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -21,6 +21,7 @@
#include <asm/kvm_vcpu_sbi.h>
#include <asm/kvm_vcpu_timer.h>
#include <asm/kvm_vcpu_pmu.h>
+#include <asm/kvm_vcpu_debug.h>

#define KVM_MAX_VCPUS 1024

@@ -175,6 +176,10 @@ struct kvm_vcpu_smstateen_csr {
unsigned long sstateen0;
};

+struct kvm_vcpu_sdtrig_csr {
+ unsigned long scontext;
+};
+
struct kvm_vcpu_arch {
/* VCPU ran at least once */
bool ran_atleast_once;
@@ -197,6 +202,9 @@ struct kvm_vcpu_arch {
unsigned long host_senvcfg;
unsigned long host_sstateen0;

+ /* SCONTEXT of Host */
+ unsigned long host_scontext;
+
/* CPU context of Host */
struct kvm_cpu_context host_context;

@@ -209,6 +217,9 @@ struct kvm_vcpu_arch {
/* CPU Smstateen CSR context of Guest VCPU */
struct kvm_vcpu_smstateen_csr smstateen_csr;

+ /* CPU Sdtrig CSR context of Guest VCPU */
+ struct kvm_vcpu_sdtrig_csr sdtrig_csr;
+
/* CPU context upon Guest VCPU reset */
struct kvm_cpu_context guest_reset_context;

diff --git a/arch/riscv/include/asm/kvm_vcpu_debug.h b/arch/riscv/include/asm/kvm_vcpu_debug.h
new file mode 100644
index 000000000000..6e7ce6b408a6
--- /dev/null
+++ b/arch/riscv/include/asm/kvm_vcpu_debug.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2024 SiFive
+ *
+ * Authors:
+ * Yong-Xuan Wang <[email protected]>
+ */
+
+#ifndef __KVM_VCPU_RISCV_DEBUG_H
+#define __KVM_VCPU_RISCV_DEBUG_H
+
+#include <linux/types.h>
+
+void kvm_riscv_debug_vcpu_swap_in_guest_context(struct kvm_vcpu *vcpu);
+void kvm_riscv_debug_vcpu_swap_in_host_context(struct kvm_vcpu *vcpu);
+
+#endif
diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index b1c503c2959c..9f70da85ed51 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -167,6 +167,7 @@ enum KVM_RISCV_ISA_EXT_ID {
KVM_RISCV_ISA_EXT_ZFA,
KVM_RISCV_ISA_EXT_ZTSO,
KVM_RISCV_ISA_EXT_ZACAS,
+ KVM_RISCV_ISA_EXT_SDTRIG,
KVM_RISCV_ISA_EXT_MAX,
};

diff --git a/arch/riscv/kvm/Makefile b/arch/riscv/kvm/Makefile
index c9646521f113..387be968d9ea 100644
--- a/arch/riscv/kvm/Makefile
+++ b/arch/riscv/kvm/Makefile
@@ -15,6 +15,7 @@ kvm-y += vmid.o
kvm-y += tlb.o
kvm-y += mmu.o
kvm-y += vcpu.o
+kvm-y += vcpu_debug.o
kvm-y += vcpu_exit.o
kvm-y += vcpu_fp.o
kvm-y += vcpu_vector.o
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index b5ca9f2e98ac..1d0e43ab0652 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -20,6 +20,7 @@
#include <asm/csr.h>
#include <asm/cacheflush.h>
#include <asm/kvm_vcpu_vector.h>
+#include <asm/switch_to.h>

const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
KVM_GENERIC_VCPU_STATS(),
@@ -504,6 +505,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
SMSTATEEN0_AIA_ISEL;
if (riscv_isa_extension_available(isa, SMSTATEEN))
cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
+
+ if (has_scontext())
+ cfg->hstateen0 |= SMSTATEEN0_HSCONTEXT;
}
}

@@ -643,6 +647,8 @@ static __always_inline void kvm_riscv_vcpu_swap_in_guest_state(struct kvm_vcpu *
(cfg->hstateen0 & SMSTATEEN0_SSTATEEN0))
vcpu->arch.host_sstateen0 = csr_swap(CSR_SSTATEEN0,
smcsr->sstateen0);
+
+ kvm_riscv_debug_vcpu_swap_in_guest_context(vcpu);
}

static __always_inline void kvm_riscv_vcpu_swap_in_host_state(struct kvm_vcpu *vcpu)
@@ -656,6 +662,8 @@ static __always_inline void kvm_riscv_vcpu_swap_in_host_state(struct kvm_vcpu *v
(cfg->hstateen0 & SMSTATEEN0_SSTATEEN0))
smcsr->sstateen0 = csr_swap(CSR_SSTATEEN0,
vcpu->arch.host_sstateen0);
+
+ kvm_riscv_debug_vcpu_swap_in_host_context(vcpu);
}

/*
diff --git a/arch/riscv/kvm/vcpu_debug.c b/arch/riscv/kvm/vcpu_debug.c
new file mode 100644
index 000000000000..e7e9263c2e30
--- /dev/null
+++ b/arch/riscv/kvm/vcpu_debug.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 SiFive
+ */
+
+#include <linux/kvm_host.h>
+#include <asm/switch_to.h>
+
+void kvm_riscv_debug_vcpu_swap_in_guest_context(struct kvm_vcpu *vcpu)
+{
+ struct kvm_vcpu_sdtrig_csr *csr = &vcpu->arch.sdtrig_csr;
+ unsigned long hcontext = vcpu->kvm->arch.hcontext;
+
+ if (has_hcontext())
+ csr_write(CSR_HCONTEXT, hcontext);
+ if (has_scontext())
+ vcpu->arch.host_scontext = csr_swap(CSR_SCONTEXT, csr->scontext);
+}
+
+void kvm_riscv_debug_vcpu_swap_in_host_context(struct kvm_vcpu *vcpu)
+{
+ struct kvm_vcpu_sdtrig_csr *csr = &vcpu->arch.sdtrig_csr;
+
+ /* Hypervisor uses the hcontext ID 0 */
+ if (has_hcontext())
+ csr_write(CSR_HCONTEXT, 0);
+ if (has_scontext())
+ csr->scontext = csr_swap(CSR_SCONTEXT, vcpu->arch.host_scontext);
+}
diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index f4a6124d25c9..10dda5ddc0a6 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -34,6 +34,7 @@ static const unsigned long kvm_isa_ext_arr[] = {
[KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m,
[KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v,
/* Multi letter extensions (alphabetically sorted) */
+ KVM_ISA_EXT_ARR(SDTRIG),
KVM_ISA_EXT_ARR(SMSTATEEN),
KVM_ISA_EXT_ARR(SSAIA),
KVM_ISA_EXT_ARR(SSTC),

--
2.43.2


2024-03-29 09:31:24

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 09/11] riscv: KVM: Add scontext to ONE_REG

From: Yong-Xuan Wang <[email protected]>

Updte the ONE_REG interface to allow the scontext CSR can be accessed from
user space.

Signed-off-by: Yong-Xuan Wang <[email protected]>
Co-developed-by: Max Hsu <[email protected]>
Signed-off-by: Max Hsu <[email protected]>
---
arch/riscv/include/uapi/asm/kvm.h | 8 +++++
arch/riscv/kvm/vcpu_onereg.c | 62 +++++++++++++++++++++++++++++++++++++--
2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index 9f70da85ed51..1886722127d7 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -98,6 +98,11 @@ struct kvm_riscv_smstateen_csr {
unsigned long sstateen0;
};

+/* Sdtrig CSR for KVM_GET_ONE_REG and KVM_SET_ONE_REG */
+struct kvm_riscv_sdtrig_csr {
+ unsigned long scontext;
+};
+
/* TIMER registers for KVM_GET_ONE_REG and KVM_SET_ONE_REG */
struct kvm_riscv_timer {
__u64 frequency;
@@ -224,12 +229,15 @@ struct kvm_riscv_sbi_sta {
#define KVM_REG_RISCV_CSR_GENERAL (0x0 << KVM_REG_RISCV_SUBTYPE_SHIFT)
#define KVM_REG_RISCV_CSR_AIA (0x1 << KVM_REG_RISCV_SUBTYPE_SHIFT)
#define KVM_REG_RISCV_CSR_SMSTATEEN (0x2 << KVM_REG_RISCV_SUBTYPE_SHIFT)
+#define KVM_REG_RISCV_CSR_SDTRIG (0x3 << KVM_REG_RISCV_SUBTYPE_SHIFT)
#define KVM_REG_RISCV_CSR_REG(name) \
(offsetof(struct kvm_riscv_csr, name) / sizeof(unsigned long))
#define KVM_REG_RISCV_CSR_AIA_REG(name) \
(offsetof(struct kvm_riscv_aia_csr, name) / sizeof(unsigned long))
#define KVM_REG_RISCV_CSR_SMSTATEEN_REG(name) \
(offsetof(struct kvm_riscv_smstateen_csr, name) / sizeof(unsigned long))
+#define KVM_REG_RISCV_CSR_SDTRIG_REG(name) \
+ (offsetof(struct kvm_riscv_sdtrig_csr, name) / sizeof(unsigned long))

/* Timer registers are mapped as type 4 */
#define KVM_REG_RISCV_TIMER (0x04 << KVM_REG_RISCV_TYPE_SHIFT)
diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index 10dda5ddc0a6..2796a86ec70b 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -471,6 +471,34 @@ static int kvm_riscv_vcpu_smstateen_get_csr(struct kvm_vcpu *vcpu,
return 0;
}

+static inline int kvm_riscv_vcpu_sdtrig_set_csr(struct kvm_vcpu *vcpu,
+ unsigned long reg_num,
+ unsigned long reg_val)
+{
+ struct kvm_vcpu_sdtrig_csr *csr = &vcpu->arch.sdtrig_csr;
+
+ if (reg_num >= sizeof(struct kvm_riscv_sdtrig_csr) /
+ sizeof(unsigned long))
+ return -EINVAL;
+
+ ((unsigned long *)csr)[reg_num] = reg_val;
+ return 0;
+}
+
+static int kvm_riscv_vcpu_sdtrig_get_csr(struct kvm_vcpu *vcpu,
+ unsigned long reg_num,
+ unsigned long *out_val)
+{
+ struct kvm_vcpu_sdtrig_csr *csr = &vcpu->arch.sdtrig_csr;
+
+ if (reg_num >= sizeof(struct kvm_riscv_sdtrig_csr) /
+ sizeof(unsigned long))
+ return -EINVAL;
+
+ *out_val = ((unsigned long *)csr)[reg_num];
+ return 0;
+}
+
static int kvm_riscv_vcpu_get_reg_csr(struct kvm_vcpu *vcpu,
const struct kvm_one_reg *reg)
{
@@ -500,6 +528,11 @@ static int kvm_riscv_vcpu_get_reg_csr(struct kvm_vcpu *vcpu,
rc = kvm_riscv_vcpu_smstateen_get_csr(vcpu, reg_num,
&reg_val);
break;
+ case KVM_REG_RISCV_CSR_SDTRIG:
+ rc = -EINVAL;
+ if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SDTRIG))
+ rc = kvm_riscv_vcpu_sdtrig_get_csr(vcpu, reg_num, &reg_val);
+ break;
default:
rc = -ENOENT;
break;
@@ -545,6 +578,11 @@ static int kvm_riscv_vcpu_set_reg_csr(struct kvm_vcpu *vcpu,
rc = kvm_riscv_vcpu_smstateen_set_csr(vcpu, reg_num,
reg_val);
break;
+ case KVM_REG_RISCV_CSR_SDTRIG:
+ rc = -EINVAL;
+ if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SDTRIG))
+ rc = kvm_riscv_vcpu_sdtrig_set_csr(vcpu, reg_num, reg_val);
+ break;
default:
rc = -ENOENT;
break;
@@ -803,6 +841,8 @@ static inline unsigned long num_csr_regs(const struct kvm_vcpu *vcpu)
n += sizeof(struct kvm_riscv_aia_csr) / sizeof(unsigned long);
if (riscv_isa_extension_available(vcpu->arch.isa, SMSTATEEN))
n += sizeof(struct kvm_riscv_smstateen_csr) / sizeof(unsigned long);
+ if (riscv_isa_extension_available(vcpu->arch.isa, SDTRIG))
+ n += sizeof(struct kvm_riscv_sdtrig_csr) / sizeof(unsigned long);

return n;
}
@@ -811,7 +851,7 @@ static int copy_csr_reg_indices(const struct kvm_vcpu *vcpu,
u64 __user *uindices)
{
int n1 = sizeof(struct kvm_riscv_csr) / sizeof(unsigned long);
- int n2 = 0, n3 = 0;
+ int n2 = 0, n3 = 0, n4 = 0;

/* copy general csr regs */
for (int i = 0; i < n1; i++) {
@@ -863,7 +903,25 @@ static int copy_csr_reg_indices(const struct kvm_vcpu *vcpu,
}
}

- return n1 + n2 + n3;
+ /* copy Sdtrig csr regs */
+ if (riscv_isa_extension_available(vcpu->arch.isa, SDTRIG)) {
+ n4 = sizeof(struct kvm_riscv_sdtrig_csr) / sizeof(unsigned long);
+
+ for (int i = 0; i < n4; i++) {
+ u64 size = IS_ENABLED(CONFIG_32BIT) ?
+ KVM_REG_SIZE_U32 : KVM_REG_SIZE_U64;
+ u64 reg = KVM_REG_RISCV | size | KVM_REG_RISCV_CSR |
+ KVM_REG_RISCV_CSR_SDTRIG | i;
+
+ if (uindices) {
+ if (put_user(reg, uindices))
+ return -EFAULT;
+ uindices++;
+ }
+ }
+ }
+
+ return n1 + n2 + n3 + n4;
}

static inline unsigned long num_timer_regs(void)

--
2.43.2


2024-03-29 09:31:49

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 10/11] riscv: KVM: Add hcontext support

From: Yong-Xuan Wang <[email protected]>

hcontext CSR store the ID of the currently running machine status.
When a virtual machine is initialized, it will obtain and utilize
the first available ID.
It will be updated to VM ID when switch to a virtual machine,
and updated to 0 when switch back to host machine.

Signed-off-by: Yong-Xuan Wang <[email protected]>
Co-developed-by: Max Hsu <[email protected]>
Signed-off-by: Max Hsu <[email protected]>
---
arch/riscv/include/asm/kvm_host.h | 3 ++
arch/riscv/include/asm/kvm_vcpu_debug.h | 7 +++
arch/riscv/kvm/main.c | 4 ++
arch/riscv/kvm/vcpu_debug.c | 78 +++++++++++++++++++++++++++++++++
arch/riscv/kvm/vm.c | 4 ++
5 files changed, 96 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index d495279d99e1..b5d972783116 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -103,6 +103,9 @@ struct kvm_arch {

/* AIA Guest/VM context */
struct kvm_aia aia;
+
+ /* hcontext ID for guest VM */
+ unsigned long hcontext;
};

struct kvm_cpu_trap {
diff --git a/arch/riscv/include/asm/kvm_vcpu_debug.h b/arch/riscv/include/asm/kvm_vcpu_debug.h
index 6e7ce6b408a6..0a025fc4e6dd 100644
--- a/arch/riscv/include/asm/kvm_vcpu_debug.h
+++ b/arch/riscv/include/asm/kvm_vcpu_debug.h
@@ -11,6 +11,13 @@

#include <linux/types.h>

+DECLARE_STATIC_KEY_FALSE(use_hcontext);
+extern atomic_long_t hcontext_id_share;
+
+void kvm_riscv_debug_init(void);
+void kvm_riscv_debug_exit(void);
+void kvm_riscv_debug_get_hcontext_id(struct kvm *kvm);
+void kvm_riscv_debug_return_hcontext_id(struct kvm *kvm);
void kvm_riscv_debug_vcpu_swap_in_guest_context(struct kvm_vcpu *vcpu);
void kvm_riscv_debug_vcpu_swap_in_host_context(struct kvm_vcpu *vcpu);

diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
index 225a435d9c9a..ff28b96ad70b 100644
--- a/arch/riscv/kvm/main.c
+++ b/arch/riscv/kvm/main.c
@@ -125,6 +125,8 @@ static int __init riscv_kvm_init(void)
return rc;
}

+ kvm_riscv_debug_init();
+
return 0;
}
module_init(riscv_kvm_init);
@@ -133,6 +135,8 @@ static void __exit riscv_kvm_exit(void)
{
kvm_riscv_aia_exit();

+ kvm_riscv_debug_exit();
+
kvm_exit();
}
module_exit(riscv_kvm_exit);
diff --git a/arch/riscv/kvm/vcpu_debug.c b/arch/riscv/kvm/vcpu_debug.c
index e7e9263c2e30..5081c272f01d 100644
--- a/arch/riscv/kvm/vcpu_debug.c
+++ b/arch/riscv/kvm/vcpu_debug.c
@@ -6,6 +6,84 @@
#include <linux/kvm_host.h>
#include <asm/switch_to.h>

+DEFINE_SPINLOCK(hcontext_lock);
+unsigned long *hcontext_bitmap;
+unsigned long hcontext_bitmap_len;
+
+static __always_inline bool has_hcontext(void)
+{
+ return static_branch_likely(&use_hcontext);
+}
+
+void kvm_riscv_debug_init(void)
+{
+ /*
+ * As from riscv-debug-spec, Chapter 5.7.9:
+ * If the H extension is implemented, it’s recommended to
+ * implement no more than 7 bits on RV32 and 14 on RV64.
+ * Allocating bit array according to spec size.
+ */
+#if __riscv_xlen > 32
+ unsigned long tmp = atomic_long_read(&hcontext_id_share) & GENMASK(13, 0);
+#else
+ unsigned long tmp = atomic_long_read(&hcontext_id_share) & GENMASK(6, 0);
+#endif
+ if (has_hcontext()) {
+ while (tmp) {
+ kvm_info("hcontext: try to allocate 0x%lx-bit array\n", tmp);
+ hcontext_bitmap_len = tmp + 1;
+ hcontext_bitmap = bitmap_zalloc(tmp, 0);
+ if (hcontext_bitmap)
+ break;
+ tmp = tmp >> 1;
+ }
+
+ if (tmp == 0) {
+ /* We can't allocate any space for hcontext bitmap */
+ static_branch_disable(&use_hcontext);
+ } else {
+ /* ID 0 is hypervisor */
+ set_bit(0, hcontext_bitmap);
+ }
+ }
+}
+
+void kvm_riscv_debug_exit(void)
+{
+ if (has_hcontext()) {
+ static_branch_disable(&use_hcontext);
+ kfree(hcontext_bitmap);
+ }
+}
+
+void kvm_riscv_debug_get_hcontext_id(struct kvm *kvm)
+{
+ if (has_hcontext()) {
+ unsigned long free_id;
+
+ spin_lock(&hcontext_lock);
+ free_id = find_first_zero_bit(hcontext_bitmap, hcontext_bitmap_len);
+
+ /* share the maximum ID when we run out of the hcontext ID */
+ if (free_id <= hcontext_bitmap_len)
+ set_bit(free_id, hcontext_bitmap);
+ else
+ free_id -= 1;
+
+ kvm->arch.hcontext = free_id;
+ spin_unlock(&hcontext_lock);
+ }
+}
+
+void kvm_riscv_debug_return_hcontext_id(struct kvm *kvm)
+{
+ if (has_hcontext()) {
+ spin_lock(&hcontext_lock);
+ clear_bit(kvm->arch.hcontext, hcontext_bitmap);
+ spin_unlock(&hcontext_lock);
+ }
+}
+
void kvm_riscv_debug_vcpu_swap_in_guest_context(struct kvm_vcpu *vcpu)
{
struct kvm_vcpu_sdtrig_csr *csr = &vcpu->arch.sdtrig_csr;
diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
index ce58bc48e5b8..275f5f05d4dd 100644
--- a/arch/riscv/kvm/vm.c
+++ b/arch/riscv/kvm/vm.c
@@ -45,6 +45,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)

kvm_riscv_guest_timer_init(kvm);

+ kvm_riscv_debug_get_hcontext_id(kvm);
+
return 0;
}

@@ -53,6 +55,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
kvm_destroy_vcpus(kvm);

kvm_riscv_aia_destroy_vm(kvm);
+
+ kvm_riscv_debug_return_hcontext_id(kvm);
}

int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irql,

--
2.43.2


2024-03-29 09:32:10

by Max Hsu

[permalink] [raw]
Subject: [PATCH RFC 11/11] KVM: riscv: selftests: Add Sdtrig Extension to get-reg-list test

From: Yong-Xuan Wang <[email protected]>

Update the get-reg-list test to test the Sdtrig Extension is available
for guest OS.

Signed-off-by: Yong-Xuan Wang <[email protected]>
Co-developed-by: Max Hsu <[email protected]>
Signed-off-by: Max Hsu <[email protected]>
---
tools/testing/selftests/kvm/riscv/get-reg-list.c | 27 ++++++++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
index b882b7b9b785..f2696e308509 100644
--- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
+++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
@@ -41,6 +41,7 @@ bool filter_reg(__u64 reg)
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_I:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_M:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_V:
+ case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SDTRIG:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SMSTATEEN:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SSAIA:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SSTC:
@@ -247,6 +248,8 @@ static const char *core_id_to_str(const char *prefix, __u64 id)
"KVM_REG_RISCV_CSR_AIA | KVM_REG_RISCV_CSR_REG(" #csr ")"
#define RISCV_CSR_SMSTATEEN(csr) \
"KVM_REG_RISCV_CSR_SMSTATEEN | KVM_REG_RISCV_CSR_REG(" #csr ")"
+#define RISCV_CSR_SDTRIG(csr) \
+ "KVM_REG_RISCV_CSR_SDTRIG | KVM_REG_RISCV_CSR_REG(" #csr ")"

static const char *general_csr_id_to_str(__u64 reg_off)
{
@@ -314,6 +317,18 @@ static const char *smstateen_csr_id_to_str(__u64 reg_off)
return NULL;
}

+static const char *sdtrig_csr_id_to_str(__u64 reg_off)
+{
+ /* reg_off is the offset into struct kvm_riscv_smstateen_csr */
+ switch (reg_off) {
+ case KVM_REG_RISCV_CSR_SDTRIG_REG(scontext):
+ return RISCV_CSR_SDTRIG(scontext);
+ }
+
+ TEST_FAIL("Unknown sdtrig csr reg: 0x%llx", reg_off);
+ return NULL;
+}
+
static const char *csr_id_to_str(const char *prefix, __u64 id)
{
__u64 reg_off = id & ~(REG_MASK | KVM_REG_RISCV_CSR);
@@ -330,6 +345,8 @@ static const char *csr_id_to_str(const char *prefix, __u64 id)
return aia_csr_id_to_str(reg_off);
case KVM_REG_RISCV_CSR_SMSTATEEN:
return smstateen_csr_id_to_str(reg_off);
+ case KVM_REG_RISCV_CSR_SDTRIG:
+ return sdtrig_csr_id_to_str(reg_off);
}

return strdup_printf("%lld | %lld /* UNKNOWN */", reg_subtype, reg_off);
@@ -406,6 +423,7 @@ static const char *isa_ext_single_id_to_str(__u64 reg_off)
KVM_ISA_EXT_ARR(I),
KVM_ISA_EXT_ARR(M),
KVM_ISA_EXT_ARR(V),
+ KVM_ISA_EXT_ARR(SDTRIG),
KVM_ISA_EXT_ARR(SMSTATEEN),
KVM_ISA_EXT_ARR(SSAIA),
KVM_ISA_EXT_ARR(SSTC),
@@ -764,6 +782,11 @@ static __u64 smstateen_regs[] = {
KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SMSTATEEN,
};

+static __u64 sdtrig_regs[] = {
+ KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_CSR | KVM_REG_RISCV_CSR_SDTRIG | KVM_REG_RISCV_CSR_SDTRIG_REG(scontext),
+ KVM_REG_RISCV | KVM_REG_SIZE_ULONG | KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SDTRIG,
+};
+
static __u64 fp_f_regs[] = {
KVM_REG_RISCV | KVM_REG_SIZE_U32 | KVM_REG_RISCV_FP_F | KVM_REG_RISCV_FP_F_REG(f[0]),
KVM_REG_RISCV | KVM_REG_SIZE_U32 | KVM_REG_RISCV_FP_F | KVM_REG_RISCV_FP_F_REG(f[1]),
@@ -853,6 +876,8 @@ static __u64 fp_d_regs[] = {
{"zicboz", .feature = KVM_RISCV_ISA_EXT_ZICBOZ, .regs = zicboz_regs, .regs_n = ARRAY_SIZE(zicboz_regs),}
#define SUBLIST_AIA \
{"aia", .feature = KVM_RISCV_ISA_EXT_SSAIA, .regs = aia_regs, .regs_n = ARRAY_SIZE(aia_regs),}
+#define SUBLIST_SDTRIG \
+ {"sdtrig", .feature = KVM_RISCV_ISA_EXT_SDTRIG, .regs = sdtrig_regs, .regs_n = ARRAY_SIZE(sdtrig_regs),}
#define SUBLIST_SMSTATEEN \
{"smstateen", .feature = KVM_RISCV_ISA_EXT_SMSTATEEN, .regs = smstateen_regs, .regs_n = ARRAY_SIZE(smstateen_regs),}
#define SUBLIST_FP_F \
@@ -930,6 +955,7 @@ KVM_ISA_EXT_SUBLIST_CONFIG(aia, AIA);
KVM_ISA_EXT_SUBLIST_CONFIG(fp_f, FP_F);
KVM_ISA_EXT_SUBLIST_CONFIG(fp_d, FP_D);
KVM_ISA_EXT_SIMPLE_CONFIG(h, H);
+KVM_ISA_EXT_SUBLIST_CONFIG(sdtrig, SDTRIG);
KVM_ISA_EXT_SUBLIST_CONFIG(smstateen, SMSTATEEN);
KVM_ISA_EXT_SIMPLE_CONFIG(sstc, SSTC);
KVM_ISA_EXT_SIMPLE_CONFIG(svinval, SVINVAL);
@@ -985,6 +1011,7 @@ struct vcpu_reg_list *vcpu_configs[] = {
&config_fp_f,
&config_fp_d,
&config_h,
+ &config_sdtrig,
&config_smstateen,
&config_sstc,
&config_svinval,

--
2.43.2


2024-03-29 10:23:16

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH RFC 05/11] riscv: cpufeature: Add Sdtrig optional CSRs checks

On Fri, Mar 29, 2024 at 05:26:21PM +0800, Max Hsu wrote:
> Sdtrig extension introduce two optional CSRs [hcontext/scontext],
> that will be storing PID/Guest OS ID for the debug feature.
>
> The availability of these two CSRs will be determined by
> DTS and Smstateen extension [h/s]stateen0 CSR bit 57.
>
> If all CPUs hcontext/scontext checks are satisfied, it will enable the
> use_hcontext/use_scontext static branch.
>
> Signed-off-by: Max Hsu <[email protected]>
> ---
> arch/riscv/include/asm/switch_to.h | 6 ++
> arch/riscv/kernel/cpufeature.c | 161 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 167 insertions(+)
>
> diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
> index 7efdb0584d47..07432550ed54 100644
> --- a/arch/riscv/include/asm/switch_to.h
> +++ b/arch/riscv/include/asm/switch_to.h
> @@ -69,6 +69,12 @@ static __always_inline bool has_fpu(void) { return false; }
> #define __switch_to_fpu(__prev, __next) do { } while (0)
> #endif
>
> +DECLARE_STATIC_KEY_FALSE(use_scontext);
> +static __always_inline bool has_scontext(void)
> +{
> + return static_branch_likely(&use_scontext);
> +}
> +
> extern struct task_struct *__switch_to(struct task_struct *,
> struct task_struct *);
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 080c06b76f53..44ff84b920af 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -35,6 +35,19 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly;
> /* Per-cpu ISA extensions. */
> struct riscv_isainfo hart_isa[NR_CPUS];
>
> +atomic_t hcontext_disable;
> +atomic_t scontext_disable;
> +
> +DEFINE_STATIC_KEY_FALSE_RO(use_hcontext);
> +EXPORT_SYMBOL(use_hcontext);
> +
> +DEFINE_STATIC_KEY_FALSE_RO(use_scontext);
> +EXPORT_SYMBOL(use_scontext);
> +
> +/* Record the maximum number that the hcontext CSR allowed to hold */
> +atomic_long_t hcontext_id_share;
> +EXPORT_SYMBOL(hcontext_id_share);
> +
> /**
> * riscv_isa_extension_base() - Get base extension word
> *
> @@ -719,6 +732,154 @@ unsigned long riscv_get_elf_hwcap(void)
> return hwcap;
> }
>
> +static void __init sdtrig_percpu_csrs_check(void *data)
> +{
> + struct device_node *node;
> + struct device_node *debug_node;
> + struct device_node *trigger_module;
> +
> + unsigned int cpu = smp_processor_id();
> +
> + /*
> + * Expect every cpu node has the [h/s]context-present property
> + * otherwise, jump to sdtrig_csrs_disable_all to disable all access to
> + * [h/s]context CSRs

I think the wording of this comment is kinda strange. What you're trying
to say is that homogeneous support for sdtrig (and the contexts) is
required.

> + */
> + node = of_cpu_device_node_get(cpu);

If there's no ACPI support, shouldn't the first thing here by a fast
path out before you start assuming DT?

> + if (!node)
> + goto sdtrig_csrs_disable_all;
> +
> + debug_node = of_get_compatible_child(node, "riscv,debug-v1.0.0");
> + of_node_put(node);
> +
> + if (!debug_node)
> + goto sdtrig_csrs_disable_all;
> +
> + trigger_module = of_get_child_by_name(debug_node, "trigger-module");
> + of_node_put(debug_node);
> +
> + if (!trigger_module)
> + goto sdtrig_csrs_disable_all;
> +
> + if (!(IS_ENABLED(CONFIG_KVM) &&
> + of_property_read_bool(trigger_module, "hcontext-present")))
> + atomic_inc(&hcontext_disable);
> +
> + if (!of_property_read_bool(trigger_module, "scontext-present"))
> + atomic_inc(&scontext_disable);

I think we should define pseudo extensions for {h,s}context-present and
parse this out of riscv,isa-extensions. That'd also give you the per-cpu
checks for homogeneous support for "free".
My immediate thought is that sdtrig doesn't seem valuable in isolation,
if you're gonna need additional properties that communicate support for
additional modes.

> + of_node_put(trigger_module);
> +
> + /*
> + * Before access to hcontext/scontext CSRs, if the smstateen
> + * extension is present, the accessibility will be controlled
> + * by the hstateen0[H]/sstateen0 CSRs.
> + */
> + if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_SMSTATEEN)) {

Why can't you use the non-underscore prefixed version of this function
here?

> + u64 hstateen_bit, sstateen_bit;
> +
> + if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_h)) {
> +#if __riscv_xlen > 32
> + csr_set(CSR_HSTATEEN0, SMSTATEEN0_HSCONTEXT);

For zkr we require the CSR to be usable at the privilege level to which
the DT is passed:
- const: zkr
description:
The standard Zkr entropy source extension as ratified in version
1.0 of RISC-V Cryptography Extensions Volume I specification.
This string being present means that the CSR associated to this
extension is accessible at the privilege level to which that
device-tree has been provided.

I wonder if we should do something similar here and make this a
requirement for anything with bits in stateen registers. I'd love to
avoid having to read CSRs on all harts before being able to make
judgements about whether or not an extension is enabled. I dunno if that
is possible here though, given you want to also make some checks on the
exact nature of the support below.

Cheers,
Conor.

> + hstateen_bit = csr_read(CSR_HSTATEEN0);
> +#else
> + csr_set(CSR_HSTATEEN0H, SMSTATEEN0_HSCONTEXT >> 32);
> + hstateen_bit = csr_read(CSR_HSTATEEN0H) << 32;
> +#endif
> + if (!(hstateen_bit & SMSTATEEN0_HSCONTEXT))
> + goto sdtrig_csrs_disable_all;
> +
> + } else {
> + if (IS_ENABLED(CONFIG_KVM))
> + atomic_inc(&hcontext_disable);
> +
> + /*
> + * In RV32, the smstateen extension doesn't provide
> + * high 32 bits of sstateen0 CSR which represent
> + * accessibility for scontext CSR;
> + * The decision is left on whether the dts has the
> + * property to access the scontext CSR.
> + */
> +#if __riscv_xlen > 32
> + csr_set(CSR_SSTATEEN0, SMSTATEEN0_HSCONTEXT);
> + sstateen_bit = csr_read(CSR_SSTATEEN0);
> +
> + if (!(sstateen_bit & SMSTATEEN0_HSCONTEXT))
> + atomic_inc(&scontext_disable);
> +#endif
> + }
> + }
> +
> + /*
> + * The code can only access hcontext/scontext CSRs if:
> + * The cpu dts node have [h/s]context-present;
> + * If Smstateen extension is presented, then the accessibility bit
> + * toward hcontext/scontext CSRs is enabled; Or the Smstateen extension
> + * isn't available, thus the access won't be blocked by it.
> + *
> + * With writing 1 to the every bit of these CSRs, we retrieve the
> + * maximum bits that is available on the CSRs. and decide
> + * whether it's suit for its context recording operation.
> + */
> + if (IS_ENABLED(CONFIG_KVM) &&
> + !atomic_read(&hcontext_disable)) {
> + unsigned long hcontext_available_bits = 0;
> +
> + csr_write(CSR_HCONTEXT, -1UL);
> + hcontext_available_bits = csr_swap(CSR_HCONTEXT, hcontext_available_bits);
> +
> + /* hcontext CSR is required by at least 1 bit */
> + if (hcontext_available_bits)
> + atomic_long_and(hcontext_available_bits, &hcontext_id_share);
> + else
> + atomic_inc(&hcontext_disable);
> + }
> +
> + if (!atomic_read(&scontext_disable)) {
> + unsigned long scontext_available_bits = 0;
> +
> + csr_write(CSR_SCONTEXT, -1UL);
> + scontext_available_bits = csr_swap(CSR_SCONTEXT, scontext_available_bits);
> +
> + /* scontext CSR is required by at least the sizeof pid_t */
> + if (scontext_available_bits < ((1UL << (sizeof(pid_t) << 3)) - 1))
> + atomic_inc(&scontext_disable);
> + }
> +
> + return;
> +
> +sdtrig_csrs_disable_all:
> + if (IS_ENABLED(CONFIG_KVM))
> + atomic_inc(&hcontext_disable);
> +
> + atomic_inc(&scontext_disable);
> +}
> +
> +static int __init sdtrig_enable_csrs_fill(void)
> +{
> + if (__riscv_isa_extension_available(NULL, RISCV_ISA_EXT_SDTRIG)) {
> + atomic_long_set(&hcontext_id_share, -1UL);
> +
> + /* check every CPUs sdtrig extension optional CSRs */
> + sdtrig_percpu_csrs_check(NULL);
> + smp_call_function(sdtrig_percpu_csrs_check, NULL, 1);
> +
> + if (IS_ENABLED(CONFIG_KVM) &&
> + !atomic_read(&hcontext_disable)) {
> + pr_info("riscv-sdtrig: Writing 'GuestOS ID' to hcontext CSR is enabled\n");
> + static_branch_enable(&use_hcontext);
> + }
> +
> + if (!atomic_read(&scontext_disable)) {
> + pr_info("riscv-sdtrig: Writing 'PID' to scontext CSR is enabled\n");
> + static_branch_enable(&use_scontext);
> + }
> + }
> + return 0;
> +}
> +
> +arch_initcall(sdtrig_enable_csrs_fill);
> +
> void riscv_user_isa_enable(void)
> {
> if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
>
> --
> 2.43.2
>


Attachments:
(No filename) (8.74 kB)
signature.asc (235.00 B)
Download all attachments

2024-03-29 10:32:34

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH RFC 02/11] dt-bindings: riscv: Add Sdtrig optional CSRs existence on DT

On Fri, Mar 29, 2024 at 05:26:18PM +0800, Max Hsu wrote:
> The mcontext/hcontext/scontext CSRs are optional in the Sdtrig extension,
> to prevent RW operations to the missing CSRs, which will cause
> illegal instructions.
>
> As a solution, we have proposed the dt format for these CSRs.

As I mentioned in your other patch, I amn't sure what the actual value
is in being told about "sdtrig" itself if so many of the CSRs are
optional. I think we should define pseudo extensions that represent
usable subsets that are allowed by riscv,isa-extensions, such as
those you describe here: sdtrig + mcontext, sdtrig + scontext and
sdtrig + hcontext. Probably also for strig + mscontext. What
additional value does having a debug child node give us that makes
it worth having over something like the above?

Thanks,
Conor.

>
> Signed-off-by: Max Hsu <[email protected]>
> ---
> Documentation/devicetree/bindings/riscv/cpus.yaml | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> index d87dd50f1a4b..c713a48c5025 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> @@ -137,6 +137,24 @@ properties:
> DMIPS/MHz, relative to highest capacity-dmips-mhz
> in the system.
>
> + debug:
> + type: object
> + properties:
> + compatible:
> + const: riscv,debug-v1.0.0
> + trigger-module:
> + type: object
> + description: |
> + An indication set of optional CSR existence from
> + riscv-debug-spec Sdtrig extension
> + properties:
> + mcontext-present:
> + type: boolean
> + hcontext-present:
> + type: boolean
> + scontext-present:
> + type: boolean
> +
> anyOf:
> - required:
> - riscv,isa
>
> --
> 2.43.2
>


Attachments:
(No filename) (1.97 kB)
signature.asc (235.00 B)
Download all attachments

2024-04-05 16:16:28

by Andrew Jones

[permalink] [raw]
Subject: Re: [PATCH RFC 02/11] dt-bindings: riscv: Add Sdtrig optional CSRs existence on DT

On Fri, Mar 29, 2024 at 10:31:10AM +0000, Conor Dooley wrote:
> On Fri, Mar 29, 2024 at 05:26:18PM +0800, Max Hsu wrote:
> > The mcontext/hcontext/scontext CSRs are optional in the Sdtrig extension,
> > to prevent RW operations to the missing CSRs, which will cause
> > illegal instructions.
> >
> > As a solution, we have proposed the dt format for these CSRs.
>
> As I mentioned in your other patch, I amn't sure what the actual value
> is in being told about "sdtrig" itself if so many of the CSRs are
> optional. I think we should define pseudo extensions that represent
> usable subsets that are allowed by riscv,isa-extensions, such as
> those you describe here: sdtrig + mcontext, sdtrig + scontext and
> sdtrig + hcontext. Probably also for strig + mscontext. What
> additional value does having a debug child node give us that makes
> it worth having over something like the above?

Yeah, Sdtrig, which doesn't tell you what you get, isn't nice at all.
I wonder if we can start with requiring Sdtrig to be accompanied by
Ssstrict in order to enable the context CSRs, i.e.

Sdtrig - support without optional CSRs
Sdtrig+Ssstrict - probe for optional CSRs, support what's found

If there are platforms with Sdtrig and optional CSRs, but not Ssstrict,
then maybe the optional CSRs can be detected in some vendor-specific way,
where the decision as to whether or not that vendor-specific way is
acceptable is handled case-by-case.

Thanks,
drew

>
> Thanks,
> Conor.
>
> >
> > Signed-off-by: Max Hsu <[email protected]>
> > ---
> > Documentation/devicetree/bindings/riscv/cpus.yaml | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > index d87dd50f1a4b..c713a48c5025 100644
> > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > @@ -137,6 +137,24 @@ properties:
> > DMIPS/MHz, relative to highest capacity-dmips-mhz
> > in the system.
> >
> > + debug:
> > + type: object
> > + properties:
> > + compatible:
> > + const: riscv,debug-v1.0.0
> > + trigger-module:
> > + type: object
> > + description: |
> > + An indication set of optional CSR existence from
> > + riscv-debug-spec Sdtrig extension
> > + properties:
> > + mcontext-present:
> > + type: boolean
> > + hcontext-present:
> > + type: boolean
> > + scontext-present:
> > + type: boolean
> > +
> > anyOf:
> > - required:
> > - riscv,isa
> >
> > --
> > 2.43.2
> >



> --
> kvm-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/kvm-riscv


2024-04-09 16:34:27

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH RFC 02/11] dt-bindings: riscv: Add Sdtrig optional CSRs existence on DT

On Fri, Apr 05, 2024 at 05:59:41PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2024 at 10:31:10AM +0000, Conor Dooley wrote:
> > On Fri, Mar 29, 2024 at 05:26:18PM +0800, Max Hsu wrote:
> > > The mcontext/hcontext/scontext CSRs are optional in the Sdtrig extension,
> > > to prevent RW operations to the missing CSRs, which will cause
> > > illegal instructions.
> > >
> > > As a solution, we have proposed the dt format for these CSRs.
> >
> > As I mentioned in your other patch, I amn't sure what the actual value
> > is in being told about "sdtrig" itself if so many of the CSRs are
> > optional. I think we should define pseudo extensions that represent
> > usable subsets that are allowed by riscv,isa-extensions, such as
> > those you describe here: sdtrig + mcontext, sdtrig + scontext and
> > sdtrig + hcontext. Probably also for strig + mscontext. What
> > additional value does having a debug child node give us that makes
> > it worth having over something like the above?
>
> Yeah, Sdtrig, which doesn't tell you what you get, isn't nice at all.
> I wonder if we can start with requiring Sdtrig to be accompanied by
> Ssstrict in order to enable the context CSRs, i.e.
>
> Sdtrig - support without optional CSRs
> Sdtrig+Ssstrict - probe for optional CSRs, support what's found
>
> If there are platforms with Sdtrig and optional CSRs, but not Ssstrict,
> then maybe the optional CSRs can be detected in some vendor-specific way,
> where the decision as to whether or not that vendor-specific way is
> acceptable is handled case-by-case.

I think it's pretty reasonable to make sstrict a requirement for the
kernel's use of sdtrig. If we have some non-sstrict systems that do
implement these particular CSRs, then I guess we can add some psuedo
instructions then (and nothing would stop the sstrict systems also
specifying directly). If they're using some non-standard CSRs then
case-by-case I guess.

I'm just specifically not keen on adding extra dt properties that do
things we can already do with the ones we have!


Attachments:
(No filename) (2.04 kB)
signature.asc (235.00 B)
Download all attachments