* [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
@ 2025-03-07 12:44 Rajnesh Kanwal
2025-03-07 18:00 ` Atish Kumar Patra
2025-04-01 0:46 ` Samuel Holland
0 siblings, 2 replies; 8+ messages in thread
From: Rajnesh Kanwal @ 2025-03-07 12:44 UTC (permalink / raw)
To: opensbi
The Control Transfer Records (CTR) extension provides a method to
record a limited branch history in register-accessible internal chip
storage.
This extension is similar to Arch LBR in x86 and BRBE in ARM.
The Extension has been stable and the latest release can be found here
https://github.com/riscv/riscv-control-transfer-records/release
Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
---
Changes in V3:
Removed FWFT bit. It was discussed in tech-prs group to
use trap and enable approach rather than FWFT bit. Trap
and enable approach leads to letter number of traps than
enabling and disabling FWFT bit on each CTR usage.
https://lists.riscv.org/g/tech-prs/message/1339
Changes in V2:
Added FWFT bit for CTR.
---
include/sbi/riscv_encoding.h | 13 +++++++++++++
include/sbi/sbi_hart.h | 4 ++++
lib/sbi/sbi_hart.c | 7 +++++++
3 files changed, 24 insertions(+)
diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
index 03c68a57..f3e2afd5 100644
--- a/include/sbi/riscv_encoding.h
+++ b/include/sbi/riscv_encoding.h
@@ -375,6 +375,17 @@
#define CSR_SSTATEEN2 0x10E
#define CSR_SSTATEEN3 0x10F
+/* Machine-Level Control transfer records CSRs */
+#define CSR_MCTRCTL 0x34e
+
+/* Supervisor-Level Control transfer records CSRs */
+#define CSR_SCTRCTL 0x14e
+#define CSR_SCTRSTATUS 0x14f
+#define CSR_SCTRDEPTH 0x15f
+
+/* VS-Level Control transfer records CSRs */
+#define CSR_VSCTRCTL 0x24e
+
/* ===== Hypervisor-level CSRs ===== */
/* Hypervisor Trap Setup (H-extension) */
@@ -799,6 +810,8 @@
#define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
#define SMSTATEEN0_FCSR_SHIFT 1
#define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
+#define SMSTATEEN0_CTR_SHIFT 54
+#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
#define SMSTATEEN0_CONTEXT_SHIFT 57
#define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
#define SMSTATEEN0_IMSIC_SHIFT 58
diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
index 4c36c778..c3a7feb7 100644
--- a/include/sbi/sbi_hart.h
+++ b/include/sbi/sbi_hart.h
@@ -75,6 +75,10 @@ enum sbi_hart_extensions {
SBI_HART_EXT_ZICFISS,
/** Hart has Ssdbltrp extension */
SBI_HART_EXT_SSDBLTRP,
+ /** HART has CTR M-mode CSRs */
+ SBI_HART_EXT_SMCTR,
+ /** HART has CTR S-mode CSRs */
+ SBI_HART_EXT_SSCTR,
/** Maximum index of Hart extension */
SBI_HART_EXT_MAX,
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index 8e2979b5..c343805c 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
else
mstateen_val &= ~(SMSTATEEN0_SVSLCT);
+ if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
+ mstateen_val |= SMSTATEEN0_CTR;
+ else
+ mstateen_val &= ~SMSTATEEN0_CTR;
+
csr_write(CSR_MSTATEEN0, mstateen_val);
#if __riscv_xlen == 32
csr_write(CSR_MSTATEEN0H, mstateen_val >> 32);
@@ -688,6 +693,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
__SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
__SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
__SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
+ __SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
+ __SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
};
_Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
2025-03-07 12:44 [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen Rajnesh Kanwal
@ 2025-03-07 18:00 ` Atish Kumar Patra
2025-03-31 22:57 ` Atish Patra
2025-04-01 0:46 ` Samuel Holland
1 sibling, 1 reply; 8+ messages in thread
From: Atish Kumar Patra @ 2025-03-07 18:00 UTC (permalink / raw)
To: opensbi
On Fri, Mar 7, 2025 at 4:44?AM Rajnesh Kanwal <rkanwal@rivosinc.com> wrote:
>
> The Control Transfer Records (CTR) extension provides a method to
> record a limited branch history in register-accessible internal chip
> storage.
>
> This extension is similar to Arch LBR in x86 and BRBE in ARM.
> The Extension has been stable and the latest release can be found here
> https://github.com/riscv/riscv-control-transfer-records/release
>
> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
>
> ---
> Changes in V3:
> Removed FWFT bit. It was discussed in tech-prs group to
> use trap and enable approach rather than FWFT bit. Trap
> and enable approach leads to letter number of traps than
> enabling and disabling FWFT bit on each CTR usage.
> https://lists.riscv.org/g/tech-prs/message/1339
>
> Changes in V2:
> Added FWFT bit for CTR.
> ---
> include/sbi/riscv_encoding.h | 13 +++++++++++++
> include/sbi/sbi_hart.h | 4 ++++
> lib/sbi/sbi_hart.c | 7 +++++++
> 3 files changed, 24 insertions(+)
>
> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> index 03c68a57..f3e2afd5 100644
> --- a/include/sbi/riscv_encoding.h
> +++ b/include/sbi/riscv_encoding.h
> @@ -375,6 +375,17 @@
> #define CSR_SSTATEEN2 0x10E
> #define CSR_SSTATEEN3 0x10F
>
> +/* Machine-Level Control transfer records CSRs */
> +#define CSR_MCTRCTL 0x34e
> +
> +/* Supervisor-Level Control transfer records CSRs */
> +#define CSR_SCTRCTL 0x14e
> +#define CSR_SCTRSTATUS 0x14f
> +#define CSR_SCTRDEPTH 0x15f
> +
> +/* VS-Level Control transfer records CSRs */
> +#define CSR_VSCTRCTL 0x24e
> +
> /* ===== Hypervisor-level CSRs ===== */
>
> /* Hypervisor Trap Setup (H-extension) */
> @@ -799,6 +810,8 @@
> #define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
> #define SMSTATEEN0_FCSR_SHIFT 1
> #define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
> +#define SMSTATEEN0_CTR_SHIFT 54
> +#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
> #define SMSTATEEN0_CONTEXT_SHIFT 57
> #define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
> #define SMSTATEEN0_IMSIC_SHIFT 58
> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
> index 4c36c778..c3a7feb7 100644
> --- a/include/sbi/sbi_hart.h
> +++ b/include/sbi/sbi_hart.h
> @@ -75,6 +75,10 @@ enum sbi_hart_extensions {
> SBI_HART_EXT_ZICFISS,
> /** Hart has Ssdbltrp extension */
> SBI_HART_EXT_SSDBLTRP,
> + /** HART has CTR M-mode CSRs */
> + SBI_HART_EXT_SMCTR,
> + /** HART has CTR S-mode CSRs */
> + SBI_HART_EXT_SSCTR,
>
> /** Maximum index of Hart extension */
> SBI_HART_EXT_MAX,
> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
> index 8e2979b5..c343805c 100644
> --- a/lib/sbi/sbi_hart.c
> +++ b/lib/sbi/sbi_hart.c
> @@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
> else
> mstateen_val &= ~(SMSTATEEN0_SVSLCT);
>
> + if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
> + mstateen_val |= SMSTATEEN0_CTR;
> + else
> + mstateen_val &= ~SMSTATEEN0_CTR;
> +
> csr_write(CSR_MSTATEEN0, mstateen_val);
> #if __riscv_xlen == 32
> csr_write(CSR_MSTATEEN0H, mstateen_val >> 32);
> @@ -688,6 +693,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
> __SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
> __SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
> __SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
> + __SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
> + __SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
> };
>
LGTM.
Reviewed-by: Atish Patra <atishp@rivosinc.com>
> _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
2025-03-07 18:00 ` Atish Kumar Patra
@ 2025-03-31 22:57 ` Atish Patra
0 siblings, 0 replies; 8+ messages in thread
From: Atish Patra @ 2025-03-31 22:57 UTC (permalink / raw)
To: opensbi, Anup Patel
On 3/7/25 10:00 AM, Atish Kumar Patra wrote:
> On Fri, Mar 7, 2025 at 4:44?AM Rajnesh Kanwal <rkanwal@rivosinc.com> wrote:
>>
>> The Control Transfer Records (CTR) extension provides a method to
>> record a limited branch history in register-accessible internal chip
>> storage.
>>
>> This extension is similar to Arch LBR in x86 and BRBE in ARM.
>> The Extension has been stable and the latest release can be found here
>> https://github.com/riscv/riscv-control-transfer-records/release
>>
>> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
>>
>> ---
>> Changes in V3:
>> Removed FWFT bit. It was discussed in tech-prs group to
>> use trap and enable approach rather than FWFT bit. Trap
>> and enable approach leads to letter number of traps than
>> enabling and disabling FWFT bit on each CTR usage.
>> https://lists.riscv.org/g/tech-prs/message/1339
>>
>> Changes in V2:
>> Added FWFT bit for CTR.
>> ---
>> include/sbi/riscv_encoding.h | 13 +++++++++++++
>> include/sbi/sbi_hart.h | 4 ++++
>> lib/sbi/sbi_hart.c | 7 +++++++
>> 3 files changed, 24 insertions(+)
>>
>> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
>> index 03c68a57..f3e2afd5 100644
>> --- a/include/sbi/riscv_encoding.h
>> +++ b/include/sbi/riscv_encoding.h
>> @@ -375,6 +375,17 @@
>> #define CSR_SSTATEEN2 0x10E
>> #define CSR_SSTATEEN3 0x10F
>>
>> +/* Machine-Level Control transfer records CSRs */
>> +#define CSR_MCTRCTL 0x34e
>> +
>> +/* Supervisor-Level Control transfer records CSRs */
>> +#define CSR_SCTRCTL 0x14e
>> +#define CSR_SCTRSTATUS 0x14f
>> +#define CSR_SCTRDEPTH 0x15f
>> +
>> +/* VS-Level Control transfer records CSRs */
>> +#define CSR_VSCTRCTL 0x24e
>> +
>> /* ===== Hypervisor-level CSRs ===== */
>>
>> /* Hypervisor Trap Setup (H-extension) */
>> @@ -799,6 +810,8 @@
>> #define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
>> #define SMSTATEEN0_FCSR_SHIFT 1
>> #define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
>> +#define SMSTATEEN0_CTR_SHIFT 54
>> +#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
>> #define SMSTATEEN0_CONTEXT_SHIFT 57
>> #define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
>> #define SMSTATEEN0_IMSIC_SHIFT 58
>> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
>> index 4c36c778..c3a7feb7 100644
>> --- a/include/sbi/sbi_hart.h
>> +++ b/include/sbi/sbi_hart.h
>> @@ -75,6 +75,10 @@ enum sbi_hart_extensions {
>> SBI_HART_EXT_ZICFISS,
>> /** Hart has Ssdbltrp extension */
>> SBI_HART_EXT_SSDBLTRP,
>> + /** HART has CTR M-mode CSRs */
>> + SBI_HART_EXT_SMCTR,
>> + /** HART has CTR S-mode CSRs */
>> + SBI_HART_EXT_SSCTR,
>>
>> /** Maximum index of Hart extension */
>> SBI_HART_EXT_MAX,
>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>> index 8e2979b5..c343805c 100644
>> --- a/lib/sbi/sbi_hart.c
>> +++ b/lib/sbi/sbi_hart.c
>> @@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
>> else
>> mstateen_val &= ~(SMSTATEEN0_SVSLCT);
>>
>> + if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
>> + mstateen_val |= SMSTATEEN0_CTR;
>> + else
>> + mstateen_val &= ~SMSTATEEN0_CTR;
>> +
>> csr_write(CSR_MSTATEEN0, mstateen_val);
>> #if __riscv_xlen == 32
>> csr_write(CSR_MSTATEEN0H, mstateen_val >> 32);
>> @@ -688,6 +693,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
>> __SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
>> __SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
>> __SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
>> + __SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
>> + __SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
>> };
>>
>
> LGTM.
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
>
Hi Anup,
Can we pick this patch if no other concerns are there ?
>> _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
>> --
>> 2.43.0
>>
>
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
2025-03-07 12:44 [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen Rajnesh Kanwal
2025-03-07 18:00 ` Atish Kumar Patra
@ 2025-04-01 0:46 ` Samuel Holland
2025-04-10 19:08 ` Atish Patra
1 sibling, 1 reply; 8+ messages in thread
From: Samuel Holland @ 2025-04-01 0:46 UTC (permalink / raw)
To: Rajnesh Kanwal, opensbi; +Cc: alistair.francis, atishp, apatel, beeman
Hi Rajnesh,
On 2025-03-07 6:44 AM, Rajnesh Kanwal wrote:
> The Control Transfer Records (CTR) extension provides a method to
> record a limited branch history in register-accessible internal chip
> storage.
>
> This extension is similar to Arch LBR in x86 and BRBE in ARM.
> The Extension has been stable and the latest release can be found here
> https://github.com/riscv/riscv-control-transfer-records/release
>
> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
>
> ---
> Changes in V3:
> Removed FWFT bit. It was discussed in tech-prs group to
> use trap and enable approach rather than FWFT bit. Trap
> and enable approach leads to letter number of traps than
> enabling and disabling FWFT bit on each CTR usage.
> https://lists.riscv.org/g/tech-prs/message/1339
If you want to use the trap and enable approach, then you cannot enable the
mstateen0 bit by default, or you will never trap. This is also missing the code
for the trap handler.
>
> Changes in V2:
> Added FWFT bit for CTR.
> ---
> include/sbi/riscv_encoding.h | 13 +++++++++++++
> include/sbi/sbi_hart.h | 4 ++++
> lib/sbi/sbi_hart.c | 7 +++++++
> 3 files changed, 24 insertions(+)
>
> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> index 03c68a57..f3e2afd5 100644
> --- a/include/sbi/riscv_encoding.h
> +++ b/include/sbi/riscv_encoding.h
> @@ -375,6 +375,17 @@
> #define CSR_SSTATEEN2 0x10E
> #define CSR_SSTATEEN3 0x10F
>
> +/* Machine-Level Control transfer records CSRs */
> +#define CSR_MCTRCTL 0x34e
> +
> +/* Supervisor-Level Control transfer records CSRs */
> +#define CSR_SCTRCTL 0x14e
> +#define CSR_SCTRSTATUS 0x14f
> +#define CSR_SCTRDEPTH 0x15f
> +
> +/* VS-Level Control transfer records CSRs */
> +#define CSR_VSCTRCTL 0x24e
> +
> /* ===== Hypervisor-level CSRs ===== */
>
> /* Hypervisor Trap Setup (H-extension) */
> @@ -799,6 +810,8 @@
> #define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
> #define SMSTATEEN0_FCSR_SHIFT 1
> #define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
> +#define SMSTATEEN0_CTR_SHIFT 54
> +#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
> #define SMSTATEEN0_CONTEXT_SHIFT 57
> #define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
> #define SMSTATEEN0_IMSIC_SHIFT 58
> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
> index 4c36c778..c3a7feb7 100644
> --- a/include/sbi/sbi_hart.h
> +++ b/include/sbi/sbi_hart.h
> @@ -75,6 +75,10 @@ enum sbi_hart_extensions {
> SBI_HART_EXT_ZICFISS,
> /** Hart has Ssdbltrp extension */
> SBI_HART_EXT_SSDBLTRP,
> + /** HART has CTR M-mode CSRs */
> + SBI_HART_EXT_SMCTR,
> + /** HART has CTR S-mode CSRs */
> + SBI_HART_EXT_SSCTR,
>
> /** Maximum index of Hart extension */
> SBI_HART_EXT_MAX,
> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
> index 8e2979b5..c343805c 100644
> --- a/lib/sbi/sbi_hart.c
> +++ b/lib/sbi/sbi_hart.c
> @@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
> else
> mstateen_val &= ~(SMSTATEEN0_SVSLCT);
>
> + if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
> + mstateen_val |= SMSTATEEN0_CTR;
> + else
> + mstateen_val &= ~SMSTATEEN0_CTR;
The purpose of the Smstateen extension is to prevent side channels between
contexts, by blocking access to registers that are not context switched. That
means any code that sets a new mstateen0 bit needs to also include code to
perform the context switch, or you are creating the side channel anyway.
Regards,
Samuel
> +
> csr_write(CSR_MSTATEEN0, mstateen_val);
> #if __riscv_xlen == 32
> csr_write(CSR_MSTATEEN0H, mstateen_val >> 32);
> @@ -688,6 +693,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
> __SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
> __SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
> __SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
> + __SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
> + __SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
> };
>
> _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
2025-04-01 0:46 ` Samuel Holland
@ 2025-04-10 19:08 ` Atish Patra
2025-04-13 13:41 ` Anup Patel
0 siblings, 1 reply; 8+ messages in thread
From: Atish Patra @ 2025-04-10 19:08 UTC (permalink / raw)
To: Samuel Holland, Rajnesh Kanwal, opensbi; +Cc: alistair.francis, apatel, beeman
On 3/31/25 5:46 PM, Samuel Holland wrote:
> Hi Rajnesh,
>
> On 2025-03-07 6:44 AM, Rajnesh Kanwal wrote:
>> The Control Transfer Records (CTR) extension provides a method to
>> record a limited branch history in register-accessible internal chip
>> storage.
>>
>> This extension is similar to Arch LBR in x86 and BRBE in ARM.
>> The Extension has been stable and the latest release can be found here
>> https://github.com/riscv/riscv-control-transfer-records/release
>>
>> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
>>
>> ---
>> Changes in V3:
>> Removed FWFT bit. It was discussed in tech-prs group to
>> use trap and enable approach rather than FWFT bit. Trap
>> and enable approach leads to letter number of traps than
>> enabling and disabling FWFT bit on each CTR usage.
>> https://lists.riscv.org/g/tech-prs/message/1339
>
> If you want to use the trap and enable approach, then you cannot enable the
> mstateen0 bit by default, or you will never trap. This is also missing the code
> for the trap handler.
>
I think you mean you can't trap in M-mode. However, there is no use case
for M-mode profiling.
The PRS discussion was around how we handle CTR state for guests where
the earlier approach used FWFT instead of the trap/enable.
>>
>> Changes in V2:
>> Added FWFT bit for CTR.
>> ---
>> include/sbi/riscv_encoding.h | 13 +++++++++++++
>> include/sbi/sbi_hart.h | 4 ++++
>> lib/sbi/sbi_hart.c | 7 +++++++
>> 3 files changed, 24 insertions(+)
>>
>> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
>> index 03c68a57..f3e2afd5 100644
>> --- a/include/sbi/riscv_encoding.h
>> +++ b/include/sbi/riscv_encoding.h
>> @@ -375,6 +375,17 @@
>> #define CSR_SSTATEEN2 0x10E
>> #define CSR_SSTATEEN3 0x10F
>>
>> +/* Machine-Level Control transfer records CSRs */
>> +#define CSR_MCTRCTL 0x34e
>> +
>> +/* Supervisor-Level Control transfer records CSRs */
>> +#define CSR_SCTRCTL 0x14e
>> +#define CSR_SCTRSTATUS 0x14f
>> +#define CSR_SCTRDEPTH 0x15f
>> +
>> +/* VS-Level Control transfer records CSRs */
>> +#define CSR_VSCTRCTL 0x24e
>> +
>> /* ===== Hypervisor-level CSRs ===== */
>>
>> /* Hypervisor Trap Setup (H-extension) */
>> @@ -799,6 +810,8 @@
>> #define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
>> #define SMSTATEEN0_FCSR_SHIFT 1
>> #define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
>> +#define SMSTATEEN0_CTR_SHIFT 54
>> +#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
>> #define SMSTATEEN0_CONTEXT_SHIFT 57
>> #define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
>> #define SMSTATEEN0_IMSIC_SHIFT 58
>> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
>> index 4c36c778..c3a7feb7 100644
>> --- a/include/sbi/sbi_hart.h
>> +++ b/include/sbi/sbi_hart.h
>> @@ -75,6 +75,10 @@ enum sbi_hart_extensions {
>> SBI_HART_EXT_ZICFISS,
>> /** Hart has Ssdbltrp extension */
>> SBI_HART_EXT_SSDBLTRP,
>> + /** HART has CTR M-mode CSRs */
>> + SBI_HART_EXT_SMCTR,
>> + /** HART has CTR S-mode CSRs */
>> + SBI_HART_EXT_SSCTR,
>>
>> /** Maximum index of Hart extension */
>> SBI_HART_EXT_MAX,
>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>> index 8e2979b5..c343805c 100644
>> --- a/lib/sbi/sbi_hart.c
>> +++ b/lib/sbi/sbi_hart.c
>> @@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
>> else
>> mstateen_val &= ~(SMSTATEEN0_SVSLCT);
>>
>> + if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
>> + mstateen_val |= SMSTATEEN0_CTR;
>> + else
>> + mstateen_val &= ~SMSTATEEN0_CTR;
>
> The purpose of the Smstateen extension is to prevent side channels between
> contexts, by blocking access to registers that are not context switched. That
> means any code that sets a new mstateen0 bit needs to also include code to
> perform the context switch, or you are creating the side channel anyway.
>
yes. We need to save/restore the CTR state during domain context switch.
I am not aware of any use cases where we are actually running two
different worlds with opensbi domains that require this at this point.
Once we have full supervisor domain extension support, these must be
taken care of. Can we merge this with a TODO item about domain context
switch issue ?
> Regards,
> Samuel
>
>> +
>> csr_write(CSR_MSTATEEN0, mstateen_val);
>> #if __riscv_xlen == 32
>> csr_write(CSR_MSTATEEN0H, mstateen_val >> 32);
>> @@ -688,6 +693,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
>> __SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
>> __SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
>> __SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
>> + __SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
>> + __SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
>> };
>>
>> _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
>
>
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
2025-04-10 19:08 ` Atish Patra
@ 2025-04-13 13:41 ` Anup Patel
2025-04-13 16:30 ` Samuel Holland
2025-04-14 21:08 ` Atish Patra
0 siblings, 2 replies; 8+ messages in thread
From: Anup Patel @ 2025-04-13 13:41 UTC (permalink / raw)
To: Atish Patra; +Cc: apatel, beeman, Rajnesh Kanwal, alistair.francis, opensbi
On Fri, Apr 11, 2025 at 12:39 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> On 3/31/25 5:46 PM, Samuel Holland wrote:
> > Hi Rajnesh,
> >
> > On 2025-03-07 6:44 AM, Rajnesh Kanwal wrote:
> >> The Control Transfer Records (CTR) extension provides a method to
> >> record a limited branch history in register-accessible internal chip
> >> storage.
> >>
> >> This extension is similar to Arch LBR in x86 and BRBE in ARM.
> >> The Extension has been stable and the latest release can be found here
> >> https://github.com/riscv/riscv-control-transfer-records/release
> >>
> >> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
> >>
> >> ---
> >> Changes in V3:
> >> Removed FWFT bit. It was discussed in tech-prs group to
> >> use trap and enable approach rather than FWFT bit. Trap
> >> and enable approach leads to letter number of traps than
> >> enabling and disabling FWFT bit on each CTR usage.
> >> https://lists.riscv.org/g/tech-prs/message/1339
> >
> > If you want to use the trap and enable approach, then you cannot enable the
> > mstateen0 bit by default, or you will never trap. This is also missing the code
> > for the trap handler.
> >
>
> I think you mean you can't trap in M-mode. However, there is no use case
> for M-mode profiling.
>
> The PRS discussion was around how we handle CTR state for guests where
> the earlier approach used FWFT instead of the trap/enable.
>
> >>
> >> Changes in V2:
> >> Added FWFT bit for CTR.
> >> ---
> >> include/sbi/riscv_encoding.h | 13 +++++++++++++
> >> include/sbi/sbi_hart.h | 4 ++++
> >> lib/sbi/sbi_hart.c | 7 +++++++
> >> 3 files changed, 24 insertions(+)
> >>
> >> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> >> index 03c68a57..f3e2afd5 100644
> >> --- a/include/sbi/riscv_encoding.h
> >> +++ b/include/sbi/riscv_encoding.h
> >> @@ -375,6 +375,17 @@
> >> #define CSR_SSTATEEN2 0x10E
> >> #define CSR_SSTATEEN3 0x10F
> >>
> >> +/* Machine-Level Control transfer records CSRs */
> >> +#define CSR_MCTRCTL 0x34e
> >> +
> >> +/* Supervisor-Level Control transfer records CSRs */
> >> +#define CSR_SCTRCTL 0x14e
> >> +#define CSR_SCTRSTATUS 0x14f
> >> +#define CSR_SCTRDEPTH 0x15f
> >> +
> >> +/* VS-Level Control transfer records CSRs */
> >> +#define CSR_VSCTRCTL 0x24e
> >> +
> >> /* ===== Hypervisor-level CSRs ===== */
> >>
> >> /* Hypervisor Trap Setup (H-extension) */
> >> @@ -799,6 +810,8 @@
> >> #define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
> >> #define SMSTATEEN0_FCSR_SHIFT 1
> >> #define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
> >> +#define SMSTATEEN0_CTR_SHIFT 54
> >> +#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
> >> #define SMSTATEEN0_CONTEXT_SHIFT 57
> >> #define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
> >> #define SMSTATEEN0_IMSIC_SHIFT 58
> >> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
> >> index 4c36c778..c3a7feb7 100644
> >> --- a/include/sbi/sbi_hart.h
> >> +++ b/include/sbi/sbi_hart.h
> >> @@ -75,6 +75,10 @@ enum sbi_hart_extensions {
> >> SBI_HART_EXT_ZICFISS,
> >> /** Hart has Ssdbltrp extension */
> >> SBI_HART_EXT_SSDBLTRP,
> >> + /** HART has CTR M-mode CSRs */
> >> + SBI_HART_EXT_SMCTR,
> >> + /** HART has CTR S-mode CSRs */
> >> + SBI_HART_EXT_SSCTR,
> >>
> >> /** Maximum index of Hart extension */
> >> SBI_HART_EXT_MAX,
> >> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
> >> index 8e2979b5..c343805c 100644
> >> --- a/lib/sbi/sbi_hart.c
> >> +++ b/lib/sbi/sbi_hart.c
> >> @@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
> >> else
> >> mstateen_val &= ~(SMSTATEEN0_SVSLCT);
> >>
> >> + if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
> >> + mstateen_val |= SMSTATEEN0_CTR;
> >> + else
> >> + mstateen_val &= ~SMSTATEEN0_CTR;
> >
> > The purpose of the Smstateen extension is to prevent side channels between
> > contexts, by blocking access to registers that are not context switched. That
> > means any code that sets a new mstateen0 bit needs to also include code to
> > perform the context switch, or you are creating the side channel anyway.
> >
>
> yes. We need to save/restore the CTR state during domain context switch.
> I am not aware of any use cases where we are actually running two
> different worlds with opensbi domains that require this at this point.
>
> Once we have full supervisor domain extension support, these must be
> taken care of. Can we merge this with a TODO item about domain context
> switch issue ?
Ideally, we should not leave the CTR state enabled by default even
in absence of domain context switch. Currently, we already have
AIA and a few other bits which are enabled by default in mstateen0
at boot-time but those should also be enabled lazily upon first access
fault to align with the purpose of Smstateen extension.
For now, let's go ahead with this patch but separately we need to
add infrastructure in OpenSBI to lazily enable bits in Smstateen
upon first-access-trap.
Reviewed-by: Anup Patel <anup@brainfault.org>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
>
> > Regards,
> > Samuel
> >
> >> +
> >> csr_write(CSR_MSTATEEN0, mstateen_val);
> >> #if __riscv_xlen == 32
> >> csr_write(CSR_MSTATEEN0H, mstateen_val >> 32);
> >> @@ -688,6 +693,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
> >> __SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
> >> __SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
> >> __SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
> >> + __SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
> >> + __SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
> >> };
> >>
> >> _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
> >
> >
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
2025-04-13 13:41 ` Anup Patel
@ 2025-04-13 16:30 ` Samuel Holland
2025-04-14 21:08 ` Atish Patra
1 sibling, 0 replies; 8+ messages in thread
From: Samuel Holland @ 2025-04-13 16:30 UTC (permalink / raw)
To: Atish Patra
Cc: Rajnesh Kanwal, opensbi, alistair.francis, apatel, beeman,
Anup Patel
Hi Atish,
On 2025-04-13 8:41 AM, Anup Patel wrote:
> On Fri, Apr 11, 2025 at 12:39 AM Atish Patra <atishp@rivosinc.com> wrote:
>>
>> On 3/31/25 5:46 PM, Samuel Holland wrote:
>>> Hi Rajnesh,
>>>
>>> On 2025-03-07 6:44 AM, Rajnesh Kanwal wrote:
>>>> The Control Transfer Records (CTR) extension provides a method to
>>>> record a limited branch history in register-accessible internal chip
>>>> storage.
>>>>
>>>> This extension is similar to Arch LBR in x86 and BRBE in ARM.
>>>> The Extension has been stable and the latest release can be found here
>>>> https://github.com/riscv/riscv-control-transfer-records/release
>>>>
>>>> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
>>>>
>>>> ---
>>>> Changes in V3:
>>>> Removed FWFT bit. It was discussed in tech-prs group to
>>>> use trap and enable approach rather than FWFT bit. Trap
>>>> and enable approach leads to letter number of traps than
>>>> enabling and disabling FWFT bit on each CTR usage.
>>>> https://lists.riscv.org/g/tech-prs/message/1339
>>>
>>> If you want to use the trap and enable approach, then you cannot enable the
>>> mstateen0 bit by default, or you will never trap. This is also missing the code
>>> for the trap handler.
>>>
>>
>> I think you mean you can't trap in M-mode. However, there is no use case
>> for M-mode profiling.
>>
>> The PRS discussion was around how we handle CTR state for guests where
>> the earlier approach used FWFT instead of the trap/enable.
For clarity: yes, I do mean trapping in M-mode. This is not for M-mode
profiling, but to handle CTR state for (H)S-mode domains, just like HS-mode
needs trapping to handle state for VS-mode guests. The same rationale for
trapping applies at both privilege modes.
>>>>
>>>> Changes in V2:
>>>> Added FWFT bit for CTR.
>>>> ---
>>>> include/sbi/riscv_encoding.h | 13 +++++++++++++
>>>> include/sbi/sbi_hart.h | 4 ++++
>>>> lib/sbi/sbi_hart.c | 7 +++++++
>>>> 3 files changed, 24 insertions(+)
>>>>
>>>> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
>>>> index 03c68a57..f3e2afd5 100644
>>>> --- a/include/sbi/riscv_encoding.h
>>>> +++ b/include/sbi/riscv_encoding.h
>>>> @@ -375,6 +375,17 @@
>>>> #define CSR_SSTATEEN2 0x10E
>>>> #define CSR_SSTATEEN3 0x10F
>>>>
>>>> +/* Machine-Level Control transfer records CSRs */
>>>> +#define CSR_MCTRCTL 0x34e
>>>> +
>>>> +/* Supervisor-Level Control transfer records CSRs */
>>>> +#define CSR_SCTRCTL 0x14e
>>>> +#define CSR_SCTRSTATUS 0x14f
>>>> +#define CSR_SCTRDEPTH 0x15f
>>>> +
>>>> +/* VS-Level Control transfer records CSRs */
>>>> +#define CSR_VSCTRCTL 0x24e
>>>> +
>>>> /* ===== Hypervisor-level CSRs ===== */
>>>>
>>>> /* Hypervisor Trap Setup (H-extension) */
>>>> @@ -799,6 +810,8 @@
>>>> #define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
>>>> #define SMSTATEEN0_FCSR_SHIFT 1
>>>> #define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
>>>> +#define SMSTATEEN0_CTR_SHIFT 54
>>>> +#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
>>>> #define SMSTATEEN0_CONTEXT_SHIFT 57
>>>> #define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
>>>> #define SMSTATEEN0_IMSIC_SHIFT 58
>>>> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
>>>> index 4c36c778..c3a7feb7 100644
>>>> --- a/include/sbi/sbi_hart.h
>>>> +++ b/include/sbi/sbi_hart.h
>>>> @@ -75,6 +75,10 @@ enum sbi_hart_extensions {
>>>> SBI_HART_EXT_ZICFISS,
>>>> /** Hart has Ssdbltrp extension */
>>>> SBI_HART_EXT_SSDBLTRP,
>>>> + /** HART has CTR M-mode CSRs */
>>>> + SBI_HART_EXT_SMCTR,
>>>> + /** HART has CTR S-mode CSRs */
>>>> + SBI_HART_EXT_SSCTR,
>>>>
>>>> /** Maximum index of Hart extension */
>>>> SBI_HART_EXT_MAX,
>>>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>>>> index 8e2979b5..c343805c 100644
>>>> --- a/lib/sbi/sbi_hart.c
>>>> +++ b/lib/sbi/sbi_hart.c
>>>> @@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
>>>> else
>>>> mstateen_val &= ~(SMSTATEEN0_SVSLCT);
>>>>
>>>> + if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
>>>> + mstateen_val |= SMSTATEEN0_CTR;
>>>> + else
>>>> + mstateen_val &= ~SMSTATEEN0_CTR;
>>>
>>> The purpose of the Smstateen extension is to prevent side channels between
>>> contexts, by blocking access to registers that are not context switched. That
>>> means any code that sets a new mstateen0 bit needs to also include code to
>>> perform the context switch, or you are creating the side channel anyway.
>>>
>>
>> yes. We need to save/restore the CTR state during domain context switch.
>> I am not aware of any use cases where we are actually running two
>> different worlds with opensbi domains that require this at this point.
Upstream OpenSBI does not have any built-in way to trigger a domain context
switch at the moment, but there are several downstream forks/users that do, for
example the OP-TEE and StandaloneMM PoCs. These may have production users, if
not already, then soon.
>> Once we have full supervisor domain extension support, these must be
>> taken care of. Can we merge this with a TODO item about domain context
>> switch issue ?
>
> Ideally, we should not leave the CTR state enabled by default even
> in absence of domain context switch. Currently, we already have
> AIA and a few other bits which are enabled by default in mstateen0
> at boot-time but those should also be enabled lazily upon first access
> fault to align with the purpose of Smstateen extension.
>
> For now, let's go ahead with this patch but separately we need to
> add infrastructure in OpenSBI to lazily enable bits in Smstateen
> upon first-access-trap.
Right, this is not the first extension missing context switch support. This
patch is adding more technical debt, but it is not really a new problem, so I
can accept pushing off the fix for now.
Regards,
Samuel
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
2025-04-13 13:41 ` Anup Patel
2025-04-13 16:30 ` Samuel Holland
@ 2025-04-14 21:08 ` Atish Patra
1 sibling, 0 replies; 8+ messages in thread
From: Atish Patra @ 2025-04-14 21:08 UTC (permalink / raw)
To: Anup Patel; +Cc: apatel, beeman, Rajnesh Kanwal, alistair.francis, opensbi
On 4/13/25 6:41 AM, Anup Patel wrote:
> On Fri, Apr 11, 2025 at 12:39 AM Atish Patra <atishp@rivosinc.com> wrote:
>>
>> On 3/31/25 5:46 PM, Samuel Holland wrote:
>>> Hi Rajnesh,
>>>
>>> On 2025-03-07 6:44 AM, Rajnesh Kanwal wrote:
>>>> The Control Transfer Records (CTR) extension provides a method to
>>>> record a limited branch history in register-accessible internal chip
>>>> storage.
>>>>
>>>> This extension is similar to Arch LBR in x86 and BRBE in ARM.
>>>> The Extension has been stable and the latest release can be found here
>>>> https://github.com/riscv/riscv-control-transfer-records/release
>>>>
>>>> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>
>>>>
>>>> ---
>>>> Changes in V3:
>>>> Removed FWFT bit. It was discussed in tech-prs group to
>>>> use trap and enable approach rather than FWFT bit. Trap
>>>> and enable approach leads to letter number of traps than
>>>> enabling and disabling FWFT bit on each CTR usage.
>>>> https://lists.riscv.org/g/tech-prs/message/1339
>>>
>>> If you want to use the trap and enable approach, then you cannot enable the
>>> mstateen0 bit by default, or you will never trap. This is also missing the code
>>> for the trap handler.
>>>
>>
>> I think you mean you can't trap in M-mode. However, there is no use case
>> for M-mode profiling.
>>
>> The PRS discussion was around how we handle CTR state for guests where
>> the earlier approach used FWFT instead of the trap/enable.
>>
>>>>
>>>> Changes in V2:
>>>> Added FWFT bit for CTR.
>>>> ---
>>>> include/sbi/riscv_encoding.h | 13 +++++++++++++
>>>> include/sbi/sbi_hart.h | 4 ++++
>>>> lib/sbi/sbi_hart.c | 7 +++++++
>>>> 3 files changed, 24 insertions(+)
>>>>
>>>> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
>>>> index 03c68a57..f3e2afd5 100644
>>>> --- a/include/sbi/riscv_encoding.h
>>>> +++ b/include/sbi/riscv_encoding.h
>>>> @@ -375,6 +375,17 @@
>>>> #define CSR_SSTATEEN2 0x10E
>>>> #define CSR_SSTATEEN3 0x10F
>>>>
>>>> +/* Machine-Level Control transfer records CSRs */
>>>> +#define CSR_MCTRCTL 0x34e
>>>> +
>>>> +/* Supervisor-Level Control transfer records CSRs */
>>>> +#define CSR_SCTRCTL 0x14e
>>>> +#define CSR_SCTRSTATUS 0x14f
>>>> +#define CSR_SCTRDEPTH 0x15f
>>>> +
>>>> +/* VS-Level Control transfer records CSRs */
>>>> +#define CSR_VSCTRCTL 0x24e
>>>> +
>>>> /* ===== Hypervisor-level CSRs ===== */
>>>>
>>>> /* Hypervisor Trap Setup (H-extension) */
>>>> @@ -799,6 +810,8 @@
>>>> #define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
>>>> #define SMSTATEEN0_FCSR_SHIFT 1
>>>> #define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
>>>> +#define SMSTATEEN0_CTR_SHIFT 54
>>>> +#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
>>>> #define SMSTATEEN0_CONTEXT_SHIFT 57
>>>> #define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
>>>> #define SMSTATEEN0_IMSIC_SHIFT 58
>>>> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
>>>> index 4c36c778..c3a7feb7 100644
>>>> --- a/include/sbi/sbi_hart.h
>>>> +++ b/include/sbi/sbi_hart.h
>>>> @@ -75,6 +75,10 @@ enum sbi_hart_extensions {
>>>> SBI_HART_EXT_ZICFISS,
>>>> /** Hart has Ssdbltrp extension */
>>>> SBI_HART_EXT_SSDBLTRP,
>>>> + /** HART has CTR M-mode CSRs */
>>>> + SBI_HART_EXT_SMCTR,
>>>> + /** HART has CTR S-mode CSRs */
>>>> + SBI_HART_EXT_SSCTR,
>>>>
>>>> /** Maximum index of Hart extension */
>>>> SBI_HART_EXT_MAX,
>>>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>>>> index 8e2979b5..c343805c 100644
>>>> --- a/lib/sbi/sbi_hart.c
>>>> +++ b/lib/sbi/sbi_hart.c
>>>> @@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
>>>> else
>>>> mstateen_val &= ~(SMSTATEEN0_SVSLCT);
>>>>
>>>> + if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
>>>> + mstateen_val |= SMSTATEEN0_CTR;
>>>> + else
>>>> + mstateen_val &= ~SMSTATEEN0_CTR;
>>>
>>> The purpose of the Smstateen extension is to prevent side channels between
>>> contexts, by blocking access to registers that are not context switched. That
>>> means any code that sets a new mstateen0 bit needs to also include code to
>>> perform the context switch, or you are creating the side channel anyway.
>>>
>>
>> yes. We need to save/restore the CTR state during domain context switch.
>> I am not aware of any use cases where we are actually running two
>> different worlds with opensbi domains that require this at this point.
>>
>> Once we have full supervisor domain extension support, these must be
>> taken care of. Can we merge this with a TODO item about domain context
>> switch issue ?
>
> Ideally, we should not leave the CTR state enabled by default even
> in absence of domain context switch. Currently, we already have
> AIA and a few other bits which are enabled by default in mstateen0
> at boot-time but those should also be enabled lazily upon first access
> fault to align with the purpose of Smstateen extension.
>
Yes. I will send a patch for lazily enabling smstateen bits in opensbi &
kvm.
> For now, let's go ahead with this patch but separately we need to
> add infrastructure in OpenSBI to lazily enable bits in Smstateen
> upon first-access-trap.
>
> Reviewed-by: Anup Patel <anup@brainfault.org>
>
> Applied this patch to the riscv/opensbi repo.
>
> Thanks,
> Anup
>
>
>
>>
>>> Regards,
>>> Samuel
>>>
>>>> +
>>>> csr_write(CSR_MSTATEEN0, mstateen_val);
>>>> #if __riscv_xlen == 32
>>>> csr_write(CSR_MSTATEEN0H, mstateen_val >> 32);
>>>> @@ -688,6 +693,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
>>>> __SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
>>>> __SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
>>>> __SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
>>>> + __SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
>>>> + __SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
>>>> };
>>>>
>>>> _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
>>>
>>>
>>
>>
>> --
>> opensbi mailing list
>> opensbi@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
>
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-04-14 21:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-07 12:44 [PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen Rajnesh Kanwal
2025-03-07 18:00 ` Atish Kumar Patra
2025-03-31 22:57 ` Atish Patra
2025-04-01 0:46 ` Samuel Holland
2025-04-10 19:08 ` Atish Patra
2025-04-13 13:41 ` Anup Patel
2025-04-13 16:30 ` Samuel Holland
2025-04-14 21:08 ` Atish Patra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.