Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-15 Thread Deepak Gupta
On Wed, Feb 15, 2023 at 6:44 PM Richard Henderson
 wrote:
>
> On 2/15/23 15:38, Deepak Gupta wrote:
> > Question:
> > I'll basically need two bits (one for forward cfi and one for backward cfi).
>
> Are they separately enabled?  It may also be possible to use a single bit and 
> then perform
> a runtime check.  I guess I should read the spec...

There is a master enable but there're controls to individually enable & disable.
Thus the CPU could be in a state where one is enabled and other is not.

>
> > But I need to throw away the TB if cfi enabling bits mismatch at the
> > time TB was generated and the current state of enabling bits.
> > Reason being, this needs to get translated again and zimops need to be
> > generated.
> >
> > What's the best way to throw away a single TB?
>
> You don't throw TBs away at all.
>
> The current cpu state is produced by cpu_get_tb_cpu_state.  This is included 
> into the hash
> table lookup and will only match a TB which has been generated with the same 
> state.  Which
> means that you can have multiple live TBs, those with CFI enabled and those 
> without, and
> the correct one will be selected at runtime.
>

Thanks a lot. I see `tb_lookup`  & `tb_htable_lookup`.
I'll revise and add some bits in TB_FLAGS for both of them.
And probably ask some questions on the list if anything is not clear to me.

>
> r~



Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-15 Thread Richard Henderson

On 2/15/23 15:38, Deepak Gupta wrote:

Question:
I'll basically need two bits (one for forward cfi and one for backward cfi).


Are they separately enabled?  It may also be possible to use a single bit and then perform 
a runtime check.  I guess I should read the spec...



But I need to throw away the TB if cfi enabling bits mismatch at the
time TB was generated and the current state of enabling bits.
Reason being, this needs to get translated again and zimops need to be
generated.

What's the best way to throw away a single TB?


You don't throw TBs away at all.

The current cpu state is produced by cpu_get_tb_cpu_state.  This is included into the hash 
table lookup and will only match a TB which has been generated with the same state.  Which 
means that you can have multiple live TBs, those with CFI enabled and those without, and 
the correct one will be selected at runtime.



r~



Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-15 Thread Deepak Gupta
On Wed, Feb 15, 2023 at 4:02 PM Richard Henderson
 wrote:
>
> On 2/15/23 13:33, Deepak Gupta wrote:
> > On Tue, Feb 14, 2023 at 9:47 PM LIU Zhiwei  
> > wrote:
> >> And MSTATUS_UBCFIEN field change don't need flush tlb.
> >>
> >
> > TCG code-gen would be different depending on whether ubcfi is enabled or 
> > not.
> > As an example a TB might have code generated when bcfi was enabled.
> > But if someone disables it,
> > translation needs to happen again and zimops implementation should be
> > generated this time.
>
> tlb_flush does not affect translation.  TB are tied to physical addresses and 
> are only
> flushed by writes or tb_flush().
>
> The correct fix is to allocate a bit from FIELD(TB_FLAGS, X, Y, 1), and adjust
> cpu_get_tb_cpu_state to indicate when CFI is active in the current cpu mode.
>
>

Hmm... So this looks like a major oversight on my side.
I had been under the impression that tlb flush does TB flushes too.
I was trying to save on TB_FLAGS.
I saw `tb_jmp_cache` was being cleared, didn't dig deep and assumed
that tlb flush clears up TB as well.
Now that you've pointed it out, it looks like that's a different optimization.

So looks like this definitely needs a fix.

Question:
I'll basically need two bits (one for forward cfi and one for backward cfi).
But I need to throw away the TB if cfi enabling bits mismatch at the
time TB was generated and the current state of enabling bits.
Reason being, this needs to get translated again and zimops need to be
generated.

What's the best way to throw away a single TB?


> r~



Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-15 Thread Richard Henderson

On 2/15/23 13:33, Deepak Gupta wrote:

On Tue, Feb 14, 2023 at 9:47 PM LIU Zhiwei  wrote:

And MSTATUS_UBCFIEN field change don't need flush tlb.



TCG code-gen would be different depending on whether ubcfi is enabled or not.
As an example a TB might have code generated when bcfi was enabled.
But if someone disables it,
translation needs to happen again and zimops implementation should be
generated this time.


tlb_flush does not affect translation.  TB are tied to physical addresses and are only 
flushed by writes or tb_flush().


The correct fix is to allocate a bit from FIELD(TB_FLAGS, X, Y, 1), and adjust 
cpu_get_tb_cpu_state to indicate when CFI is active in the current cpu mode.



r~



Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-15 Thread Deepak Gupta
On Tue, Feb 14, 2023 at 10:24 PM LIU Zhiwei
 wrote:
>
> I don't find the modification for read_mstatus.

Doesn't need any modification in read_mstatus.
It just returns whatever is in the mstatus.

>
> Zhiwei
>
> On 2023/2/15 13:47, LIU Zhiwei wrote:
> >
> > On 2023/2/9 14:23, Deepak Gupta wrote:
> >> CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch
> >> allows
> >> access to these CSRs. A predicate routine handles access to these CSR as
> >> per specification.
> >>
> >> This patch also implments new bit definitions in
> >> menvcfg/henvcfg/mstatus/
> >> sstatus CSRs to master enabled cfi and enable forward cfi in S and M
> >> mode.
> >> mstatus CSR holds forward and backward cfi enabling for U mode.
> >>
> >> There is no enabling bit for backward cfi in S and M mode. It is always
> >> enabled if extension is implemented by CPU.
> >>
> >> Signed-off-by: Deepak Gupta 
> >> Signed-off-by: Kip Walker  
> >> ---
> >>   target/riscv/csr.c | 137 -
> >>   target/riscv/pmp.c |   9 +++
> >>   2 files changed, 145 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index 0db2c233e5..24e208ebed 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env,
> >> int csrno)
> >>   return ctr(env, csrno);
> >>   }
> >>   +static RISCVException cfi(CPURISCVState *env, int csrno)
> >> +{
> >> +/* no cfi extension */
> >> +if (!env_archcpu(env)->cfg.ext_cfi) {
> >> +return RISCV_EXCP_ILLEGAL_INST;
> >> +}
> >> +/*
> >> + * CONFIG_USER_MODE always allow access for now. Better for user
> >> mode only
> >> + * functionality
> >> + */
> >> +#if !defined(CONFIG_USER_ONLY)
> >> +/* current priv not M */
> >> +if (env->priv != PRV_M) {
> >> +/* menvcfg says no CFI */
> >> +if (!get_field(env->menvcfg, MENVCFG_CFI)) {
> >> +return RISCV_EXCP_ILLEGAL_INST;
> >> +}
> >> +
> >> +/* V = 1 and henvcfg says no CFI. raise virtual instr fault */
> >> +if (riscv_cpu_virt_enabled(env) &&
> >> +!get_field(env->henvcfg, HENVCFG_CFI)) {
> >> +return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> >> +}
> >> +
> >> +/*
> >> + * LPLR and SSP are not accessible to U mode if disabled via
> >> status
> >> + * CSR
> >> + */
> >> +if (env->priv == PRV_U) {
> >> +if (csrno == CSR_LPLR &&
> >> +!get_field(env->mstatus, MSTATUS_UFCFIEN)) {
> >> +return RISCV_EXCP_ILLEGAL_INST;
> >> +}
> >> +if (csrno == CSR_SSP &&
> >> +!get_field(env->mstatus, MSTATUS_UBCFIEN)) {
> >> +return RISCV_EXCP_ILLEGAL_INST;
> >> +}
> >> +}
> >> +}
> >> +#endif
> >> +
> >> +return RISCV_EXCP_NONE;
> >> +}
> >> +
> >>   #if !defined(CONFIG_USER_ONLY)
> >>   static RISCVException mctr(CPURISCVState *env, int csrno)
> >>   {
> >> @@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env,
> >> int csrno)
> >>   #endif
> >>   }
> >>   +/* Zisslpcfi CSR_LPLR read/write */
> >> +static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val)
> >> +{
> >> +*val = env->lplr;
> >> +return RISCV_EXCP_NONE;
> >> +}
> >> +
> >> +static int write_lplr(CPURISCVState *env, int csrno, target_ulong val)
> >> +{
> >> +env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL);
> >> +return RISCV_EXCP_NONE;
> >> +}
> >> +
> >> +/* Zisslpcfi CSR_SSP read/write */
> >> +static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val)
> >> +{
> >> +*val = env->ssp;
> >> +return RISCV_EXCP_NONE;
> >> +}
> >> +
> >> +static int write_ssp(CPURISCVState *env, int csrno, target_ulong val)
> >> +{
> >> +env->ssp = val;
> >> +return RISCV_EXCP_NONE;
> >> +}
> >> +
> >>   /* User Floating-Point CSRs */
> >>   static RISCVException read_fflags(CPURISCVState *env, int csrno,
> >> target_ulong *val)
> >> @@ -1227,7 +1297,7 @@ static RISCVException
> >> write_mstatus(CPURISCVState *env, int csrno,
> >> /* flush tlb on mstatus fields that affect VM */
> >>   if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
> >> -MSTATUS_MPRV | MSTATUS_SUM)) {
> >> +MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN |
> >> MSTATUS_UBCFIEN)) {
> >
> > These two fields should be guarded by the check of ext_cfi.
> >
> > And MSTATUS_UBCFIEN field change don't need flush tlb.
> >
> > I didn't get why we should flush tlb for forward cfi. For background,
> > there are some enhancement for the PTE and PMP, we may need do some
> > memory adjustments. But forward cfi just adds some instructions. Why
> > we should flush tlb? Does the tlb can't be used any more?
> >
> >>   tlb_flush(env_cpu(env));
> >>   }
> >>   mask = MSTATUS_SIE | MSTATUS

Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-15 Thread Deepak Gupta
On Tue, Feb 14, 2023 at 9:47 PM LIU Zhiwei  wrote:
>
>
> On 2023/2/9 14:23, Deepak Gupta wrote:
> > CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch allows
> > access to these CSRs. A predicate routine handles access to these CSR as
> > per specification.
> >
> > This patch also implments new bit definitions in menvcfg/henvcfg/mstatus/
> > sstatus CSRs to master enabled cfi and enable forward cfi in S and M mode.
> > mstatus CSR holds forward and backward cfi enabling for U mode.
> >
> > There is no enabling bit for backward cfi in S and M mode. It is always
> > enabled if extension is implemented by CPU.
> >
> > Signed-off-by: Deepak Gupta 
> > Signed-off-by: Kip Walker  
> > ---
> >   target/riscv/csr.c | 137 -
> >   target/riscv/pmp.c |   9 +++
> >   2 files changed, 145 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 0db2c233e5..24e208ebed 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, int 
> > csrno)
> >   return ctr(env, csrno);
> >   }
> >
> > +static RISCVException cfi(CPURISCVState *env, int csrno)
> > +{
> > +/* no cfi extension */
> > +if (!env_archcpu(env)->cfg.ext_cfi) {
> > +return RISCV_EXCP_ILLEGAL_INST;
> > +}
> > +/*
> > + * CONFIG_USER_MODE always allow access for now. Better for user mode 
> > only
> > + * functionality
> > + */
> > +#if !defined(CONFIG_USER_ONLY)
> > +/* current priv not M */
> > +if (env->priv != PRV_M) {
> > +/* menvcfg says no CFI */
> > +if (!get_field(env->menvcfg, MENVCFG_CFI)) {
> > +return RISCV_EXCP_ILLEGAL_INST;
> > +}
> > +
> > +/* V = 1 and henvcfg says no CFI. raise virtual instr fault */
> > +if (riscv_cpu_virt_enabled(env) &&
> > +!get_field(env->henvcfg, HENVCFG_CFI)) {
> > +return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > +}
> > +
> > +/*
> > + * LPLR and SSP are not accessible to U mode if disabled via status
> > + * CSR
> > + */
> > +if (env->priv == PRV_U) {
> > +if (csrno == CSR_LPLR &&
> > +!get_field(env->mstatus, MSTATUS_UFCFIEN)) {
> > +return RISCV_EXCP_ILLEGAL_INST;
> > +}
> > +if (csrno == CSR_SSP &&
> > +!get_field(env->mstatus, MSTATUS_UBCFIEN)) {
> > +return RISCV_EXCP_ILLEGAL_INST;
> > +}
> > +}
> > +}
> > +#endif
> > +
> > +return RISCV_EXCP_NONE;
> > +}
> > +
> >   #if !defined(CONFIG_USER_ONLY)
> >   static RISCVException mctr(CPURISCVState *env, int csrno)
> >   {
> > @@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, int 
> > csrno)
> >   #endif
> >   }
> >
> > +/* Zisslpcfi CSR_LPLR read/write */
> > +static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +*val = env->lplr;
> > +return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int write_lplr(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > +env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL);
> > +return RISCV_EXCP_NONE;
> > +}
> > +
> > +/* Zisslpcfi CSR_SSP read/write */
> > +static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +*val = env->ssp;
> > +return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int write_ssp(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > +env->ssp = val;
> > +return RISCV_EXCP_NONE;
> > +}
> > +
> >   /* User Floating-Point CSRs */
> >   static RISCVException read_fflags(CPURISCVState *env, int csrno,
> > target_ulong *val)
> > @@ -1227,7 +1297,7 @@ static RISCVException write_mstatus(CPURISCVState 
> > *env, int csrno,
> >
> >   /* flush tlb on mstatus fields that affect VM */
> >   if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
> > -MSTATUS_MPRV | MSTATUS_SUM)) {
> > +MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | 
> > MSTATUS_UBCFIEN)) {
>
> These two fields should be guarded by the check of ext_cfi.

Yes this makes sense. Will fix it.

>
> And MSTATUS_UBCFIEN field change don't need flush tlb.
>

TCG code-gen would be different depending on whether ubcfi is enabled or not.
As an example a TB might have code generated when bcfi was enabled.
But if someone disables it,
translation needs to happen again and zimops implementation should be
generated this time.

> I didn't get why we should flush tlb for forward cfi. For background,
> there are some enhancement for the PTE and PMP, we may need do some
> memory adjustments. But forward cfi just adds some instructions. Why we
> should flush tlb? Does the tlb can't be used any more?

Pretty much same reasoning as back cfi. commit message in patch #7
goes in more detail on that.
TLB flush is mainly to invalidate TB.

Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-14 Thread LIU Zhiwei

I don't find the modification for read_mstatus.

Zhiwei

On 2023/2/15 13:47, LIU Zhiwei wrote:


On 2023/2/9 14:23, Deepak Gupta wrote:
CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch 
allows

access to these CSRs. A predicate routine handles access to these CSR as
per specification.

This patch also implments new bit definitions in 
menvcfg/henvcfg/mstatus/
sstatus CSRs to master enabled cfi and enable forward cfi in S and M 
mode.

mstatus CSR holds forward and backward cfi enabling for U mode.

There is no enabling bit for backward cfi in S and M mode. It is always
enabled if extension is implemented by CPU.

Signed-off-by: Deepak Gupta 
Signed-off-by: Kip Walker  
---
  target/riscv/csr.c | 137 -
  target/riscv/pmp.c |   9 +++
  2 files changed, 145 insertions(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0db2c233e5..24e208ebed 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, 
int csrno)

  return ctr(env, csrno);
  }
  +static RISCVException cfi(CPURISCVState *env, int csrno)
+{
+    /* no cfi extension */
+    if (!env_archcpu(env)->cfg.ext_cfi) {
+    return RISCV_EXCP_ILLEGAL_INST;
+    }
+    /*
+ * CONFIG_USER_MODE always allow access for now. Better for user 
mode only

+ * functionality
+ */
+#if !defined(CONFIG_USER_ONLY)
+    /* current priv not M */
+    if (env->priv != PRV_M) {
+    /* menvcfg says no CFI */
+    if (!get_field(env->menvcfg, MENVCFG_CFI)) {
+    return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    /* V = 1 and henvcfg says no CFI. raise virtual instr fault */
+    if (riscv_cpu_virt_enabled(env) &&
+    !get_field(env->henvcfg, HENVCFG_CFI)) {
+    return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+    }
+
+    /*
+ * LPLR and SSP are not accessible to U mode if disabled via 
status

+ * CSR
+ */
+    if (env->priv == PRV_U) {
+    if (csrno == CSR_LPLR &&
+    !get_field(env->mstatus, MSTATUS_UFCFIEN)) {
+    return RISCV_EXCP_ILLEGAL_INST;
+    }
+    if (csrno == CSR_SSP &&
+    !get_field(env->mstatus, MSTATUS_UBCFIEN)) {
+    return RISCV_EXCP_ILLEGAL_INST;
+    }
+    }
+    }
+#endif
+
+    return RISCV_EXCP_NONE;
+}
+
  #if !defined(CONFIG_USER_ONLY)
  static RISCVException mctr(CPURISCVState *env, int csrno)
  {
@@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, 
int csrno)

  #endif
  }
  +/* Zisslpcfi CSR_LPLR read/write */
+static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    *val = env->lplr;
+    return RISCV_EXCP_NONE;
+}
+
+static int write_lplr(CPURISCVState *env, int csrno, target_ulong val)
+{
+    env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL);
+    return RISCV_EXCP_NONE;
+}
+
+/* Zisslpcfi CSR_SSP read/write */
+static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val)
+{
+    *val = env->ssp;
+    return RISCV_EXCP_NONE;
+}
+
+static int write_ssp(CPURISCVState *env, int csrno, target_ulong val)
+{
+    env->ssp = val;
+    return RISCV_EXCP_NONE;
+}
+
  /* User Floating-Point CSRs */
  static RISCVException read_fflags(CPURISCVState *env, int csrno,
    target_ulong *val)
@@ -1227,7 +1297,7 @@ static RISCVException 
write_mstatus(CPURISCVState *env, int csrno,

    /* flush tlb on mstatus fields that affect VM */
  if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-    MSTATUS_MPRV | MSTATUS_SUM)) {
+    MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | 
MSTATUS_UBCFIEN)) {


These two fields should be guarded by the check of ext_cfi.

And MSTATUS_UBCFIEN field change don't need flush tlb.

I didn't get why we should flush tlb for forward cfi. For background, 
there are some enhancement for the PTE and PMP, we may need do some
memory adjustments. But forward cfi just adds some instructions. Why 
we should flush tlb? Does the tlb can't be used any more?



  tlb_flush(env_cpu(env));
  }
  mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
@@ -1250,6 +1320,11 @@ static RISCVException 
write_mstatus(CPURISCVState *env, int csrno,

  }
  }
  +    /* If cfi extension is available, then apply cfi status mask */
+    if (env_archcpu(env)->cfg.ext_cfi) {
+    mask |= CFISTATUS_M_MASK;
+    }
+
  mstatus = (mstatus & ~mask) | (val & mask);
    if (xl > MXL_RV32) {
@@ -1880,9 +1955,17 @@ static RISCVException 
write_menvcfg(CPURISCVState *env, int csrno,

    target_ulong val)
  {
  uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | 
MENVCFG_CBZE;

+    uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN;
    if (riscv_cpu_mxl(env) == MXL_RV64) {
  mask |= MENVCFG_PBMTE | MENVCFG_STCE;
+  

Re: [PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-14 Thread LIU Zhiwei



On 2023/2/9 14:23, Deepak Gupta wrote:

CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch allows
access to these CSRs. A predicate routine handles access to these CSR as
per specification.

This patch also implments new bit definitions in menvcfg/henvcfg/mstatus/
sstatus CSRs to master enabled cfi and enable forward cfi in S and M mode.
mstatus CSR holds forward and backward cfi enabling for U mode.

There is no enabling bit for backward cfi in S and M mode. It is always
enabled if extension is implemented by CPU.

Signed-off-by: Deepak Gupta 
Signed-off-by: Kip Walker  
---
  target/riscv/csr.c | 137 -
  target/riscv/pmp.c |   9 +++
  2 files changed, 145 insertions(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0db2c233e5..24e208ebed 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, int csrno)
  return ctr(env, csrno);
  }
  
+static RISCVException cfi(CPURISCVState *env, int csrno)

+{
+/* no cfi extension */
+if (!env_archcpu(env)->cfg.ext_cfi) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+/*
+ * CONFIG_USER_MODE always allow access for now. Better for user mode only
+ * functionality
+ */
+#if !defined(CONFIG_USER_ONLY)
+/* current priv not M */
+if (env->priv != PRV_M) {
+/* menvcfg says no CFI */
+if (!get_field(env->menvcfg, MENVCFG_CFI)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+
+/* V = 1 and henvcfg says no CFI. raise virtual instr fault */
+if (riscv_cpu_virt_enabled(env) &&
+!get_field(env->henvcfg, HENVCFG_CFI)) {
+return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+}
+
+/*
+ * LPLR and SSP are not accessible to U mode if disabled via status
+ * CSR
+ */
+if (env->priv == PRV_U) {
+if (csrno == CSR_LPLR &&
+!get_field(env->mstatus, MSTATUS_UFCFIEN)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+if (csrno == CSR_SSP &&
+!get_field(env->mstatus, MSTATUS_UBCFIEN)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+}
+}
+#endif
+
+return RISCV_EXCP_NONE;
+}
+
  #if !defined(CONFIG_USER_ONLY)
  static RISCVException mctr(CPURISCVState *env, int csrno)
  {
@@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, int csrno)
  #endif
  }
  
+/* Zisslpcfi CSR_LPLR read/write */

+static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val)
+{
+*val = env->lplr;
+return RISCV_EXCP_NONE;
+}
+
+static int write_lplr(CPURISCVState *env, int csrno, target_ulong val)
+{
+env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL);
+return RISCV_EXCP_NONE;
+}
+
+/* Zisslpcfi CSR_SSP read/write */
+static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val)
+{
+*val = env->ssp;
+return RISCV_EXCP_NONE;
+}
+
+static int write_ssp(CPURISCVState *env, int csrno, target_ulong val)
+{
+env->ssp = val;
+return RISCV_EXCP_NONE;
+}
+
  /* User Floating-Point CSRs */
  static RISCVException read_fflags(CPURISCVState *env, int csrno,
target_ulong *val)
@@ -1227,7 +1297,7 @@ static RISCVException write_mstatus(CPURISCVState *env, 
int csrno,
  
  /* flush tlb on mstatus fields that affect VM */

  if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-MSTATUS_MPRV | MSTATUS_SUM)) {
+MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | MSTATUS_UBCFIEN)) {


These two fields should be guarded by the check of ext_cfi.

And MSTATUS_UBCFIEN field change don't need flush tlb.

I didn't get why we should flush tlb for forward cfi. For background, 
there are some enhancement for the PTE and PMP, we may need do some
memory adjustments. But forward cfi just adds some instructions. Why we 
should flush tlb? Does the tlb can't be used any more?



  tlb_flush(env_cpu(env));
  }
  mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
@@ -1250,6 +1320,11 @@ static RISCVException write_mstatus(CPURISCVState *env, 
int csrno,
  }
  }
  
+/* If cfi extension is available, then apply cfi status mask */

+if (env_archcpu(env)->cfg.ext_cfi) {
+mask |= CFISTATUS_M_MASK;
+}
+
  mstatus = (mstatus & ~mask) | (val & mask);
  
  if (xl > MXL_RV32) {

@@ -1880,9 +1955,17 @@ static RISCVException write_menvcfg(CPURISCVState *env, 
int csrno,
target_ulong val)
  {
  uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | 
MENVCFG_CBZE;
+uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN;
  
  if (riscv_cpu_mxl(env) == MXL_RV64) {

  mask |= MENVCFG_PBMTE | MENVCFG_STCE;
+if (env_archcpu(env)->cfg.ext_cfi) {
+mask |= cfi_mask;
+/* If any cfi ena

[PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-08 Thread Deepak Gupta
CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch allows
access to these CSRs. A predicate routine handles access to these CSR as
per specification.

This patch also implments new bit definitions in menvcfg/henvcfg/mstatus/
sstatus CSRs to master enabled cfi and enable forward cfi in S and M mode.
mstatus CSR holds forward and backward cfi enabling for U mode.

There is no enabling bit for backward cfi in S and M mode. It is always
enabled if extension is implemented by CPU.

Signed-off-by: Deepak Gupta 
Signed-off-by: Kip Walker  
---
 target/riscv/csr.c | 137 -
 target/riscv/pmp.c |   9 +++
 2 files changed, 145 insertions(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0db2c233e5..24e208ebed 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, int csrno)
 return ctr(env, csrno);
 }
 
+static RISCVException cfi(CPURISCVState *env, int csrno)
+{
+/* no cfi extension */
+if (!env_archcpu(env)->cfg.ext_cfi) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+/*
+ * CONFIG_USER_MODE always allow access for now. Better for user mode only
+ * functionality
+ */
+#if !defined(CONFIG_USER_ONLY)
+/* current priv not M */
+if (env->priv != PRV_M) {
+/* menvcfg says no CFI */
+if (!get_field(env->menvcfg, MENVCFG_CFI)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+
+/* V = 1 and henvcfg says no CFI. raise virtual instr fault */
+if (riscv_cpu_virt_enabled(env) &&
+!get_field(env->henvcfg, HENVCFG_CFI)) {
+return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+}
+
+/*
+ * LPLR and SSP are not accessible to U mode if disabled via status
+ * CSR
+ */
+if (env->priv == PRV_U) {
+if (csrno == CSR_LPLR &&
+!get_field(env->mstatus, MSTATUS_UFCFIEN)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+if (csrno == CSR_SSP &&
+!get_field(env->mstatus, MSTATUS_UBCFIEN)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+}
+}
+#endif
+
+return RISCV_EXCP_NONE;
+}
+
 #if !defined(CONFIG_USER_ONLY)
 static RISCVException mctr(CPURISCVState *env, int csrno)
 {
@@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, int csrno)
 #endif
 }
 
+/* Zisslpcfi CSR_LPLR read/write */
+static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val)
+{
+*val = env->lplr;
+return RISCV_EXCP_NONE;
+}
+
+static int write_lplr(CPURISCVState *env, int csrno, target_ulong val)
+{
+env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL);
+return RISCV_EXCP_NONE;
+}
+
+/* Zisslpcfi CSR_SSP read/write */
+static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val)
+{
+*val = env->ssp;
+return RISCV_EXCP_NONE;
+}
+
+static int write_ssp(CPURISCVState *env, int csrno, target_ulong val)
+{
+env->ssp = val;
+return RISCV_EXCP_NONE;
+}
+
 /* User Floating-Point CSRs */
 static RISCVException read_fflags(CPURISCVState *env, int csrno,
   target_ulong *val)
@@ -1227,7 +1297,7 @@ static RISCVException write_mstatus(CPURISCVState *env, 
int csrno,
 
 /* flush tlb on mstatus fields that affect VM */
 if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-MSTATUS_MPRV | MSTATUS_SUM)) {
+MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | MSTATUS_UBCFIEN)) {
 tlb_flush(env_cpu(env));
 }
 mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
@@ -1250,6 +1320,11 @@ static RISCVException write_mstatus(CPURISCVState *env, 
int csrno,
 }
 }
 
+/* If cfi extension is available, then apply cfi status mask */
+if (env_archcpu(env)->cfg.ext_cfi) {
+mask |= CFISTATUS_M_MASK;
+}
+
 mstatus = (mstatus & ~mask) | (val & mask);
 
 if (xl > MXL_RV32) {
@@ -1880,9 +1955,17 @@ static RISCVException write_menvcfg(CPURISCVState *env, 
int csrno,
   target_ulong val)
 {
 uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
+uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN;
 
 if (riscv_cpu_mxl(env) == MXL_RV64) {
 mask |= MENVCFG_PBMTE | MENVCFG_STCE;
+if (env_archcpu(env)->cfg.ext_cfi) {
+mask |= cfi_mask;
+/* If any cfi enabling bit changes in menvcfg, flush tlb */
+if ((val ^ env->menvcfg) & cfi_mask) {
+tlb_flush(env_cpu(env));
+}
+}
 }
 env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
 
@@ -1900,8 +1983,17 @@ static RISCVException write_menvcfgh(CPURISCVState *env, 
int csrno,
   target_ulong val)
 {
 uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE;
+uint64_t cfi_mask = MENVCFG_CFI | MEN

[PATCH v1 RFC Zisslpcfi 3/9] target/riscv: implements CSRs and new bits in existing CSRs in zisslpcfi

2023-02-08 Thread Deepak Gupta
CSR_SSP and CSR_LPLR are new CSR additions to cpu/hart. This patch allows
access to these CSRs. A predicate routine handles access to these CSR as
per specification.

This patch also implments new bit definitions in menvcfg/henvcfg/mstatus/
sstatus CSRs to master enabled cfi and enable forward cfi in S and M mode.
mstatus CSR holds forward and backward cfi enabling for U mode.

There is no enabling bit for backward cfi in S and M mode. It is always
enabled if extension is implemented by CPU.

Signed-off-by: Deepak Gupta 
Signed-off-by: Kip Walker  
---
 target/riscv/csr.c | 137 -
 target/riscv/pmp.c |   9 +++
 2 files changed, 145 insertions(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0db2c233e5..24e208ebed 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -163,6 +163,50 @@ static RISCVException ctr32(CPURISCVState *env, int csrno)
 return ctr(env, csrno);
 }
 
+static RISCVException cfi(CPURISCVState *env, int csrno)
+{
+/* no cfi extension */
+if (!env_archcpu(env)->cfg.ext_cfi) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+/*
+ * CONFIG_USER_MODE always allow access for now. Better for user mode only
+ * functionality
+ */
+#if !defined(CONFIG_USER_ONLY)
+/* current priv not M */
+if (env->priv != PRV_M) {
+/* menvcfg says no CFI */
+if (!get_field(env->menvcfg, MENVCFG_CFI)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+
+/* V = 1 and henvcfg says no CFI. raise virtual instr fault */
+if (riscv_cpu_virt_enabled(env) &&
+!get_field(env->henvcfg, HENVCFG_CFI)) {
+return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+}
+
+/*
+ * LPLR and SSP are not accessible to U mode if disabled via status
+ * CSR
+ */
+if (env->priv == PRV_U) {
+if (csrno == CSR_LPLR &&
+!get_field(env->mstatus, MSTATUS_UFCFIEN)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+if (csrno == CSR_SSP &&
+!get_field(env->mstatus, MSTATUS_UBCFIEN)) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
+}
+}
+#endif
+
+return RISCV_EXCP_NONE;
+}
+
 #if !defined(CONFIG_USER_ONLY)
 static RISCVException mctr(CPURISCVState *env, int csrno)
 {
@@ -485,6 +529,32 @@ static RISCVException seed(CPURISCVState *env, int csrno)
 #endif
 }
 
+/* Zisslpcfi CSR_LPLR read/write */
+static int read_lplr(CPURISCVState *env, int csrno, target_ulong *val)
+{
+*val = env->lplr;
+return RISCV_EXCP_NONE;
+}
+
+static int write_lplr(CPURISCVState *env, int csrno, target_ulong val)
+{
+env->lplr = val & (LPLR_UL | LPLR_ML | LPLR_LL);
+return RISCV_EXCP_NONE;
+}
+
+/* Zisslpcfi CSR_SSP read/write */
+static int read_ssp(CPURISCVState *env, int csrno, target_ulong *val)
+{
+*val = env->ssp;
+return RISCV_EXCP_NONE;
+}
+
+static int write_ssp(CPURISCVState *env, int csrno, target_ulong val)
+{
+env->ssp = val;
+return RISCV_EXCP_NONE;
+}
+
 /* User Floating-Point CSRs */
 static RISCVException read_fflags(CPURISCVState *env, int csrno,
   target_ulong *val)
@@ -1227,7 +1297,7 @@ static RISCVException write_mstatus(CPURISCVState *env, 
int csrno,
 
 /* flush tlb on mstatus fields that affect VM */
 if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-MSTATUS_MPRV | MSTATUS_SUM)) {
+MSTATUS_MPRV | MSTATUS_SUM | MSTATUS_UFCFIEN | MSTATUS_UBCFIEN)) {
 tlb_flush(env_cpu(env));
 }
 mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
@@ -1250,6 +1320,11 @@ static RISCVException write_mstatus(CPURISCVState *env, 
int csrno,
 }
 }
 
+/* If cfi extension is available, then apply cfi status mask */
+if (env_archcpu(env)->cfg.ext_cfi) {
+mask |= CFISTATUS_M_MASK;
+}
+
 mstatus = (mstatus & ~mask) | (val & mask);
 
 if (xl > MXL_RV32) {
@@ -1880,9 +1955,17 @@ static RISCVException write_menvcfg(CPURISCVState *env, 
int csrno,
   target_ulong val)
 {
 uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE | MENVCFG_CBZE;
+uint64_t cfi_mask = MENVCFG_CFI | MENVCFG_SFCFIEN;
 
 if (riscv_cpu_mxl(env) == MXL_RV64) {
 mask |= MENVCFG_PBMTE | MENVCFG_STCE;
+if (env_archcpu(env)->cfg.ext_cfi) {
+mask |= cfi_mask;
+/* If any cfi enabling bit changes in menvcfg, flush tlb */
+if ((val ^ env->menvcfg) & cfi_mask) {
+tlb_flush(env_cpu(env));
+}
+}
 }
 env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
 
@@ -1900,8 +1983,17 @@ static RISCVException write_menvcfgh(CPURISCVState *env, 
int csrno,
   target_ulong val)
 {
 uint64_t mask = MENVCFG_PBMTE | MENVCFG_STCE;
+uint64_t cfi_mask = MENVCFG_CFI | MEN