[PATCH v2 0/5] Introduce svukte ISA extension

[PATCH v2 0/5] Introduce svukte ISA extension

[Fea.Wang]

The Svukte ISA extension has been approved for fast-track development.
https://lf-riscv.atlassian.net/browse/RVS-2977 
And there are Linux patches for the Svukte that are under review.
https://lore.kernel.org/kvm/20240920-dev-maxh-svukte-rebase-v1-0-7864a88a62bd@sifive.com/T/#mf70fcb22cd2987ad268c0efee9b8583197d3cb4f

Svukte provides a means to make user-mode accesses to supervisor memory
raise page faults in constant time, mitigating attacks that attempt to
discover the supervisor software's address-space layout.

Refer to the draft of svukte extension from:
https://github.com/riscv/riscv-isa-manual/pull/1564

* Refactor the code

base-commit: 27652f9ca9d831c67dd447346c6ee953669255f0

[v1]
* Add svukte extension

Fea.Wang (5):
  target/riscv: Add svukte extension capability variable
  target/riscv: Support senvcfg[UKTE] bit when svukte extension is
    enabled
  target/riscv: Support hstatus[HUKTE] bit when svukte extension is
    enabled
  target/riscv: Check memory access to meet svuket rule
  target/riscv: Expose svukte ISA extension

 target/riscv/cpu.c        |  2 ++
 target/riscv/cpu_bits.h   |  2 ++
 target/riscv/cpu_cfg.h    |  1 +
 target/riscv/cpu_helper.c | 57 +++++++++++++++++++++++++++++++++++++++
 target/riscv/csr.c        |  7 +++++
 5 files changed, 69 insertions(+)

-- 
2.34.1

[PATCH v2 1/5] target/riscv: Add svukte extension capability variable

[Fea.Wang]

Refer to the draft of svukte extension from:
https://github.com/riscv/riscv-isa-manual/pull/1564

Svukte provides a means to make user-mode accesses to supervisor memory
raise page faults in constant time, mitigating attacks that attempt to
discover the supervisor software's address-space layout.

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Jim Shu <jim.shu@sifive.com>
---
 target/riscv/cpu_cfg.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 59d6fc445d..d8771ca641 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -84,6 +84,7 @@ struct RISCVCPUConfig {
     bool ext_svnapot;
     bool ext_svpbmt;
     bool ext_svvptc;
+    bool ext_svukte;
     bool ext_zdinx;
     bool ext_zaamo;
     bool ext_zacas;
-- 
2.34.1

Re: [PATCH v2 1/5] target/riscv: Add svukte extension capability variable

[Daniel Henrique Barboza]

On 11/8/24 5:52 AM, Fea.Wang wrote:
> Refer to the draft of svukte extension from:
> https://github.com/riscv/riscv-isa-manual/pull/1564
> 
> Svukte provides a means to make user-mode accesses to supervisor memory
> raise page faults in constant time, mitigating attacks that attempt to
> discover the supervisor software's address-space layout.
> 
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> ---

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

>   target/riscv/cpu_cfg.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 59d6fc445d..d8771ca641 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -84,6 +84,7 @@ struct RISCVCPUConfig {
>       bool ext_svnapot;
>       bool ext_svpbmt;
>       bool ext_svvptc;
> +    bool ext_svukte;
>       bool ext_zdinx;
>       bool ext_zaamo;
>       bool ext_zacas;

[PATCH v2 2/5] target/riscv: Support senvcfg[UKTE] bit when svukte extension is enabled

[Fea.Wang]

Svukte extension add UKTE bit, bit[8] in senvcfg CSR. The bit will be
supported when the svukte extension is enabled.

When senvcfg[UKTE] bit is set, the memory access from U-mode should do
the svukte check only except HLV/HLVX/HSV H-mode instructions which
depend on hstatus[HUKTE].

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Jim Shu <jim.shu@sifive.com>
---
 target/riscv/cpu_bits.h | 1 +
 target/riscv/csr.c      | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 385a2c67c2..4b9f899217 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -785,6 +785,7 @@ typedef enum RISCVException {
 #define SENVCFG_CBIE                       MENVCFG_CBIE
 #define SENVCFG_CBCFE                      MENVCFG_CBCFE
 #define SENVCFG_CBZE                       MENVCFG_CBZE
+#define SENVCFG_UKTE                       BIT(8)
 
 #define HENVCFG_FIOM                       MENVCFG_FIOM
 #define HENVCFG_LPE                        MENVCFG_LPE
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9846770820..1936a6f32a 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2453,6 +2453,10 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
         mask |= SENVCFG_SSE;
     }
 
+    if (env_archcpu(env)->cfg.ext_svukte) {
+        mask |= SENVCFG_UKTE;
+    }
+
     env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
     return RISCV_EXCP_NONE;
 }
-- 
2.34.1

Re: [PATCH v2 2/5] target/riscv: Support senvcfg[UKTE] bit when svukte extension is enabled

[Daniel Henrique Barboza]

On 11/8/24 5:52 AM, Fea.Wang wrote:
> Svukte extension add UKTE bit, bit[8] in senvcfg CSR. The bit will be
> supported when the svukte extension is enabled.
> 
> When senvcfg[UKTE] bit is set, the memory access from U-mode should do
> the svukte check only except HLV/HLVX/HSV H-mode instructions which
> depend on hstatus[HUKTE].
> 
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> ---

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

>   target/riscv/cpu_bits.h | 1 +
>   target/riscv/csr.c      | 4 ++++
>   2 files changed, 5 insertions(+)
> 
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 385a2c67c2..4b9f899217 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -785,6 +785,7 @@ typedef enum RISCVException {
>   #define SENVCFG_CBIE                       MENVCFG_CBIE
>   #define SENVCFG_CBCFE                      MENVCFG_CBCFE
>   #define SENVCFG_CBZE                       MENVCFG_CBZE
> +#define SENVCFG_UKTE                       BIT(8)
>   
>   #define HENVCFG_FIOM                       MENVCFG_FIOM
>   #define HENVCFG_LPE                        MENVCFG_LPE
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 9846770820..1936a6f32a 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2453,6 +2453,10 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
>           mask |= SENVCFG_SSE;
>       }
>   
> +    if (env_archcpu(env)->cfg.ext_svukte) {
> +        mask |= SENVCFG_UKTE;
> +    }
> +
>       env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
>       return RISCV_EXCP_NONE;
>   }

[PATCH v2 3/5] target/riscv: Support hstatus[HUKTE] bit when svukte extension is enabled

[Fea.Wang]

Svukte extension add HUKTE bit, bit[24] in hstatus CSR. The written
value will be masked when the svukte extension is not enabled.

When hstatus[HUKTE] bit is set, HLV/HLVX/HSV work in the U-mode should
do svukte check.

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Jim Shu <jim.shu@sifive.com>
---
 target/riscv/cpu_bits.h | 1 +
 target/riscv/csr.c      | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 4b9f899217..fe4e34c64a 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -604,6 +604,7 @@ typedef enum {
 #define HSTATUS_VTVM         0x00100000
 #define HSTATUS_VTW          0x00200000
 #define HSTATUS_VTSR         0x00400000
+#define HSTATUS_HUKTE        0x01000000
 #define HSTATUS_VSXL         0x300000000
 
 #define HSTATUS32_WPRI       0xFF8FF87E
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 1936a6f32a..b6fa8ae53f 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3540,6 +3540,9 @@ static RISCVException read_hstatus(CPURISCVState *env, int csrno,
 static RISCVException write_hstatus(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
+    if (!env_archcpu(env)->cfg.ext_svukte) {
+        val = val & (~HSTATUS_HUKTE);
+    }
     env->hstatus = val;
     if (riscv_cpu_mxl(env) != MXL_RV32 && get_field(val, HSTATUS_VSXL) != 2) {
         qemu_log_mask(LOG_UNIMP,
-- 
2.34.1

Re: [PATCH v2 3/5] target/riscv: Support hstatus[HUKTE] bit when svukte extension is enabled

[Daniel Henrique Barboza]

On 11/8/24 5:52 AM, Fea.Wang wrote:
> Svukte extension add HUKTE bit, bit[24] in hstatus CSR. The written
> value will be masked when the svukte extension is not enabled.
> 
> When hstatus[HUKTE] bit is set, HLV/HLVX/HSV work in the U-mode should
> do svukte check.
> 
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> ---

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

>   target/riscv/cpu_bits.h | 1 +
>   target/riscv/csr.c      | 3 +++
>   2 files changed, 4 insertions(+)
> 
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 4b9f899217..fe4e34c64a 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -604,6 +604,7 @@ typedef enum {
>   #define HSTATUS_VTVM         0x00100000
>   #define HSTATUS_VTW          0x00200000
>   #define HSTATUS_VTSR         0x00400000
> +#define HSTATUS_HUKTE        0x01000000
>   #define HSTATUS_VSXL         0x300000000
>   
>   #define HSTATUS32_WPRI       0xFF8FF87E
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 1936a6f32a..b6fa8ae53f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3540,6 +3540,9 @@ static RISCVException read_hstatus(CPURISCVState *env, int csrno,
>   static RISCVException write_hstatus(CPURISCVState *env, int csrno,
>                                       target_ulong val)
>   {
> +    if (!env_archcpu(env)->cfg.ext_svukte) {
> +        val = val & (~HSTATUS_HUKTE);
> +    }
>       env->hstatus = val;
>       if (riscv_cpu_mxl(env) != MXL_RV32 && get_field(val, HSTATUS_VSXL) != 2) {
>           qemu_log_mask(LOG_UNIMP,

[PATCH v2 4/5] target/riscv: Check memory access to meet svuket rule

[Fea.Wang]

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 excute 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>
Reviewed-by: Jim Shu <jim.shu@sifive.com>
---
 target/riscv/cpu_helper.c | 57 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 0a3ead69ea..725c049f7a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -857,6 +857,57 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
     return TRANSLATE_SUCCESS;
 }
 
+/*
+ * Return 'true' means no need to do svukte check, or need to do svukte and the
+ * address is valid. Return 'false' means need to do svukte check but address
+ * is invalid.
+ */
+static bool check_svukte_valid(CPURISCVState *env, vaddr addr,
+                               int mode, bool virt)
+{
+    /*
+     * 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.
+     */
+    bool ukte;
+    if (env->priv == PRV_U && !env->virt_enabled && virt) {
+        ukte = !!(env->hstatus & HSTATUS_HUKTE);
+    } else {
+        ukte = !!(env->senvcfg & SENVCFG_UKTE);
+    }
+
+    if (VM_1_10_SV39 != get_field(env->satp, SATP64_MODE))  {
+        /* Svukte extension depends on Sv39. */
+        return true;
+    }
+
+    /*
+     * 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 true;
+    }
+
+    if (!ukte) {
+        return true;
+    }
+
+    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 +945,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 +953,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 (env_archcpu(env)->cfg.ext_svukte && first_stage &&
+        !check_svukte_valid(env, addr, mode, virt)) {
+        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

Re: [PATCH v2 4/5] target/riscv: Check memory access to meet svuket rule

[Daniel Henrique Barboza]

On 11/8/24 5:52 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 excute from U mode to VU mode

typo: s/excute/execute

> 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>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> ---
>   target/riscv/cpu_helper.c | 57 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 57 insertions(+)
> 
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 0a3ead69ea..725c049f7a 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -857,6 +857,57 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
>       return TRANSLATE_SUCCESS;
>   }
>   
> +/*
> + * Return 'true' means no need to do svukte check, or need to do svukte and the
> + * address is valid. Return 'false' means need to do svukte check but address
> + * is invalid.
> + */

I think this function is doing a bit too much while not doing everything we need.
For instance, we're still doing some checks in get_physical_address() prior to
calling it.

I would put all the svukte logic in two functions:

- 'do_svukte_check' would check all dependencies for the svukte check per se
- 'check_svukte_addr' would do the addr check if we need to do a svukte check


An example:



/* 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;
}


And then on get_physical_addr():


     if (do_svukte_check(env, first_stage, mode, virt) &&
         !check_svukte_addr(env, addr)) {
         return TRANSLATE_FAIL;
     }


Feel free to use these functions (compiled tested only) if you want. Thanks,

Daniel


> +static bool check_svukte_valid(CPURISCVState *env, vaddr addr,
> +                               int mode, bool virt)
> +{
> +    /*
> +     * 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.
> +     */
> +    bool ukte;
> +    if (env->priv == PRV_U && !env->virt_enabled && virt) {
> +        ukte = !!(env->hstatus & HSTATUS_HUKTE);
> +    } else {
> +        ukte = !!(env->senvcfg & SENVCFG_UKTE);
> +    }
> +
> +    if (VM_1_10_SV39 != get_field(env->satp, SATP64_MODE))  {
> +        /* Svukte extension depends on Sv39. */
> +        return true;
> +    }> +
> +    /*
> +     * 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 true;
> +    }
> +
> +    if (!ukte) {
> +        return true;
> +    }
> +
> +    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 +945,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 +953,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 (env_archcpu(env)->cfg.ext_svukte && first_stage &&
> +        !check_svukte_valid(env, addr, mode, virt)) {
> +        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

Re: [PATCH v2 4/5] target/riscv: Check memory access to meet svuket rule

[Fea Wang]

[-- Attachment #1: Type: text/plain, Size: 6711 bytes --]

Thank you for refining it.
I fixed some parts and will put them in the V3 patches.

Sincerely,
Fea

On Tue, Nov 12, 2024 at 2:32 AM Daniel Henrique Barboza <
dbarboza@ventanamicro.com> wrote:

>
>
> On 11/8/24 5:52 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 excute from U mode to VU mode
>
> typo: s/excute/execute
>
> > 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>
> > Reviewed-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >   target/riscv/cpu_helper.c | 57 +++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 57 insertions(+)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 0a3ead69ea..725c049f7a 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -857,6 +857,57 @@ static int get_physical_address_pmp(CPURISCVState
> *env, int *prot, hwaddr addr,
> >       return TRANSLATE_SUCCESS;
> >   }
> >
> > +/*
> > + * Return 'true' means no need to do svukte check, or need to do svukte
> and the
> > + * address is valid. Return 'false' means need to do svukte check but
> address
> > + * is invalid.
> > + */
>
> I think this function is doing a bit too much while not doing everything
> we need.
> For instance, we're still doing some checks in get_physical_address()
> prior to
> calling it.
>
> I would put all the svukte logic in two functions:
>
> - 'do_svukte_check' would check all dependencies for the svukte check per
> se
> - 'check_svukte_addr' would do the addr check if we need to do a svukte
> check
>
>
> An example:
>
>
>
> /* 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;
> }
>
>
> And then on get_physical_addr():
>
>
>      if (do_svukte_check(env, first_stage, mode, virt) &&
>          !check_svukte_addr(env, addr)) {
>          return TRANSLATE_FAIL;
>      }
>
>
> Feel free to use these functions (compiled tested only) if you want.
> Thanks,
>
> Daniel
>
>
> > +static bool check_svukte_valid(CPURISCVState *env, vaddr addr,
> > +                               int mode, bool virt)
> > +{
> > +    /*
> > +     * 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.
> > +     */
> > +    bool ukte;
> > +    if (env->priv == PRV_U && !env->virt_enabled && virt) {
> > +        ukte = !!(env->hstatus & HSTATUS_HUKTE);
> > +    } else {
> > +        ukte = !!(env->senvcfg & SENVCFG_UKTE);
> > +    }
> > +
> > +    if (VM_1_10_SV39 != get_field(env->satp, SATP64_MODE))  {
> > +        /* Svukte extension depends on Sv39. */
> > +        return true;
> > +    }> +
> > +    /*
> > +     * 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 true;
> > +    }
> > +
> > +    if (!ukte) {
> > +        return true;
> > +    }
> > +
> > +    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 +945,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 +953,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 (env_archcpu(env)->cfg.ext_svukte && first_stage &&
> > +        !check_svukte_valid(env, addr, mode, virt)) {
> > +        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
>

[-- Attachment #2: Type: text/html, Size: 8581 bytes --]

[PATCH v2 5/5] target/riscv: Expose svukte ISA extension

[Fea.Wang]

Add "svukte" in the ISA string when svukte extension is enabled.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f219f0c3b5..69187c9aa1 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -200,6 +200,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
     ISA_EXT_DATA_ENTRY(svpbmt, PRIV_VERSION_1_12_0, ext_svpbmt),
     ISA_EXT_DATA_ENTRY(svvptc, PRIV_VERSION_1_13_0, ext_svvptc),
+    ISA_EXT_DATA_ENTRY(svukte, PRIV_VERSION_1_13_0, ext_svukte),
     ISA_EXT_DATA_ENTRY(xtheadba, PRIV_VERSION_1_11_0, ext_xtheadba),
     ISA_EXT_DATA_ENTRY(xtheadbb, PRIV_VERSION_1_11_0, ext_xtheadbb),
     ISA_EXT_DATA_ENTRY(xtheadbs, PRIV_VERSION_1_11_0, ext_xtheadbs),
@@ -1595,6 +1596,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
 
 /* These are experimental so mark with 'x-' */
 const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
+    MULTI_EXT_CFG_BOOL("x-svukte", ext_svukte, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.34.1

Re: [PATCH v2 5/5] target/riscv: Expose svukte ISA extension

[Daniel Henrique Barboza]

On 11/8/24 5:52 AM, Fea.Wang wrote:
> Add "svukte" in the ISA string when svukte extension is enabled.
> 
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> ---
>   target/riscv/cpu.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f219f0c3b5..69187c9aa1 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -200,6 +200,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>       ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
>       ISA_EXT_DATA_ENTRY(svpbmt, PRIV_VERSION_1_12_0, ext_svpbmt),
>       ISA_EXT_DATA_ENTRY(svvptc, PRIV_VERSION_1_13_0, ext_svvptc),
> +    ISA_EXT_DATA_ENTRY(svukte, PRIV_VERSION_1_13_0, ext_svukte),

We need to stick with the riscv,isa order, i.e. svukte must go before
svvptc:


>       ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
>       ISA_EXT_DATA_ENTRY(svpbmt, PRIV_VERSION_1_12_0, ext_svpbmt),
> +    ISA_EXT_DATA_ENTRY(svukte, PRIV_VERSION_1_13_0, ext_svukte),
>       ISA_EXT_DATA_ENTRY(svvptc, PRIV_VERSION_1_13_0, ext_svvptc),



Thanks,

Daniel


>       ISA_EXT_DATA_ENTRY(xtheadba, PRIV_VERSION_1_11_0, ext_xtheadba),
>       ISA_EXT_DATA_ENTRY(xtheadbb, PRIV_VERSION_1_11_0, ext_xtheadbb),
>       ISA_EXT_DATA_ENTRY(xtheadbs, PRIV_VERSION_1_11_0, ext_xtheadbs),
> @@ -1595,6 +1596,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
>   
>   /* These are experimental so mark with 'x-' */
>   const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
> +    MULTI_EXT_CFG_BOOL("x-svukte", ext_svukte, false),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   

Re: [PATCH v2 5/5] target/riscv: Expose svukte ISA extension

[Fea Wang]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

Got it, thank you.

Sincerely,
Fea

On Tue, Nov 12, 2024 at 2:33 AM Daniel Henrique Barboza <
dbarboza@ventanamicro.com> wrote:

>
>
> On 11/8/24 5:52 AM, Fea.Wang wrote:
> > Add "svukte" in the ISA string when svukte extension is enabled.
> >
> > Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> > Reviewed-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >   target/riscv/cpu.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index f219f0c3b5..69187c9aa1 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -200,6 +200,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >       ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
> >       ISA_EXT_DATA_ENTRY(svpbmt, PRIV_VERSION_1_12_0, ext_svpbmt),
> >       ISA_EXT_DATA_ENTRY(svvptc, PRIV_VERSION_1_13_0, ext_svvptc),
> > +    ISA_EXT_DATA_ENTRY(svukte, PRIV_VERSION_1_13_0, ext_svukte),
>
> We need to stick with the riscv,isa order, i.e. svukte must go before
> svvptc:
>
>
> >       ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
> >       ISA_EXT_DATA_ENTRY(svpbmt, PRIV_VERSION_1_12_0, ext_svpbmt),
> > +    ISA_EXT_DATA_ENTRY(svukte, PRIV_VERSION_1_13_0, ext_svukte),
> >       ISA_EXT_DATA_ENTRY(svvptc, PRIV_VERSION_1_13_0, ext_svvptc),
>
>
>
> Thanks,
>
> Daniel
>
>
> >       ISA_EXT_DATA_ENTRY(xtheadba, PRIV_VERSION_1_11_0, ext_xtheadba),
> >       ISA_EXT_DATA_ENTRY(xtheadbb, PRIV_VERSION_1_11_0, ext_xtheadbb),
> >       ISA_EXT_DATA_ENTRY(xtheadbs, PRIV_VERSION_1_11_0, ext_xtheadbs),
> > @@ -1595,6 +1596,7 @@ const RISCVCPUMultiExtConfig
> riscv_cpu_vendor_exts[] = {
> >
> >   /* These are experimental so mark with 'x-' */
> >   const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
> > +    MULTI_EXT_CFG_BOOL("x-svukte", ext_svukte, false),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >
>

[-- Attachment #2: Type: text/html, Size: 2787 bytes --]