Re: [PATCH v1 RFC Zisslpcfi 2/9] target/riscv: zisslpcfi CSR, bit positions and other definitions

[Deepak Gupta]

On Tue, Feb 14, 2023 at 7:31 PM LIU Zhiwei <zhiwei_liu@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 <debug@rivosinc.com>
> > Signed-off-by: Kip Walker  <kip@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 {