diff mbox series

[v3,4/5] target/riscv: Check memory access to meet svukte rule

Message ID 20241112091423.2529583-5-fea.wang@sifive.com
State New
Headers show
Series Introduce svukte ISA extension | expand

Commit Message

Fea.Wang Nov. 12, 2024, 9:14 a.m. UTC 
Follow the Svukte spec, do the memory access address checking

1. Include instruction fetches or explicit memory accesses
2. System run in effective privilege U or VU
3. Check senvcfg[UKTE] being set, or hstatus[HUKTE] being set if
instruction is HLV, HLVX, HSV and execute from U mode to VU mode
4. Depend on Sv39 and check virtual addresses bit[SXLEN-1]
5. Raises a page-fault exception corresponding to the original access
type.

Ref: https://github.com/riscv/riscv-isa-manual/pull/1564/files

Signed-off-by: Frank Chang <frank.chang@sifive.com>
Signed-off-by: Fea.Wang <fea.wang@sifive.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Jim Shu <jim.shu@sifive.com>
---
 target/riscv/cpu_helper.c | 61 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

Comments

Daniel Henrique Barboza Nov. 12, 2024, 12:52 p.m. UTC | #1 
On 11/12/24 6:14 AM, Fea.Wang wrote:
> Follow the Svukte spec, do the memory access address checking
> 
> 1. Include instruction fetches or explicit memory accesses
> 2. System run in effective privilege U or VU
> 3. Check senvcfg[UKTE] being set, or hstatus[HUKTE] being set if
> instruction is HLV, HLVX, HSV and execute from U mode to VU mode
> 4. Depend on Sv39 and check virtual addresses bit[SXLEN-1]
> 5. Raises a page-fault exception corresponding to the original access
> type.
> 
> Ref: https://github.com/riscv/riscv-isa-manual/pull/1564/files
> 
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

> ---
>   target/riscv/cpu_helper.c | 61 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 61 insertions(+)
> 
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 0a3ead69ea..5b29344c4f 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -857,6 +857,61 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
>       return TRANSLATE_SUCCESS;
>   }
>   
> +/* Returns 'true' if a svukte address check is needed */
> +static bool do_svukte_check(CPURISCVState *env, bool first_stage,
> +                             int mode, bool virt)
> +{
> +    bool ukte;
> +
> +    /* Svukte extension depends on Sv39. */
> +    if (!(env_archcpu(env)->cfg.ext_svukte ||
> +        !first_stage ||
> +        VM_1_10_SV39 != get_field(env->satp, SATP64_MODE))) {
> +        return false;
> +    }
> +
> +    /*
> +     * Check hstatus.HUKTE if the effective mode is switched to VU-mode by
> +     * executing HLV/HLVX/HSV in U-mode.
> +     * For other cases, check senvcfg.UKTE.
> +     */
> +    if (env->priv == PRV_U && !env->virt_enabled && virt) {
> +        ukte = !!(env->hstatus & HSTATUS_HUKTE);
> +    } else {
> +        ukte = !!(env->senvcfg & SENVCFG_UKTE);
> +    }
> +
> +    if (!ukte) {
> +        return false;
> +    }
> +
> +    /*
> +     * Svukte extension is qualified only in U or VU-mode.
> +     *
> +     * Effective mode can be switched to U or VU-mode by:
> +     *   - M-mode + mstatus.MPRV=1 + mstatus.MPP=U-mode.
> +     *   - Execute HLV/HLVX/HSV from HS-mode + hstatus.SPVP=0.
> +     *   - U-mode.
> +     *   - VU-mode.
> +     *   - Execute HLV/HLVX/HSV from U-mode + hstatus.HU=1.
> +     */
> +    if (mode != PRV_U) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static bool check_svukte_addr(CPURISCVState *env, vaddr addr)
> +{
> +    uint32_t sxl = riscv_cpu_sxl(env);
> +    sxl = (sxl == 0) ? MXL_RV32 : sxl;
> +    uint32_t sxlen = 32 * sxl;
> +    uint64_t high_bit = addr & (1UL << (sxlen - 1));
> +
> +    return !high_bit;
> +}
> +
>   /*
>    * get_physical_address - get the physical address for this virtual address
>    *
> @@ -894,6 +949,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>       MemTxResult res;
>       MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>       int mode = mmuidx_priv(mmu_idx);
> +    bool virt = mmuidx_2stage(mmu_idx);
>       bool use_background = false;
>       hwaddr ppn;
>       int napot_bits = 0;
> @@ -901,6 +957,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>       bool is_sstack_idx = ((mmu_idx & MMU_IDX_SS_WRITE) == MMU_IDX_SS_WRITE);
>       bool sstack_page = false;
>   
> +    if (do_svukte_check(env, first_stage, mode, virt) &&
> +        !check_svukte_addr(env, addr)) {
> +        return TRANSLATE_FAIL;
> +    }
> +
>       /*
>        * Check if we should use the background registers for the two
>        * stage translation. We don't need to check if we actually need
Alistair Francis Nov. 19, 2024, 3:33 a.m. UTC | #2 
On Tue, Nov 12, 2024 at 7:13 PM Fea.Wang <fea.wang@sifive.com> wrote:
>
> Follow the Svukte spec, do the memory access address checking
>
> 1. Include instruction fetches or explicit memory accesses
> 2. System run in effective privilege U or VU
> 3. Check senvcfg[UKTE] being set, or hstatus[HUKTE] being set if
> instruction is HLV, HLVX, HSV and execute from U mode to VU mode
> 4. Depend on Sv39 and check virtual addresses bit[SXLEN-1]
> 5. Raises a page-fault exception corresponding to the original access
> type.
>
> Ref: https://github.com/riscv/riscv-isa-manual/pull/1564/files
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> ---
>  target/riscv/cpu_helper.c | 61 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 0a3ead69ea..5b29344c4f 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -857,6 +857,61 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
>      return TRANSLATE_SUCCESS;
>  }
>
> +/* Returns 'true' if a svukte address check is needed */
> +static bool do_svukte_check(CPURISCVState *env, bool first_stage,
> +                             int mode, bool virt)
> +{
> +    bool ukte;
> +
> +    /* Svukte extension depends on Sv39. */
> +    if (!(env_archcpu(env)->cfg.ext_svukte ||
> +        !first_stage ||
> +        VM_1_10_SV39 != get_field(env->satp, SATP64_MODE))) {
> +        return false;
> +    }
> +
> +    /*
> +     * Check hstatus.HUKTE if the effective mode is switched to VU-mode by
> +     * executing HLV/HLVX/HSV in U-mode.
> +     * For other cases, check senvcfg.UKTE.
> +     */
> +    if (env->priv == PRV_U && !env->virt_enabled && virt) {
> +        ukte = !!(env->hstatus & HSTATUS_HUKTE);

You should just be able to use get_field() here

> +    } else {
> +        ukte = !!(env->senvcfg & SENVCFG_UKTE);
> +    }
> +
> +    if (!ukte) {
> +        return false;

and it's probably simpler to remove the ukte variable and just return
based on the result of get_field()

> +    }
> +
> +    /*
> +     * Svukte extension is qualified only in U or VU-mode.
> +     *
> +     * Effective mode can be switched to U or VU-mode by:
> +     *   - M-mode + mstatus.MPRV=1 + mstatus.MPP=U-mode.
> +     *   - Execute HLV/HLVX/HSV from HS-mode + hstatus.SPVP=0.
> +     *   - U-mode.
> +     *   - VU-mode.
> +     *   - Execute HLV/HLVX/HSV from U-mode + hstatus.HU=1.
> +     */
> +    if (mode != PRV_U) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static bool check_svukte_addr(CPURISCVState *env, vaddr addr)
> +{
> +    uint32_t sxl = riscv_cpu_sxl(env);
> +    sxl = (sxl == 0) ? MXL_RV32 : sxl;

I don't think riscv_cpu_sxl() can return 0, do we actually need this check?

Also this extension isn't defined for RV32

Alistair

> +    uint32_t sxlen = 32 * sxl;
> +    uint64_t high_bit = addr & (1UL << (sxlen - 1));
> +
> +    return !high_bit;
> +}
> +
>  /*
>   * get_physical_address - get the physical address for this virtual address
>   *
> @@ -894,6 +949,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>      MemTxResult res;
>      MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>      int mode = mmuidx_priv(mmu_idx);
> +    bool virt = mmuidx_2stage(mmu_idx);
>      bool use_background = false;
>      hwaddr ppn;
>      int napot_bits = 0;
> @@ -901,6 +957,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>      bool is_sstack_idx = ((mmu_idx & MMU_IDX_SS_WRITE) == MMU_IDX_SS_WRITE);
>      bool sstack_page = false;
>
> +    if (do_svukte_check(env, first_stage, mode, virt) &&
> +        !check_svukte_addr(env, addr)) {
> +        return TRANSLATE_FAIL;
> +    }
> +
>      /*
>       * Check if we should use the background registers for the two
>       * stage translation. We don't need to check if we actually need
> --
> 2.34.1
>
>
Fea.Wang Nov. 20, 2024, 7:42 a.m. UTC | #3 
Thanks for the advice.
I will fix them in the next patch version.

Sincerely,
Fea

On Tue, Nov 19, 2024 at 11:33 AM Alistair Francis <alistair23@gmail.com>
wrote:

> On Tue, Nov 12, 2024 at 7:13 PM Fea.Wang <fea.wang@sifive.com> wrote:
> >
> > Follow the Svukte spec, do the memory access address checking
> >
> > 1. Include instruction fetches or explicit memory accesses
> > 2. System run in effective privilege U or VU
> > 3. Check senvcfg[UKTE] being set, or hstatus[HUKTE] being set if
> > instruction is HLV, HLVX, HSV and execute from U mode to VU mode
> > 4. Depend on Sv39 and check virtual addresses bit[SXLEN-1]
> > 5. Raises a page-fault exception corresponding to the original access
> > type.
> >
> > Ref: https://github.com/riscv/riscv-isa-manual/pull/1564/files
> >
> > Signed-off-by: Frank Chang <frank.chang@sifive.com>
> > Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > Reviewed-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >  target/riscv/cpu_helper.c | 61 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 0a3ead69ea..5b29344c4f 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -857,6 +857,61 @@ static int get_physical_address_pmp(CPURISCVState
> *env, int *prot, hwaddr addr,
> >      return TRANSLATE_SUCCESS;
> >  }
> >
> > +/* Returns 'true' if a svukte address check is needed */
> > +static bool do_svukte_check(CPURISCVState *env, bool first_stage,
> > +                             int mode, bool virt)
> > +{
> > +    bool ukte;
> > +
> > +    /* Svukte extension depends on Sv39. */
> > +    if (!(env_archcpu(env)->cfg.ext_svukte ||
> > +        !first_stage ||
> > +        VM_1_10_SV39 != get_field(env->satp, SATP64_MODE))) {
> > +        return false;
> > +    }
> > +
> > +    /*
> > +     * Check hstatus.HUKTE if the effective mode is switched to VU-mode
> by
> > +     * executing HLV/HLVX/HSV in U-mode.
> > +     * For other cases, check senvcfg.UKTE.
> > +     */
> > +    if (env->priv == PRV_U && !env->virt_enabled && virt) {
> > +        ukte = !!(env->hstatus & HSTATUS_HUKTE);
>
> You should just be able to use get_field() here
>
> > +    } else {
> > +        ukte = !!(env->senvcfg & SENVCFG_UKTE);
> > +    }
> > +
> > +    if (!ukte) {
> > +        return false;
>
> and it's probably simpler to remove the ukte variable and just return
> based on the result of get_field()
>
> > +    }
> > +
> > +    /*
> > +     * Svukte extension is qualified only in U or VU-mode.
> > +     *
> > +     * Effective mode can be switched to U or VU-mode by:
> > +     *   - M-mode + mstatus.MPRV=1 + mstatus.MPP=U-mode.
> > +     *   - Execute HLV/HLVX/HSV from HS-mode + hstatus.SPVP=0.
> > +     *   - U-mode.
> > +     *   - VU-mode.
> > +     *   - Execute HLV/HLVX/HSV from U-mode + hstatus.HU=1.
> > +     */
> > +    if (mode != PRV_U) {
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static bool check_svukte_addr(CPURISCVState *env, vaddr addr)
> > +{
> > +    uint32_t sxl = riscv_cpu_sxl(env);
> > +    sxl = (sxl == 0) ? MXL_RV32 : sxl;
>
> I don't think riscv_cpu_sxl() can return 0, do we actually need this check?
>
> Also this extension isn't defined for RV32
>
> Alistair
>
> > +    uint32_t sxlen = 32 * sxl;
> > +    uint64_t high_bit = addr & (1UL << (sxlen - 1));
> > +
> > +    return !high_bit;
> > +}
> > +
> >  /*
> >   * get_physical_address - get the physical address for this virtual
> address
> >   *
> > @@ -894,6 +949,7 @@ static int get_physical_address(CPURISCVState *env,
> hwaddr *physical,
> >      MemTxResult res;
> >      MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> >      int mode = mmuidx_priv(mmu_idx);
> > +    bool virt = mmuidx_2stage(mmu_idx);
> >      bool use_background = false;
> >      hwaddr ppn;
> >      int napot_bits = 0;
> > @@ -901,6 +957,11 @@ static int get_physical_address(CPURISCVState *env,
> hwaddr *physical,
> >      bool is_sstack_idx = ((mmu_idx & MMU_IDX_SS_WRITE) ==
> MMU_IDX_SS_WRITE);
> >      bool sstack_page = false;
> >
> > +    if (do_svukte_check(env, first_stage, mode, virt) &&
> > +        !check_svukte_addr(env, addr)) {
> > +        return TRANSLATE_FAIL;
> > +    }
> > +
> >      /*
> >       * Check if we should use the background registers for the two
> >       * stage translation. We don't need to check if we actually need
> > --
> > 2.34.1
> >
> >
>
diff mbox series

Patch

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 0a3ead69ea..5b29344c4f 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -857,6 +857,61 @@  static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
     return TRANSLATE_SUCCESS;
 }
 
+/* Returns 'true' if a svukte address check is needed */
+static bool do_svukte_check(CPURISCVState *env, bool first_stage,
+                             int mode, bool virt)
+{
+    bool ukte;
+
+    /* Svukte extension depends on Sv39. */
+    if (!(env_archcpu(env)->cfg.ext_svukte ||
+        !first_stage ||
+        VM_1_10_SV39 != get_field(env->satp, SATP64_MODE))) {
+        return false;
+    }
+
+    /*
+     * Check hstatus.HUKTE if the effective mode is switched to VU-mode by
+     * executing HLV/HLVX/HSV in U-mode.
+     * For other cases, check senvcfg.UKTE.
+     */
+    if (env->priv == PRV_U && !env->virt_enabled && virt) {
+        ukte = !!(env->hstatus & HSTATUS_HUKTE);
+    } else {
+        ukte = !!(env->senvcfg & SENVCFG_UKTE);
+    }
+
+    if (!ukte) {
+        return false;
+    }
+
+    /*
+     * Svukte extension is qualified only in U or VU-mode.
+     *
+     * Effective mode can be switched to U or VU-mode by:
+     *   - M-mode + mstatus.MPRV=1 + mstatus.MPP=U-mode.
+     *   - Execute HLV/HLVX/HSV from HS-mode + hstatus.SPVP=0.
+     *   - U-mode.
+     *   - VU-mode.
+     *   - Execute HLV/HLVX/HSV from U-mode + hstatus.HU=1.
+     */
+    if (mode != PRV_U) {
+        return false;
+    }
+
+    return true;
+}
+
+static bool check_svukte_addr(CPURISCVState *env, vaddr addr)
+{
+    uint32_t sxl = riscv_cpu_sxl(env);
+    sxl = (sxl == 0) ? MXL_RV32 : sxl;
+    uint32_t sxlen = 32 * sxl;
+    uint64_t high_bit = addr & (1UL << (sxlen - 1));
+
+    return !high_bit;
+}
+
 /*
  * get_physical_address - get the physical address for this virtual address
  *
@@ -894,6 +949,7 @@  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
     MemTxResult res;
     MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     int mode = mmuidx_priv(mmu_idx);
+    bool virt = mmuidx_2stage(mmu_idx);
     bool use_background = false;
     hwaddr ppn;
     int napot_bits = 0;
@@ -901,6 +957,11 @@  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
     bool is_sstack_idx = ((mmu_idx & MMU_IDX_SS_WRITE) == MMU_IDX_SS_WRITE);
     bool sstack_page = false;
 
+    if (do_svukte_check(env, first_stage, mode, virt) &&
+        !check_svukte_addr(env, addr)) {
+        return TRANSLATE_FAIL;
+    }
+
     /*
      * Check if we should use the background registers for the two
      * stage translation. We don't need to check if we actually need