Thank you for your advice. I will take them after the spec is more finalized.
Sincerely, Fea On Wed, Sep 4, 2024 at 6:18 AM Daniel Henrique Barboza < dbarb...@ventanamicro.com> wrote: > > > On 9/3/24 3:17 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 > > 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.ch...@sifive.com> > > Signed-off-by: Fea.Wang <fea.w...@sifive.com> > > Reviewed-by: Jim Shu <jim....@sifive.com> > > --- > > target/riscv/cpu_helper.c | 55 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > index 395a1d9140..db65ed14b9 100644 > > --- a/target/riscv/cpu_helper.c > > +++ b/target/riscv/cpu_helper.c > > @@ -777,6 +777,54 @@ 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) > > +{ > > + 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; > > + } > > + > > + /* > > + * 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 = (env->priv == PRV_U && !env->virt_enabled && virt) ? > > + !!(env->hstatus & > HSTATUS_HUKTE) : > > + !!(env->senvcfg & > SENVCFG_UKTE); > > I would move the 'bool ukte' to the start of the function, and would avoid > the > ternary to make the code a bit more readable: > > if (env->priv == PRV_U && !env->virt_enabled && virt) { > ukte = !!(env->hstatus & HSTATUS_HUKTE); > } else { > ukte = !!(env->senvcfg & SENVCFG_UKTE); > } > > > > + > > + 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 > > * > > @@ -814,11 +862,18 @@ 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; > > target_ulong napot_mask; > > > > + if (first_stage) { > > + if (!check_svukte_valid(env, addr, mode, virt)) { > > + return TRANSLATE_FAIL; > > + } > > + } > > + > > We can avoid the nested 'if': > > > + if (first_stage && !check_svukte_valid(env, addr, mode, virt)) { > > + return TRANSLATE_FAIL; > > + } > > > I would also add a check for ext_svukte before doing any checks. If we > don't have > the ext enabled we can skip everything: > > > > + if (env_archcpu(env)->cfg.ext_svukte && first_stage && > > + !check_svukte_valid(env, addr, mode, virt)) { > > + return TRANSLATE_FAIL; > > + } > > > > Thanks, > > Daniel > > > > /* > > * Check if we should use the background registers for the two > > * stage translation. We don't need to check if we actually need >
