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

[Fea Wang]

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