| Message ID | 20230529121719.179507-2-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Fix mstatus related problems | expand |
On 5/29/23 09:17, Weiwei Li wrote: > Upon MRET or explicit memory access with MPRV=1, MPV should be ignored > when MPP=PRV_M. > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- > target/riscv/cpu_helper.c | 3 ++- > target/riscv/op_helper.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 09ea227ceb..bd892c05d4 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > > if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { > mode = get_field(env->mstatus, MSTATUS_MPP); > - virt = get_field(env->mstatus, MSTATUS_MPV); > + virt = get_field(env->mstatus, MSTATUS_MPV) && > + (mode != PRV_M); This change makes more sense in patch 2 where you also removed the 'mode' check for MPRV. As it is now I read the code above and thought "but mode is guaranteed to be == PRV_M, so (mode != PRV_M) is guaranteed to be false every time". The change in helper_mret() below is fine. Thanks, Daniel > if (virt) { > status = env->vsstatus; > } > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index f563dc3981..9cdb9cdd06 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) > riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC()); > } > > - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); > + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && > + (prev_priv != PRV_M); > mstatus = set_field(mstatus, MSTATUS_MIE, > get_field(mstatus, MSTATUS_MPIE)); > mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
On 2023/5/31 04:23, Daniel Henrique Barboza wrote: > > > On 5/29/23 09:17, Weiwei Li wrote: >> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored >> when MPP=PRV_M. >> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> --- >> target/riscv/cpu_helper.c | 3 ++- >> target/riscv/op_helper.c | 3 ++- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index 09ea227ceb..bd892c05d4 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool >> ifetch) >> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { >> mode = get_field(env->mstatus, MSTATUS_MPP); >> - virt = get_field(env->mstatus, MSTATUS_MPV); >> + virt = get_field(env->mstatus, MSTATUS_MPV) && >> + (mode != PRV_M); > > This change makes more sense in patch 2 where you also removed the 'mode' > check for MPRV. As it is now I read the code above and thought "but mode > is guaranteed to be == PRV_M, so (mode != PRV_M) is guaranteed to be > false every time". No, this 'mode' (get from MPP) is not the previous 'mode'(the current privilege mode). Regards, Weiwei Li > > The change in helper_mret() below is fine. > > Thanks, > > Daniel > >> if (virt) { >> status = env->vsstatus; >> } >> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c >> index f563dc3981..9cdb9cdd06 100644 >> --- a/target/riscv/op_helper.c >> +++ b/target/riscv/op_helper.c >> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) >> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, >> GETPC()); >> } >> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); >> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && >> + (prev_priv != PRV_M); >> mstatus = set_field(mstatus, MSTATUS_MIE, >> get_field(mstatus, MSTATUS_MPIE)); >> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
On 5/30/23 21:27, Weiwei Li wrote: > > On 2023/5/31 04:23, Daniel Henrique Barboza wrote: >> >> >> On 5/29/23 09:17, Weiwei Li wrote: >>> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored >>> when MPP=PRV_M. >>> >>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>> --- >>> target/riscv/cpu_helper.c | 3 ++- >>> target/riscv/op_helper.c | 3 ++- >>> 2 files changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>> index 09ea227ceb..bd892c05d4 100644 >>> --- a/target/riscv/cpu_helper.c >>> +++ b/target/riscv/cpu_helper.c >>> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) >>> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { >>> mode = get_field(env->mstatus, MSTATUS_MPP); >>> - virt = get_field(env->mstatus, MSTATUS_MPV); >>> + virt = get_field(env->mstatus, MSTATUS_MPV) && >>> + (mode != PRV_M); >> >> This change makes more sense in patch 2 where you also removed the 'mode' >> check for MPRV. As it is now I read the code above and thought "but mode >> is guaranteed to be == PRV_M, so (mode != PRV_M) is guaranteed to be >> false every time". > > No, this 'mode' (get from MPP) is not the previous 'mode'(the current privilege mode). Oh, right: >>> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { >>> mode = get_field(env->mstatus, MSTATUS_MPP); 'mode' is being reassigned here. > > Regards, > > Weiwei Li > >> >> The change in helper_mret() below is fine. >> >> Thanks, >> >> Daniel >> >>> if (virt) { >>> status = env->vsstatus; >>> } >>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c >>> index f563dc3981..9cdb9cdd06 100644 >>> --- a/target/riscv/op_helper.c >>> +++ b/target/riscv/op_helper.c >>> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) >>> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC()); >>> } >>> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); >>> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && >>> + (prev_priv != PRV_M); >>> mstatus = set_field(mstatus, MSTATUS_MIE, >>> get_field(mstatus, MSTATUS_MPIE)); >>> mstatus = set_field(mstatus, MSTATUS_MPIE, 1); >
On 5/29/23 09:17, Weiwei Li wrote: > Upon MRET or explicit memory access with MPRV=1, MPV should be ignored > when MPP=PRV_M. > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > target/riscv/cpu_helper.c | 3 ++- > target/riscv/op_helper.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 09ea227ceb..bd892c05d4 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > > if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { > mode = get_field(env->mstatus, MSTATUS_MPP); > - virt = get_field(env->mstatus, MSTATUS_MPV); > + virt = get_field(env->mstatus, MSTATUS_MPV) && > + (mode != PRV_M); > if (virt) { > status = env->vsstatus; > } > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index f563dc3981..9cdb9cdd06 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) > riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC()); > } > > - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); > + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && > + (prev_priv != PRV_M); > mstatus = set_field(mstatus, MSTATUS_MIE, > get_field(mstatus, MSTATUS_MPIE)); > mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > > Upon MRET or explicit memory access with MPRV=1, MPV should be ignored > when MPP=PRV_M. > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu_helper.c | 3 ++- > target/riscv/op_helper.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 09ea227ceb..bd892c05d4 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > > if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { > mode = get_field(env->mstatus, MSTATUS_MPP); > - virt = get_field(env->mstatus, MSTATUS_MPV); > + virt = get_field(env->mstatus, MSTATUS_MPV) && > + (mode != PRV_M); > if (virt) { > status = env->vsstatus; > } > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index f563dc3981..9cdb9cdd06 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) > riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC()); > } > > - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); > + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && > + (prev_priv != PRV_M); > mstatus = set_field(mstatus, MSTATUS_MIE, > get_field(mstatus, MSTATUS_MPIE)); > mstatus = set_field(mstatus, MSTATUS_MPIE, 1); > -- > 2.25.1 > >
On 2023/5/29 20:17, Weiwei Li wrote: > Upon MRET or explicit memory access with MPRV=1, MPV should be ignored > when MPP=PRV_M. Does MPP==PRV_M always indicate the MPV==0? Zhiwei > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- > target/riscv/cpu_helper.c | 3 ++- > target/riscv/op_helper.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 09ea227ceb..bd892c05d4 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > > if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { > mode = get_field(env->mstatus, MSTATUS_MPP); > - virt = get_field(env->mstatus, MSTATUS_MPV); > + virt = get_field(env->mstatus, MSTATUS_MPV) && > + (mode != PRV_M); > if (virt) { > status = env->vsstatus; > } > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c > index f563dc3981..9cdb9cdd06 100644 > --- a/target/riscv/op_helper.c > +++ b/target/riscv/op_helper.c > @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) > riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC()); > } > > - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); > + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && > + (prev_priv != PRV_M); > mstatus = set_field(mstatus, MSTATUS_MIE, > get_field(mstatus, MSTATUS_MPIE)); > mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
On 2023/6/12 10:45, LIU Zhiwei wrote: > > On 2023/5/29 20:17, Weiwei Li wrote: >> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored >> when MPP=PRV_M. > Does MPP==PRV_M always indicate the MPV==0? No, I think . The spec doesn't restrict this. When MPP=PRV_M, MPV wll be 0 in normal case. But users can set MPV=1 by write to mstatus CSR directly. As described in spec, "When an MRET instruction is executed, the virtualization mode V is set to MPV, unless MPP=3, in which case V remains 0." MPV is just ignored if MPP = 3. This also can be seen in "table 9.5 Effect of MPRV on the translation and protection of explicit memory accesses". Regards, Weiwei Li > > Zhiwei > >> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> --- >> target/riscv/cpu_helper.c | 3 ++- >> target/riscv/op_helper.c | 3 ++- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index 09ea227ceb..bd892c05d4 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool >> ifetch) >> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { >> mode = get_field(env->mstatus, MSTATUS_MPP); >> - virt = get_field(env->mstatus, MSTATUS_MPV); >> + virt = get_field(env->mstatus, MSTATUS_MPV) && >> + (mode != PRV_M); >> if (virt) { >> status = env->vsstatus; >> } >> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c >> index f563dc3981..9cdb9cdd06 100644 >> --- a/target/riscv/op_helper.c >> +++ b/target/riscv/op_helper.c >> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) >> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, >> GETPC()); >> } >> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); >> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && >> + (prev_priv != PRV_M); >> mstatus = set_field(mstatus, MSTATUS_MIE, >> get_field(mstatus, MSTATUS_MPIE)); >> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
On 2023/6/12 11:10, Weiwei Li wrote: > > On 2023/6/12 10:45, LIU Zhiwei wrote: >> >> On 2023/5/29 20:17, Weiwei Li wrote: >>> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored >>> when MPP=PRV_M. >> Does MPP==PRV_M always indicate the MPV==0? > > No, I think . The spec doesn't restrict this. When MPP=PRV_M, MPV wll > be 0 in normal case. > > But users can set MPV=1 by write to mstatus CSR directly. Make sense. The fields specification(WARL and others) of mstatus and other CSR always not clear on the specification. Maybe I missed something. But I think this field could be written by the mode software. Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei > > As described in spec, "When an MRET instruction is executed, the > virtualization mode V is set to MPV, unless > > MPP=3, in which case V remains 0." > > MPV is just ignored if MPP = 3. This also can be seen in "table 9.5 > Effect of MPRV on the translation and protection of explicit memory > accesses". > > Regards, > > Weiwei Li > >> >> Zhiwei >> >>> >>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>> --- >>> target/riscv/cpu_helper.c | 3 ++- >>> target/riscv/op_helper.c | 3 ++- >>> 2 files changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>> index 09ea227ceb..bd892c05d4 100644 >>> --- a/target/riscv/cpu_helper.c >>> +++ b/target/riscv/cpu_helper.c >>> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool >>> ifetch) >>> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { >>> mode = get_field(env->mstatus, MSTATUS_MPP); >>> - virt = get_field(env->mstatus, MSTATUS_MPV); >>> + virt = get_field(env->mstatus, MSTATUS_MPV) && >>> + (mode != PRV_M); >>> if (virt) { >>> status = env->vsstatus; >>> } >>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c >>> index f563dc3981..9cdb9cdd06 100644 >>> --- a/target/riscv/op_helper.c >>> +++ b/target/riscv/op_helper.c >>> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) >>> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, >>> GETPC()); >>> } >>> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); >>> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && >>> + (prev_priv != PRV_M); >>> mstatus = set_field(mstatus, MSTATUS_MIE, >>> get_field(mstatus, MSTATUS_MPIE)); >>> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 09ea227ceb..bd892c05d4 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { mode = get_field(env->mstatus, MSTATUS_MPP); - virt = get_field(env->mstatus, MSTATUS_MPV); + virt = get_field(env->mstatus, MSTATUS_MPV) && + (mode != PRV_M); if (virt) { status = env->vsstatus; } diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index f563dc3981..9cdb9cdd06 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env) riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC()); } - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV); + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) && + (prev_priv != PRV_M); mstatus = set_field(mstatus, MSTATUS_MIE, get_field(mstatus, MSTATUS_MPIE)); mstatus = set_field(mstatus, MSTATUS_MPIE, 1);