zicfilp and zicfiss support in opensbi
v4 cover zicfilp and zicfiss support in opensbi
v4 for zicfilp and zicfiss support in opensbi
---
v4:
- fixed up commit message in patch 2
v3:
- added fwft implementation for shadow stack and landing pad interface
v2:
- added mpelp for 32bit (mstatush) and its handling in trad redirection
- removed default SSE enabling. it'll break shadow stack enabled kernel
- put sw check delegation at correct place
Deepak Gupta (4):
include: adding support for Zicfilp / Zicfiss encodings
lib: sbi: Zicfilp/Zicfiss detection and elp cfi state reflect back in
status
lib: sbi: sw check exception delegation
lib: sbi: fwft: implement landing pad and shadow stack fwft interface
include/sbi/riscv_encoding.h | 7 ++++ include/sbi/sbi_hart.h | 3 ++ lib/sbi/sbi_fwft.c | 74 ++++++++++++++++++++++++++++++++++++ lib/sbi/sbi_hart.c | 21 +++++++++- lib/sbi/sbi_trap.c | 20 ++++++++++ 5 files changed, 124 insertions(+), 1 deletion(-)
Signature · 1 line
2.44.0
PATCH v4 1/4 include: adding support for Zicfilp / Zicfiss encodings
Zicfilp / Zicfiss extension (see link) introduces b2 (LPE) in menvcfg CSR to enable landing pads and b3 (SSE) in menvcfg CSR to enable shadow stack and landing pad for privilege less than M. Additionally extension introduces new bits in *status for recording landing pad state and a new exception type software check exception with cause=0x12.
--- include/sbi/riscv_encoding.h | 7 +++++++ 1 file changed, 7 insertions(+)
include/sbi/riscv_encoding.h +7 -0
@@ -32,6 +32,7 @@#define MSTATUS_TVM _UL(0x00100000)#define MSTATUS_TW _UL(0x00200000)#define MSTATUS_TSR _UL(0x00400000) +#define MSTATUS_SPELP _UL(0x00800000)#define MSTATUS32_SD _UL(0x80000000)#if __riscv_xlen ==64 #define MSTATUS_UXL _ULL(0x0000000300000000)
@@ -41,12 +42,14 @@#define MSTATUS_GVA _ULL(0x0000004000000000)#define MSTATUS_GVA_SHIFT 38#define MSTATUS_MPV _ULL(0x0000008000000000) +#define MSTATUS_MPELP _ULL(0x0000020000000000)#else #define MSTATUSH_SBE _UL(0x00000010)#define MSTATUSH_MBE _UL(0x00000020)#define MSTATUSH_GVA _UL(0x00000040)#define MSTATUSH_GVA_SHIFT 6#define MSTATUSH_MPV _UL(0x00000080) +#define MSTATUSH_MPELP _UL(0x00000200)#endif #define MSTATUS32_SD _UL(0x80000000)#define MSTATUS64_SD _ULL(0x8000000000000000)
@@ -220,6 +223,8 @@#define ENVCFG_CBIE_ILL _UL(0x0)#define ENVCFG_CBIE_FLUSH _UL(0x1)#define ENVCFG_CBIE_INV _UL(0x3) +#define ENVCFG_SSE (_UL(1) << 3) +#define ENVCFG_LPE (_UL(1) << 2)#define ENVCFG_FIOM _UL(0x1)/* ===== User-level CSRs ===== */
@@ -228,6 +233,7 @@#define CSR_USTATUS 0x000#define CSR_UIE 0x004#define CSR_UTVEC 0x005 +#define CSR_SSP 0x011/* User Trap Handling (N-extension) */ #define CSR_USCRATCH 0x040
@@ -763,6 +769,7 @@#define CAUSE_FETCH_PAGE_FAULT 0xc#define CAUSE_LOAD_PAGE_FAULT 0xd#define CAUSE_STORE_PAGE_FAULT 0xf +#define CAUSE_SW_CHECK_EXCP 0x12#define CAUSE_FETCH_GUEST_PAGE_FAULT 0x14#define CAUSE_LOAD_GUEST_PAGE_FAULT 0x15#define CAUSE_VIRTUAL_INST_FAULT 0x16
Signature · 1 line
2.44.0
PATCH v4 1/4 include: adding support for Zicfilp / Zicfiss encodings
Quoted 67 lines · On Fri, Aug 23, 2024 at 11:47?AM Deepak Gupta <[email protected]> wrote:
Zicfilp / Zicfiss extension (see link) introduces b2 (LPE) in menvcfg CSR to enable landing pads and b3 (SSE) in menvcfg CSR to enable shadow stack and landing pad for privilege less than M. Additionally extension introduces new bits in *status for recording landing pad state and a new exception type
software check exceptionwith cause=0x12.Signed-off-by Deepak Gupta <[email protected]>--- include/sbi/riscv_encoding.h | 7 +++++++ 1 file changed, 7 insertions(+)include/sbi/riscv_encoding.h +7
-1@@ -32,6 +32,7 @@#define MSTATUS_TVM _UL(0x00100000)#define MSTATUS_TW _UL(0x00200000)#define MSTATUS_TSR _UL(0x00400000) +#define MSTATUS_SPELP _UL(0x00800000)#define MSTATUS32_SD _UL(0x80000000)#if __riscv_xlen ==64 #define MSTATUS_UXL _ULL(0x0000000300000000)@@ -41,12 +42,14 @@#define MSTATUS_GVA _ULL(0x0000004000000000)#define MSTATUS_GVA_SHIFT 38#define MSTATUS_MPV _ULL(0x0000008000000000) +#define MSTATUS_MPELP _ULL(0x0000020000000000)#else #define MSTATUSH_SBE _UL(0x00000010)#define MSTATUSH_MBE _UL(0x00000020)#define MSTATUSH_GVA _UL(0x00000040)#define MSTATUSH_GVA_SHIFT 6#define MSTATUSH_MPV _UL(0x00000080) +#define MSTATUSH_MPELP _UL(0x00000200)#endif #define MSTATUS32_SD _UL(0x80000000)#define MSTATUS64_SD _ULL(0x8000000000000000)@@ -220,6 +223,8 @@#define ENVCFG_CBIE_ILL _UL(0x0)#define ENVCFG_CBIE_FLUSH _UL(0x1)#define ENVCFG_CBIE_INV _UL(0x3) +#define ENVCFG_SSE (_UL(1) << 3) +#define ENVCFG_LPE (_UL(1) << 2)#define ENVCFG_FIOM _UL(0x1)/* ===== User-level CSRs ===== */ @@ -228,6 +233,7 @@#define CSR_USTATUS 0x000#define CSR_UIE 0x004#define CSR_UTVEC 0x005 +#define CSR_SSP 0x011/* User Trap Handling (N-extension) */ #define CSR_USCRATCH 0x040@@ -763,6 +769,7 @@#define CAUSE_FETCH_PAGE_FAULT 0xc#define CAUSE_LOAD_PAGE_FAULT 0xd#define CAUSE_STORE_PAGE_FAULT 0xf +#define CAUSE_SW_CHECK_EXCP 0x12#define CAUSE_FETCH_GUEST_PAGE_FAULT 0x14#define CAUSE_LOAD_GUEST_PAGE_FAULT 0x15#define CAUSE_VIRTUAL_INST_FAULT 0x16 --
Quoted 2 lines
2.44.0
PATCH v4 2/4 lib: sbi: Zicfilp/Zicfiss detection and elp cfi state reflect back in status
In reply to Deepak GuptaThis patch adds support for zicfilp / zicfiss detection.
Zicfilp records status of hart's ELP state in *status csr. Missing landing pad sets MPELP in mstatus. When SBI is redirecting back to S/VS/HS, SPELP is set in sstatus/vsstatus.
--- include/sbi/sbi_hart.h | 3 +++ lib/sbi/sbi_hart.c | 18 ++++++++++++++++++ lib/sbi/sbi_trap.c | 20 ++++++++++++++++++++ 3 files changed, 41 insertions(+)
include/sbi/sbi_hart.h +3 -0
@@ -67,6 +67,9 @@ enum sbi_hart_extensions {SBI_HART_EXT_SVADE ,/** Hart has Svadu extension */ SBI_HART_EXT_SVADU , +/** HART has zicfiss & zicfilp extension */ +SBI_HART_EXT_ZICFILP , +SBI_HART_EXT_ZICFISS ,/** Maximum index of Hart extension */ SBI_HART_EXT_MAX,
lib/sbi/sbi_hart.c +18 -0
@@ -680,6 +680,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {__SBI_HART_EXT_DATA (ssccfg,SBI_HART_EXT_SSCCFG ),__SBI_HART_EXT_DATA (svade,SBI_HART_EXT_SVADE ),__SBI_HART_EXT_DATA (svadu,SBI_HART_EXT_SVADU ), +__SBI_HART_EXT_DATA (zicfilp,SBI_HART_EXT_ZICFILP ), +__SBI_HART_EXT_DATA (zicfiss,SBI_HART_EXT_ZICFISS ), }; _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
@@ -776,6 +778,7 @@ static int hart_detect_features(struct sbi_scratch *scratch)unsigned long val, oldval;bool has_zicntr = false;int rc; +bool ssp_exist, elp_exist;/* If hart features already detected then do nothing */ if (hfeatures->detected)
@@ -933,6 +936,21 @@ __pmp_skip:/* Save trap based detection of Zicntr */ has_zicntr =sbi_hart_has_extension (scratch,SBI_HART_EXT_ZICNTR ); +if (hfeatures->priv_version >=SBI_HART_PRIV_VER_1_12 ) { + val =csr_read_allowed (CSR_SSP , (unsigned long )&trap); + ssp_exist = trap.cause ==0 ; +if (ssp_exist) +__sbi_hart_update_extension (hfeatures, +SBI_HART_EXT_ZICFISS , true); + +csr_set (CSR_MSTATUS ,MSTATUS_MPELP ); + val =csr_read_clear (CSR_MSTATUS ,MSTATUS_MPELP ); + elp_exist = val &MSTATUS_MPELP ; +if (elp_exist) +__sbi_hart_update_extension (hfeatures, +SBI_HART_EXT_ZICFILP , true); + } +/* Let platform populate extensions */ rc =sbi_platform_extensions_init (sbi_platform_thishart_ptr (), hfeatures);
lib/sbi/sbi_trap.c +20 -0
@@ -103,6 +103,7 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs, const struct sbi_trap_info *trap) {ulong hstatus, vsstatus, prev_mode; +bool elp = false;#if __riscv_xlen ==32 bool prev_virt = (regs->mstatusH &MSTATUSH_MPV ) ? true : false;#else
@@ -116,6 +117,17 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,if (prev_mode !=PRV_S && prev_mode !=PRV_U )return SBI_ENOTSUPP ; +/* If extension has support for CFI, clear MPELP because redirecting to VS or (H)S */ +if (sbi_hart_has_extension (sbi_scratch_thishart_ptr (),SBI_HART_EXT_ZICFILP )) { +#if __riscv_xlen ==32 + elp = regs->mstatusH &MSTATUSH_MPELP ; + regs->mstatusH &= ~MSTATUSH_MPELP ; +#else + elp = regs->mstatus &MSTATUS_MPELP ; + regs->mstatus &= ~MSTATUS_MPELP ; +#endif + } +/* If exceptions came from VS/VU-mode, redirect to VS-mode if * delegated in hedeleg */
@@ -169,6 +181,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,/* Get VS-mode SSTATUS CSR */ vsstatus =csr_read (CSR_VSSTATUS ); +/*if elp was set, set it back in vsstatus */ +if (elp) + vsstatus |=MSTATUS_SPELP ; +/* Set SPP for VS-mode */ vsstatus &= ~SSTATUS_SPP ;if (prev_mode ==PRV_S )
@@ -209,6 +225,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,/* Clear SIE for S-mode */ regs->mstatus &= ~MSTATUS_SIE ; + +/* if elp was set, set it back in mstatus */ +if (elp) + regs->mstatus |=MSTATUS_SPELP ; }return 0 ;
Signature · 1 line
2.44.0
PATCH v4 2/4 lib: sbi: Zicfilp/Zicfiss detection and elp cfi state reflect back in status
Quoted 50 lines · On Fri, Aug 23, 2024 at 11:47?AM Deepak Gupta <[email protected]> wrote:
This patch adds support for zicfilp / zicfiss detection.
Zicfilp records status of hart's ELP state in *status csr. Missing landing pad sets MPELP in mstatus. When SBI is redirecting back to S/VS/HS, SPELP is set in sstatus/vsstatus.
Signed-off-by Deepak Gupta <[email protected]>--- include/sbi/sbi_hart.h | 3 +++ lib/sbi/sbi_hart.c | 18 ++++++++++++++++++ lib/sbi/sbi_trap.c | 20 ++++++++++++++++++++ 3 files changed, 41 insertions(+)include/sbi/sbi_hart.h +3
-0@@ -67,6 +67,9 @@ enum sbi_hart_extensions {SBI_HART_EXT_SVADE ,/** Hart has Svadu extension */ SBI_HART_EXT_SVADU , +/** HART has zicfiss & zicfilp extension */ +SBI_HART_EXT_ZICFILP , +SBI_HART_EXT_ZICFISS ,/** Maximum index of Hart extension */ SBI_HART_EXT_MAX,lib/sbi/sbi_hart.c +3
-0@@ -680,6 +680,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {__SBI_HART_EXT_DATA (ssccfg,SBI_HART_EXT_SSCCFG ),__SBI_HART_EXT_DATA (svade,SBI_HART_EXT_SVADE ),__SBI_HART_EXT_DATA (svadu,SBI_HART_EXT_SVADU ), +__SBI_HART_EXT_DATA (zicfilp,SBI_HART_EXT_ZICFILP ), +__SBI_HART_EXT_DATA (zicfiss,SBI_HART_EXT_ZICFISS ), }; _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),@@ -776,6 +778,7 @@ static int hart_detect_features(struct sbi_scratch *scratch)unsigned long val, oldval;bool has_zicntr = false;int rc; +bool ssp_exist, elp_exist;/* If hart features already detected then do nothing */ if (hfeatures->detected)
lib/sbi/sbi_hart.c __pmp_skip: · line 945
Quoted 10 lines
lib/sbi/sbi_hart.c +7
-0@@ -933,6 +936,21 @@ __pmp_skip:/* Save trap based detection of Zicntr */ has_zicntr =sbi_hart_has_extension (scratch,SBI_HART_EXT_ZICNTR ); +if (hfeatures->priv_version >=SBI_HART_PRIV_VER_1_12 ) { + val =csr_read_allowed (CSR_SSP , (unsigned long )&trap); + ssp_exist = trap.cause ==0 ; +if (ssp_exist) +__sbi_hart_update_extension (hfeatures, +SBI_HART_EXT_ZICFISS , true); +
Why do we need to duplicate the code in __check_ext_csr ? You can set the SBI_HART_EXT_ZICFISS with __check_ext_csr and use sbi_hart_has_extension for the ZICFILP.
lib/sbi/sbi_hart.c __pmp_skip: · line 953
Quoted 8 lines
lib/sbi/sbi_hart.c +8
-0+csr_set (CSR_MSTATUS ,MSTATUS_MPELP ); + val =csr_read_clear (CSR_MSTATUS ,MSTATUS_MPELP ); + elp_exist = val &MSTATUS_MPELP ; +if (elp_exist) +__sbi_hart_update_extension (hfeatures, +SBI_HART_EXT_ZICFILP , true); + } +
However, OpenSBI already supports enabling extension based isa string. Is there a reason that
you can't use it for CFI extensions ?
Quoted 58 lines
lib/sbi/sbi_hart.c +0
-0/* Let platform populate extensions */ rc =sbi_platform_extensions_init (sbi_platform_thishart_ptr (), hfeatures);lib/sbi/sbi_trap.c +20
-1@@ -103,6 +103,7 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs, const struct sbi_trap_info *trap) {ulong hstatus, vsstatus, prev_mode; +bool elp = false;#if __riscv_xlen ==32 bool prev_virt = (regs->mstatusH &MSTATUSH_MPV ) ? true : false;#else @@ -116,6 +117,17 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,if (prev_mode !=PRV_S && prev_mode !=PRV_U )return SBI_ENOTSUPP ; +/* If extension has support for CFI, clear MPELP because redirecting to VS or (H)S */ +if (sbi_hart_has_extension (sbi_scratch_thishart_ptr (),SBI_HART_EXT_ZICFILP )) { +#if __riscv_xlen ==32 + elp = regs->mstatusH &MSTATUSH_MPELP ; + regs->mstatusH &= ~MSTATUSH_MPELP ; +#else + elp = regs->mstatus &MSTATUS_MPELP ; + regs->mstatus &= ~MSTATUS_MPELP ; +#endif + } +/* If exceptions came from VS/VU-mode, redirect to VS-mode if * delegated in hedeleg */ @@ -169,6 +181,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,/* Get VS-mode SSTATUS CSR */ vsstatus =csr_read (CSR_VSSTATUS ); +/*if elp was set, set it back in vsstatus */ +if (elp) + vsstatus |=MSTATUS_SPELP ; +/* Set SPP for VS-mode */ vsstatus &= ~SSTATUS_SPP ;if (prev_mode ==PRV_S )@@ -209,6 +225,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,/* Clear SIE for S-mode */ regs->mstatus &= ~MSTATUS_SIE ; + +/* if elp was set, set it back in mstatus */ +if (elp) + regs->mstatus |=MSTATUS_SPELP ; }return 0 ; --2.44.0
PATCH v4 2/4 lib: sbi: Zicfilp/Zicfiss detection and elp cfi state reflect back in status
Quoted 54 lines · Atish Kumar Patra
Quoted 53 lines · On Fri, Aug 23, 2024 at 11:47?AM Deepak Gupta <[email protected]> wrote:
This patch adds support for zicfilp / zicfiss detection.
Zicfilp records status of hart's ELP state in *status csr. Missing landing pad sets MPELP in mstatus. When SBI is redirecting back to S/VS/HS, SPELP is set in sstatus/vsstatus.
Signed-off-by Deepak Gupta <[email protected]>--- include/sbi/sbi_hart.h | 3 +++ lib/sbi/sbi_hart.c | 18 ++++++++++++++++++ lib/sbi/sbi_trap.c | 20 ++++++++++++++++++++ 3 files changed, 41 insertions(+)include/sbi/sbi_hart.h +3
-0@@ -67,6 +67,9 @@ enum sbi_hart_extensions {SBI_HART_EXT_SVADE ,/** Hart has Svadu extension */ SBI_HART_EXT_SVADU , +/** HART has zicfiss & zicfilp extension */ +SBI_HART_EXT_ZICFILP , +SBI_HART_EXT_ZICFISS ,/** Maximum index of Hart extension */ SBI_HART_EXT_MAX,lib/sbi/sbi_hart.c +3
-0@@ -680,6 +680,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {__SBI_HART_EXT_DATA (ssccfg,SBI_HART_EXT_SSCCFG ),__SBI_HART_EXT_DATA (svade,SBI_HART_EXT_SVADE ),__SBI_HART_EXT_DATA (svadu,SBI_HART_EXT_SVADU ), +__SBI_HART_EXT_DATA (zicfilp,SBI_HART_EXT_ZICFILP ), +__SBI_HART_EXT_DATA (zicfiss,SBI_HART_EXT_ZICFISS ), }; _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),@@ -776,6 +778,7 @@ static int hart_detect_features(struct sbi_scratch *scratch)unsigned long val, oldval;bool has_zicntr = false;int rc; +bool ssp_exist, elp_exist;/* If hart features already detected then do nothing */ if (hfeatures->detected)@@ -933,6 +936,21 @@ __pmp_skip:/* Save trap based detection of Zicntr */ has_zicntr =sbi_hart_has_extension (scratch,SBI_HART_EXT_ZICNTR );
Quoted 11 lines
lib/sbi/sbi_hart.c __pmp_skip: · line 945
Quoted 7 lines
lib/sbi/sbi_hart.c +7
-0+if (hfeatures->priv_version >=SBI_HART_PRIV_VER_1_12 ) { + val =csr_read_allowed (CSR_SSP , (unsigned long )&trap); + ssp_exist = trap.cause ==0 ; +if (ssp_exist) +__sbi_hart_update_extension (hfeatures, +SBI_HART_EXT_ZICFISS , true); +Why do we need to duplicate the code in
__check_ext_csr? You can set theSBI_HART_EXT_ZICFISSwith__check_ext_csrand usesbi_hart_has_extensionfor the ZICFILP.
Hmm didn't pay attention while rebasing that something like __check_ext_csr exists. Will revise patch and use it. Thanks.
I didn't get comment for ZICFILPsbi_hart_has_extension is the one which checks if extension is available or not. It doesn't do discovery and enumeration, Am I missing something.
Quoted 10 lines
lib/sbi/sbi_hart.c __pmp_skip: · line 953
Quoted 9 lines
lib/sbi/sbi_hart.c +8
-0+csr_set (CSR_MSTATUS ,MSTATUS_MPELP ); + val =csr_read_clear (CSR_MSTATUS ,MSTATUS_MPELP ); + elp_exist = val &MSTATUS_MPELP ; +if (elp_exist) +__sbi_hart_update_extension (hfeatures, +SBI_HART_EXT_ZICFILP , true); + } +
Quoted 3 lines
However, OpenSBI already supports enabling extension based isa string. Is there a reason that
you can't use it for CFI extensions ?
I am not aware of this. Or may be I am not following. I had been under the impression that firmware is supposed to detect/discover features. If OpenSBI receives ISA string froms somewhere and do light up of features based on that, I think CFI extensions can do that as well.
Question:
If OpenSBI supports enabling of extension based on parsing ISA string. Then why do we have detection mechanism(s) like __check_ext_csr.
Quoted 59 lines
Quoted 59 lines
lib/sbi/sbi_hart.c +0
-0/* Let platform populate extensions */ rc =sbi_platform_extensions_init (sbi_platform_thishart_ptr (), hfeatures);lib/sbi/sbi_trap.c +20
-1@@ -103,6 +103,7 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs, const struct sbi_trap_info *trap) {ulong hstatus, vsstatus, prev_mode; +bool elp = false;#if __riscv_xlen ==32 bool prev_virt = (regs->mstatusH &MSTATUSH_MPV ) ? true : false;#else @@ -116,6 +117,17 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,if (prev_mode !=PRV_S && prev_mode !=PRV_U )return SBI_ENOTSUPP ; +/* If extension has support for CFI, clear MPELP because redirecting to VS or (H)S */ +if (sbi_hart_has_extension (sbi_scratch_thishart_ptr (),SBI_HART_EXT_ZICFILP )) { +#if __riscv_xlen ==32 + elp = regs->mstatusH &MSTATUSH_MPELP ; + regs->mstatusH &= ~MSTATUSH_MPELP ; +#else + elp = regs->mstatus &MSTATUS_MPELP ; + regs->mstatus &= ~MSTATUS_MPELP ; +#endif + } +/* If exceptions came from VS/VU-mode, redirect to VS-mode if * delegated in hedeleg */ @@ -169,6 +181,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,/* Get VS-mode SSTATUS CSR */ vsstatus =csr_read (CSR_VSSTATUS ); +/*if elp was set, set it back in vsstatus */ +if (elp) + vsstatus |=MSTATUS_SPELP ; +/* Set SPP for VS-mode */ vsstatus &= ~SSTATUS_SPP ;if (prev_mode ==PRV_S )@@ -209,6 +225,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,/* Clear SIE for S-mode */ regs->mstatus &= ~MSTATUS_SIE ; + +/* if elp was set, set it back in mstatus */ +if (elp) + regs->mstatus |=MSTATUS_SPELP ; }return 0 ; --2.44.0
PATCH v4 2/4 lib: sbi: Zicfilp/Zicfiss detection and elp cfi state reflect back in status
Quoted 68 lines · On Mon, Sep 9, 2024 at 5:39?PM Deepak Gupta <[email protected]> wrote:
Quoted 54 lines · Atish Kumar Patra
Quoted 53 lines · On Fri, Aug 23, 2024 at 11:47?AM Deepak Gupta <[email protected]> wrote:
This patch adds support for zicfilp / zicfiss detection.
Zicfilp records status of hart's ELP state in *status csr. Missing landing pad sets MPELP in mstatus. When SBI is redirecting back to S/VS/HS, SPELP is set in sstatus/vsstatus.
Signed-off-by Deepak Gupta <[email protected]>--- include/sbi/sbi_hart.h | 3 +++ lib/sbi/sbi_hart.c | 18 ++++++++++++++++++ lib/sbi/sbi_trap.c | 20 ++++++++++++++++++++ 3 files changed, 41 insertions(+)include/sbi/sbi_hart.h +3
-0@@ -67,6 +67,9 @@ enum sbi_hart_extensions {SBI_HART_EXT_SVADE ,/** Hart has Svadu extension */ SBI_HART_EXT_SVADU , +/** HART has zicfiss & zicfilp extension */ +SBI_HART_EXT_ZICFILP , +SBI_HART_EXT_ZICFISS ,/** Maximum index of Hart extension */ SBI_HART_EXT_MAX,lib/sbi/sbi_hart.c +3
-0@@ -680,6 +680,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {__SBI_HART_EXT_DATA (ssccfg,SBI_HART_EXT_SSCCFG ),__SBI_HART_EXT_DATA (svade,SBI_HART_EXT_SVADE ),__SBI_HART_EXT_DATA (svadu,SBI_HART_EXT_SVADU ), +__SBI_HART_EXT_DATA (zicfilp,SBI_HART_EXT_ZICFILP ), +__SBI_HART_EXT_DATA (zicfiss,SBI_HART_EXT_ZICFISS ), }; _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),@@ -776,6 +778,7 @@ static int hart_detect_features(struct sbi_scratch *scratch)unsigned long val, oldval;bool has_zicntr = false;int rc; +bool ssp_exist, elp_exist;/* If hart features already detected then do nothing */ if (hfeatures->detected)@@ -933,6 +936,21 @@ __pmp_skip:/* Save trap based detection of Zicntr */ has_zicntr =sbi_hart_has_extension (scratch,SBI_HART_EXT_ZICNTR );Quoted 11 lines
lib/sbi/sbi_hart.c __pmp_skip: · line 945
Quoted 7 lines
lib/sbi/sbi_hart.c +7
-0+if (hfeatures->priv_version >=SBI_HART_PRIV_VER_1_12 ) { + val =csr_read_allowed (CSR_SSP , (unsigned long )&trap); + ssp_exist = trap.cause ==0 ; +if (ssp_exist) +__sbi_hart_update_extension (hfeatures, +SBI_HART_EXT_ZICFISS , true); +Why do we need to duplicate the code in
__check_ext_csr? You can set theSBI_HART_EXT_ZICFISSwith__check_ext_csrand usesbi_hart_has_extensionfor the ZICFILP.
Quoted 7 lines
Hmm didn't pay attention while rebasing that something like
__check_ext_csrexists. Will revise patch and use it. Thanks.I didn't get comment for ZICFILP
sbi_hart_has_extensionis the one which checks if extension is available or not. It doesn't do discovery and enumeration, Am I missing something.
Ignore that comment.
You are detecting the presence of SBI_HART_EXT_ZICFILP via mstatus not via read/trap method
that __check_ext_csr provides. So you can't use __check_ext_csr for that. I misread the code
earlier to assume that ZICFILP has some dependencies on ZICFISS. So I suggested that you can check the presence of ZICFISS with sbi_hart_has_extension.
Sorry for the confusion.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
lib/sbi/sbi_hart.c +8
-0+csr_set (CSR_MSTATUS ,MSTATUS_MPELP ); + val =csr_read_clear (CSR_MSTATUS ,MSTATUS_MPELP ); + elp_exist = val &MSTATUS_MPELP ; +if (elp_exist) +__sbi_hart_update_extension (hfeatures, +SBI_HART_EXT_ZICFILP , true); + } +
Quoted 9 lines
Quoted 3 lines
However, OpenSBI already supports enabling extension based isa string. Is there a reason that
you can't use it for CFI extensions ?I am not aware of this. Or may be I am not following. I had been under the impression that firmware is supposed to detect/discover features. If OpenSBI receives ISA string froms somewhere and do light up of features based on that, I think CFI extensions can do that as well.
Yeah. That is simpler. This feature got added recently while the trap n detect feature has been
there from the beginning.
https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/fdt/fdt_helper.c#L475C5-L475C29
That's why, any newer extension can just use DT based methods instead of adding its own csr trapping mechanism.__check_ext_csr continues to exist for legacy reasons. E.g. Zicntr/Zihpm can only be detected using traps.
We also need to detect how many hpmcounters are available which the DT doesn't describe.
Quoted 63 lines
Question:
If OpenSBI supports enabling of extension based on parsing ISA string. Then why do we have detection mechanism(s) like__check_ext_csr.Quoted 59 lines
Quoted 59 lines
lib/sbi/sbi_hart.c +0
-0/* Let platform populate extensions */ rc =sbi_platform_extensions_init (sbi_platform_thishart_ptr (), hfeatures);lib/sbi/sbi_trap.c +20
-1@@ -103,6 +103,7 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs, const struct sbi_trap_info *trap) {ulong hstatus, vsstatus, prev_mode; +bool elp = false;#if __riscv_xlen ==32 bool prev_virt = (regs->mstatusH &MSTATUSH_MPV ) ? true : false;#else @@ -116,6 +117,17 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,if (prev_mode !=PRV_S && prev_mode !=PRV_U )return SBI_ENOTSUPP ; +/* If extension has support for CFI, clear MPELP because redirecting to VS or (H)S */ +if (sbi_hart_has_extension (sbi_scratch_thishart_ptr (),SBI_HART_EXT_ZICFILP )) { +#if __riscv_xlen ==32 + elp = regs->mstatusH &MSTATUSH_MPELP ; + regs->mstatusH &= ~MSTATUSH_MPELP ; +#else + elp = regs->mstatus &MSTATUS_MPELP ; + regs->mstatus &= ~MSTATUS_MPELP ; +#endif + } +/* If exceptions came from VS/VU-mode, redirect to VS-mode if * delegated in hedeleg */ @@ -169,6 +181,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,/* Get VS-mode SSTATUS CSR */ vsstatus =csr_read (CSR_VSSTATUS ); +/*if elp was set, set it back in vsstatus */ +if (elp) + vsstatus |=MSTATUS_SPELP ; +/* Set SPP for VS-mode */ vsstatus &= ~SSTATUS_SPP ;if (prev_mode ==PRV_S )@@ -209,6 +225,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,/* Clear SIE for S-mode */ regs->mstatus &= ~MSTATUS_SIE ; + +/* if elp was set, set it back in mstatus */ +if (elp) + regs->mstatus |=MSTATUS_SPELP ; }return 0 ; --2.44.0
PATCH v4 3/4 lib: sbi: sw check exception delegation
In reply to Deepak Guptazicfiss and zicfilp introduces new exception (cause=18). Delegate this exception to S mode because cfi violations in U / S will be reported via this exception.
--- lib/sbi/sbi_hart.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/sbi/sbi_hart.c +2 -1
@@ -207,7 +207,8 @@ static int delegate_traps(struct sbi_scratch *scratch)if (sbi_platform_has_mfaults_delegation (plat)) exceptions |= (1U <<CAUSE_FETCH_PAGE_FAULT ) | (1U <<CAUSE_LOAD_PAGE_FAULT ) | - (1U <<CAUSE_STORE_PAGE_FAULT ); + (1U <<CAUSE_STORE_PAGE_FAULT )| + (1U <<CAUSE_SW_CHECK_EXCP );/* * If hypervisor extension available then we only handle hypervisor
Signature · 1 line
2.44.0
PATCH v4 3/4 lib: sbi: sw check exception delegation
Quoted 26 lines · On Fri, Aug 23, 2024 at 11:47?AM Deepak Gupta <[email protected]> wrote:
zicfiss and zicfilp introduces new exception (cause=18). Delegate this exception to S mode because cfi violations in U / S will be reported via this exception.
Signed-off-by Deepak Gupta <[email protected]>Reviewed-by Samuel Holland <[email protected]>--- lib/sbi/sbi_hart.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)lib/sbi/sbi_hart.c +2
-2@@ -207,7 +207,8 @@ static int delegate_traps(struct sbi_scratch *scratch)if (sbi_platform_has_mfaults_delegation (plat)) exceptions |= (1U <<CAUSE_FETCH_PAGE_FAULT ) | (1U <<CAUSE_LOAD_PAGE_FAULT ) | - (1U <<CAUSE_STORE_PAGE_FAULT ); + (1U <<CAUSE_STORE_PAGE_FAULT )| + (1U <<CAUSE_SW_CHECK_EXCP );/* * If hypervisor extension available then we only handle hypervisor --
Quoted 2 lines
2.44.0
PATCH v4 4/4 lib: sbi: fwft: implement landing pad and shadow stack fwft interface
In reply to Deepak GuptaSupervisor software can enable control flow integrity features for itself using fwft feature SBI_FWFT_LANDING_PAD and SBI_FWFT_SHADOW_STACK. This patch implements the mechanism to enable both these fwft.
--- lib/sbi/sbi_fwft.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)
lib/sbi/sbi_fwft.c +74 -0
@@ -145,6 +145,68 @@ static int fwft_get_adue(struct fwft_config *conf, unsigned long *value)return SBI_OK ; } +static int fwft_lpad_supported (struct fwft_config *conf) +{ +if (!sbi_hart_has_extension (sbi_scratch_thishart_ptr (), +SBI_HART_EXT_ZICFILP )) +return SBI_ENOTSUPP ; + +return SBI_OK ; +} + +static int fwft_enable_lpad (struct fwft_config *conf,unsigned long value) +{ +if (value ==1 ) +csr_set (CSR_MENVCFG ,ENVCFG_LPE ); +else if (value ==0 ) +csr_clear (CSR_MENVCFG ,ENVCFG_LPE ); +else +return SBI_EINVAL ; + +return SBI_OK ; +} + +static int fwft_get_lpad (struct fwft_config *conf,unsigned long *value) +{ +unsigned long cfg; + + cfg =csr_read (CSR_MENVCFG ) &ENVCFG_LPE ; + *value = cfg !=0 ; + +return SBI_OK ; +} + +static int fwft_sstack_supported (struct fwft_config *conf) +{ +if (!sbi_hart_has_extension (sbi_scratch_thishart_ptr (), +SBI_HART_EXT_ZICFISS )) +return SBI_ENOTSUPP ; + +return SBI_OK ; +} + +static int fwft_enable_sstack (struct fwft_config *conf,unsigned long value) +{ +if (value ==1 ) +csr_set (CSR_MENVCFG ,ENVCFG_SSE ); +else if (value ==0 ) +csr_clear (CSR_MENVCFG ,ENVCFG_SSE ); +else +return SBI_EINVAL ; + +return SBI_OK ; +} + +static int fwft_get_sstack (struct fwft_config *conf,unsigned long *value) +{ +unsigned long cfg; + + cfg =csr_read (CSR_MENVCFG ) &ENVCFG_SSE ; + *value = cfg !=0 ; + +return SBI_OK ; +} +static struct fwft_config *get_feature_config (enum sbi_fwft_feature_t feature) {int i;
@@ -236,6 +298,18 @@ static const struct fwft_feature features[] = .set = fwft_set_adue, .get = fwft_get_adue, }, + { + .id = SBI_FWFT_LANDING_PAD, + .supported = fwft_lpad_supported, + .set = fwft_enable_lpad, + .get = fwft_get_lpad, + }, + { + .id = SBI_FWFT_SHADOW_STACK, + .supported = fwft_sstack_supported, + .set = fwft_enable_sstack, + .get = fwft_get_sstack, + }, };int sbi_fwft_init (struct sbi_scratch *scratch,bool cold_boot)
Signature · 1 line
2.44.0
PATCH v4 4/4 lib: sbi: fwft: implement landing pad and shadow stack fwft interface
Quoted 103 lines · On Fri, Aug 23, 2024 at 11:47?AM Deepak Gupta <[email protected]> wrote:
Supervisor software can enable control flow integrity features for itself using fwft feature
SBI_FWFT_LANDING_PADandSBI_FWFT_SHADOW_STACK. This patch implements the mechanism to enable both these fwft.Signed-off-by Deepak Gupta <[email protected]>--- lib/sbi/sbi_fwft.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)lib/sbi/sbi_fwft.c +74
-1@@ -145,6 +145,68 @@ static int fwft_get_adue(struct fwft_config *conf, unsigned long *value)return SBI_OK ; } +static int fwft_lpad_supported (struct fwft_config *conf) +{ +if (!sbi_hart_has_extension (sbi_scratch_thishart_ptr (), +SBI_HART_EXT_ZICFILP )) +return SBI_ENOTSUPP ; + +return SBI_OK ; +} + +static int fwft_enable_lpad (struct fwft_config *conf,unsigned long value) +{ +if (value ==1 ) +csr_set (CSR_MENVCFG ,ENVCFG_LPE ); +else if (value ==0 ) +csr_clear (CSR_MENVCFG ,ENVCFG_LPE ); +else +return SBI_EINVAL ; + +return SBI_OK ; +} + +static int fwft_get_lpad (struct fwft_config *conf,unsigned long *value) +{ +unsigned long cfg; + + cfg =csr_read (CSR_MENVCFG ) &ENVCFG_LPE ; + *value = cfg !=0 ; + +return SBI_OK ; +} + +static int fwft_sstack_supported (struct fwft_config *conf) +{ +if (!sbi_hart_has_extension (sbi_scratch_thishart_ptr (), +SBI_HART_EXT_ZICFISS )) +return SBI_ENOTSUPP ; + +return SBI_OK ; +} + +static int fwft_enable_sstack (struct fwft_config *conf,unsigned long value) +{ +if (value ==1 ) +csr_set (CSR_MENVCFG ,ENVCFG_SSE ); +else if (value ==0 ) +csr_clear (CSR_MENVCFG ,ENVCFG_SSE ); +else +return SBI_EINVAL ; + +return SBI_OK ; +} + +static int fwft_get_sstack (struct fwft_config *conf,unsigned long *value) +{ +unsigned long cfg; + + cfg =csr_read (CSR_MENVCFG ) &ENVCFG_SSE ; + *value = cfg !=0 ; + +return SBI_OK ; +} +static struct fwft_config *get_feature_config (enum sbi_fwft_feature_t feature) {int i;@@ -236,6 +298,18 @@ static const struct fwft_feature features[] = .set = fwft_set_adue, .get = fwft_get_adue, }, + { + .id = SBI_FWFT_LANDING_PAD, + .supported = fwft_lpad_supported, + .set = fwft_enable_lpad, + .get = fwft_get_lpad, + }, + { + .id = SBI_FWFT_SHADOW_STACK, + .supported = fwft_sstack_supported, + .set = fwft_enable_sstack, + .get = fwft_get_sstack, + }, };int sbi_fwft_init (struct sbi_scratch *scratch,bool cold_boot) --
Quoted 2 lines
2.44.0
Lgtm.
PATCH v4 4/4 lib: sbi: fwft: implement landing pad and shadow stack fwft interface
In reply to Deepak GuptaQuoted 91 lines · Deepak Gupta
Supervisor software can enable control flow integrity features for itself using fwft feature
SBI_FWFT_LANDING_PADandSBI_FWFT_SHADOW_STACK. This patch implements the mechanism to enable both these fwft.Signed-off-by Deepak Gupta <[email protected]>--- lib/sbi/sbi_fwft.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)lib/sbi/sbi_fwft.c +67
-0@@ -145,6 +145,68 @@ static int fwft_get_adue(struct fwft_config *conf, unsigned long *value)return SBI_OK ; } +static int fwft_lpad_supported (struct fwft_config *conf) +{ +if (!sbi_hart_has_extension (sbi_scratch_thishart_ptr (), +SBI_HART_EXT_ZICFILP )) +return SBI_ENOTSUPP ; + +return SBI_OK ; +} + +static int fwft_enable_lpad (struct fwft_config *conf,unsigned long value) +{ +if (value ==1 ) +csr_set (CSR_MENVCFG ,ENVCFG_LPE ); +else if (value ==0 ) +csr_clear (CSR_MENVCFG ,ENVCFG_LPE ); +else +return SBI_EINVAL ; + +return SBI_OK ; +} + +static int fwft_get_lpad (struct fwft_config *conf,unsigned long *value) +{ +unsigned long cfg; + + cfg =csr_read (CSR_MENVCFG ) &ENVCFG_LPE ; + *value = cfg !=0 ; + +return SBI_OK ; +} + +static int fwft_sstack_supported (struct fwft_config *conf) +{ +if (!sbi_hart_has_extension (sbi_scratch_thishart_ptr (), +SBI_HART_EXT_ZICFISS )) +return SBI_ENOTSUPP ; + +return SBI_OK ; +} + +static int fwft_enable_sstack (struct fwft_config *conf,unsigned long value) +{ +if (value ==1 ) +csr_set (CSR_MENVCFG ,ENVCFG_SSE ); +else if (value ==0 ) +csr_clear (CSR_MENVCFG ,ENVCFG_SSE ); +else +return SBI_EINVAL ; + +return SBI_OK ; +} + +static int fwft_get_sstack (struct fwft_config *conf,unsigned long *value) +{ +unsigned long cfg; + + cfg =csr_read (CSR_MENVCFG ) &ENVCFG_SSE ; + *value = cfg !=0 ; + +return SBI_OK ; +} +static struct fwft_config *get_feature_config (enum sbi_fwft_feature_t feature) {int i;@@ -236,6 +298,18 @@ static const struct fwft_feature features[] = .set = fwft_set_adue, .get = fwft_get_adue, }, + { + .id = SBI_FWFT_LANDING_PAD, + .supported = fwft_lpad_supported, + .set = fwft_enable_lpad, + .get = fwft_get_lpad,
lib/sbi/sbi_fwft.c static const struct fwft_feature features[] = · line 315
Quoted 10 lines
lib/sbi/sbi_fwft.c +7
-0+ }, + { + .id = SBI_FWFT_SHADOW_STACK, + .supported = fwft_sstack_supported, + .set = fwft_enable_sstack, + .get = fwft_get_sstack, + }, };int sbi_fwft_init (struct sbi_scratch *scratch,bool cold_boot)
Hey Deepak,
LGTM, the only thing that could be done (but I guess that'll be for me when I submit the double trap feature) is factorizing the MENVCFG read/write function since we'll have 3 features doing almost the same thing.
But that's really optional for you.
PATCH v4 4/4 lib: sbi: fwft: implement landing pad and shadow stack fwft interface
Quoted 103 lines · Cl?ment L?ger
Quoted 100 lines · Deepak Gupta
Supervisor software can enable control flow integrity features for itself using fwft feature
SBI_FWFT_LANDING_PADandSBI_FWFT_SHADOW_STACK. This patch implements the mechanism to enable both these fwft.Signed-off-by Deepak Gupta <[email protected]>--- lib/sbi/sbi_fwft.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)lib/sbi/sbi_fwft.c +74
-0@@ -145,6 +145,68 @@ static int fwft_get_adue(struct fwft_config *conf, unsigned long *value)return SBI_OK ; } +static int fwft_lpad_supported (struct fwft_config *conf) +{ +if (!sbi_hart_has_extension (sbi_scratch_thishart_ptr (), +SBI_HART_EXT_ZICFILP )) +return SBI_ENOTSUPP ; + +return SBI_OK ; +} + +static int fwft_enable_lpad (struct fwft_config *conf,unsigned long value) +{ +if (value ==1 ) +csr_set (CSR_MENVCFG ,ENVCFG_LPE ); +else if (value ==0 ) +csr_clear (CSR_MENVCFG ,ENVCFG_LPE ); +else +return SBI_EINVAL ; + +return SBI_OK ; +} + +static int fwft_get_lpad (struct fwft_config *conf,unsigned long *value) +{ +unsigned long cfg; + + cfg =csr_read (CSR_MENVCFG ) &ENVCFG_LPE ; + *value = cfg !=0 ; + +return SBI_OK ; +} + +static int fwft_sstack_supported (struct fwft_config *conf) +{ +if (!sbi_hart_has_extension (sbi_scratch_thishart_ptr (), +SBI_HART_EXT_ZICFISS )) +return SBI_ENOTSUPP ; + +return SBI_OK ; +} + +static int fwft_enable_sstack (struct fwft_config *conf,unsigned long value) +{ +if (value ==1 ) +csr_set (CSR_MENVCFG ,ENVCFG_SSE ); +else if (value ==0 ) +csr_clear (CSR_MENVCFG ,ENVCFG_SSE ); +else +return SBI_EINVAL ; + +return SBI_OK ; +} + +static int fwft_get_sstack (struct fwft_config *conf,unsigned long *value) +{ +unsigned long cfg; + + cfg =csr_read (CSR_MENVCFG ) &ENVCFG_SSE ; + *value = cfg !=0 ; + +return SBI_OK ; +} +static struct fwft_config *get_feature_config (enum sbi_fwft_feature_t feature) {int i;@@ -236,6 +298,18 @@ static const struct fwft_feature features[] = .set = fwft_set_adue, .get = fwft_get_adue, }, + { + .id = SBI_FWFT_LANDING_PAD, + .supported = fwft_lpad_supported, + .set = fwft_enable_lpad, + .get = fwft_get_lpad, + }, + { + .id = SBI_FWFT_SHADOW_STACK, + .supported = fwft_sstack_supported, + .set = fwft_enable_sstack, + .get = fwft_get_sstack, + }, };
Quoted 9 lines
lib/sbi/sbi_fwft.c static const struct fwft_feature features[] = · line 315
Quoted 1 line
lib/sbi/sbi_fwft.c +0
-0int sbi_fwft_init (struct sbi_scratch *scratch,bool cold_boot)Hey Deepak,
LGTM, the only thing that could be done (but I guess that'll be for me when I submit the double trap feature) is factorizing the MENVCFG read/write function since we'll have 3 features doing almost the same thing.
Since you've thoughts on this on how to handle it, I'll leave it for you.
Quoted 3 lines
But that's really optional for you.
Reviewed-by Cl?ment L?ger <[email protected]>
Thanks.