On Tue, Feb 14, 2023 at 7:31 PM LIU Zhiwei <zhiwei_...@linux.alibaba.com> wrote:
>
>
> On 2023/2/9 14:23, Deepak Gupta wrote:
> > `zisslpcfi` extension adds two new CSRs. CSR_SSP and CSR_LPLR.
> > - CSR_SSP: This CSR holds shadow stack pointer for current privilege mode
> > CSR_SSP is accessible in all modes. Each mode must establish
> > it's own CSR_SSP.
> >
> > - CSR_LPLR: This CSR holds label value set at the callsite by compiler.
> > On call target label check instructions are emitted by
> > compiler which check label value against value present in
> > CSR_LPRL.
> >
> > Enabling of `zisslpcfi` is controlled via menvcfg (for S/HS/VS/U/VU) and
> > henvcfg (for VS/VU) at bit position 60.
> >
> > Each mode has enable/disable bits for forward cfi. Backward cfi doesn't
> > have separate enable/disable bits for S and M mode. User forward cfi and
> > user backward cfi enable/disable bits are in mstatus/sstatus CSR.
> > Supervisor forward cfi enable/disable bit are in menvcfg and henvcfg CSR.
> > Machine mode forward cfi enable/disable bit is in mseccfg CSR.
> >
> > If forward cfi enabled, all indirect branches must land on a landing pad
> > instruction (`lpcll`, introduced in later commits). CPU/hart tracks this
> > internally using a landing pad tracker called `elp` short for `expecting
> > landing pad`. An interrupt can occur between an indirect branch and
> > target. If such an event occurs `elp` is saved away in mstatus/sstatus
> > CSR
> >
> > Signed-off-by: Deepak Gupta <de...@rivosinc.com>
> > Signed-off-by: Kip Walker <k...@rivosinc.com>
> > ---
> > target/riscv/cpu.h | 5 +++++
> > target/riscv/cpu_bits.h | 25 +++++++++++++++++++++++++
> > target/riscv/pmp.h | 3 ++-
> > 3 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 9a923760b2..18db61a06a 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -181,6 +181,11 @@ struct CPUArchState {
> >
> > uint32_t features;
> >
> > + /* CFI Extension user mode registers and state */
> > + uint32_t lplr;
> > + target_ulong ssp;
> > + cfi_elp elp;
>
> I think you are coding according to the sections of the specification.
Yes, pretty much.
> However, when upstream code,
> don't add declaration or definition if you don't use it in this patch.
>
> This patch should be split into patches where use these definitions.
Noted.
>
> > +
> > #ifdef CONFIG_USER_ONLY
> > uint32_t elf_flags;
> > #endif
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 8b0d7e20ea..1663ba5775 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -39,6 +39,10 @@
> >
> > /* Control and Status Registers */
> >
> > +/* CFI CSRs */
> > +#define CSR_LPLR 0x006
> I didn't see the CSR encoding number from the link in cover-letter.
Yes, allocation is still in process. I'll ask ved (primary spec
author) to update it.
> > +#define CSR_SSP 0x020
> > +
> > /* User Trap Setup */
> > #define CSR_USTATUS 0x000
> > #define CSR_UIE 0x004
> > @@ -542,6 +546,10 @@
> > #define MSTATUS_TVM 0x00100000 /* since: priv-1.10 */
> > #define MSTATUS_TW 0x00200000 /* since: priv-1.10 */
> > #define MSTATUS_TSR 0x00400000 /* since: priv-1.10 */
> > +#define MSTATUS_UFCFIEN 0x00800000 /* Zisslpcfi-0.1 */
> > +#define MSTATUS_UBCFIEN 0x01000000 /* Zisslpcfi-0.1 */
> > +#define MSTATUS_SPELP 0x02000000 /* Zisslpcfi-0.1 */
> > +#define MSTATUS_MPELP 0x04000000 /* Zisslpcfi-0.1 */
> > #define MSTATUS_GVA 0x4000000000ULL
> > #define MSTATUS_MPV 0x8000000000ULL
> >
> > @@ -572,12 +580,21 @@ typedef enum {
> > #define SSTATUS_XS 0x00018000
> > #define SSTATUS_SUM 0x00040000 /* since: priv-1.10 */
> > #define SSTATUS_MXR 0x00080000
> > +#define SSTATUS_UFCFIEN MSTATUS_UFCFIEN /* Zisslpcfi-0.1 */
> > +#define SSTATUS_UBCFIEN MSTATUS_UBCFIEN /* Zisslpcfi-0.1 */
> > +#define SSTATUS_SPELP MSTATUS_SPELP /* Zisslpcfi-0.1 */
> >
> > #define SSTATUS64_UXL 0x0000000300000000ULL
> >
> > #define SSTATUS32_SD 0x80000000
> > #define SSTATUS64_SD 0x8000000000000000ULL
> >
> > +#define CFISTATUS_M_MASK (MSTATUS_UFCFIEN | MSTATUS_UBCFIEN | \
> > + MSTATUS_MPELP | MSTATUS_SPELP)
> > +
> > +#define CFISTATUS_S_MASK (SSTATUS_UFCFIEN | SSTATUS_UBCFIEN | \
> > + SSTATUS_SPELP)
> Why not the VSSTATUS?
It's the same mask for VSSTATUS, so pretty much using the same.
> > +
> > /* hstatus CSR bits */
> > #define HSTATUS_VSBE 0x00000020
> > #define HSTATUS_GVA 0x00000040
> > @@ -747,10 +764,14 @@ typedef enum RISCVException {
> > #define MENVCFG_CBIE (3UL << 4)
> > #define MENVCFG_CBCFE BIT(6)
> > #define MENVCFG_CBZE BIT(7)
> > +#define MENVCFG_SFCFIEN BIT(59)
> > +#define MENVCFG_CFI BIT(60)
>
> MENVCFG_CFIE according to the specification. The definitions in other
> places should also use X_CFIE.
Yes, it's a recent change in spec. I missed picking it up on impl.
>
> The same comment here with Weiwei, or you can use BIT_ULL.
>
Noted.
> Zhiwei
>
> > #define MENVCFG_PBMTE (1ULL << 62)
> > #define MENVCFG_STCE (1ULL << 63)
> >
> > /* For RV32 */
> > +#define MENVCFGH_SFCFIEN BIT(27)
> > +#define MENVCFGH_CFI BIT(28)
> > #define MENVCFGH_PBMTE BIT(30)
> > #define MENVCFGH_STCE BIT(31)
> >
> > @@ -763,10 +784,14 @@ typedef enum RISCVException {
> > #define HENVCFG_CBIE MENVCFG_CBIE
> > #define HENVCFG_CBCFE MENVCFG_CBCFE
> > #define HENVCFG_CBZE MENVCFG_CBZE
> > +#define HENVCFG_SFCFIEN MENVCFG_SFCFIEN
> > +#define HENVCFG_CFI MENVCFG_CFI
> > #define HENVCFG_PBMTE MENVCFG_PBMTE
> > #define HENVCFG_STCE MENVCFG_STCE
> >
> > /* For RV32 */
> > +#define HENVCFGH_SFCFIEN MENVCFGH_SFCFIEN
> > +#define HENVCFGH_CFI MENVCFGH_CFI
> > #define HENVCFGH_PBMTE MENVCFGH_PBMTE
> > #define HENVCFGH_STCE MENVCFGH_STCE
> >
> > diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> > index da32c61c85..f5bfc4955b 100644
> > --- a/target/riscv/pmp.h
> > +++ b/target/riscv/pmp.h
> > @@ -43,7 +43,8 @@ typedef enum {
> > MSECCFG_MMWP = 1 << 1,
> > MSECCFG_RLB = 1 << 2,
> > MSECCFG_USEED = 1 << 8,
> > - MSECCFG_SSEED = 1 << 9
> > + MSECCFG_SSEED = 1 << 9,
> > + MSECCFG_MFCFIEN = 1 << 10
> > } mseccfg_field_t;
> >
> > typedef struct {
