| Message ID | 20240903061757.1114957-5-fea.wang@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce svukte ISA extension | expand |
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.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 | 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
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 < dbarboza@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.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 | 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 >
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); + + 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; + } + } + /* * Check if we should use the background registers for the two * stage translation. We don't need to check if we actually need