| Message ID | 20230529121719.179507-3-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: > Normally, MPRV can be set to 1 only in M mode (It will be cleared > when returning to lower-privilege mode by MRET/SRET). > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- > target/riscv/cpu_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index bd892c05d4..45baf95c77 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > if (!ifetch) { > uint64_t status = env->mstatus; > > - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { > + if (get_field(status, MSTATUS_MPRV)) { As I mentioned in patch 1 this is a good place to put this change: - virt = get_field(env->mstatus, MSTATUS_MPV); + virt = get_field(env->mstatus, MSTATUS_MPV) && + (mode != PRV_M); if (virt) { status = env->vsstatus; Thanks, Daniel > mode = get_field(env->mstatus, MSTATUS_MPP); > virt = get_field(env->mstatus, MSTATUS_MPV) && > (mode != PRV_M);
On 5/29/23 09:17, Weiwei Li wrote: > Normally, MPRV can be set to 1 only in M mode (It will be cleared > when returning to lower-privilege mode by MRET/SRET). > > 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index bd892c05d4..45baf95c77 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > if (!ifetch) { > uint64_t status = env->mstatus; > > - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { > + if (get_field(status, MSTATUS_MPRV)) { > mode = get_field(env->mstatus, MSTATUS_MPP); > virt = get_field(env->mstatus, MSTATUS_MPV) && > (mode != PRV_M);
On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > > Normally, MPRV can be set to 1 only in M mode (It will be cleared > when returning to lower-privilege mode by MRET/SRET). > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- > target/riscv/cpu_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index bd892c05d4..45baf95c77 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > if (!ifetch) { > uint64_t status = env->mstatus; > > - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { > + if (get_field(status, MSTATUS_MPRV)) { The original check is correct though, why remove it? Alistair > mode = get_field(env->mstatus, MSTATUS_MPP); > virt = get_field(env->mstatus, MSTATUS_MPV) && > (mode != PRV_M); > -- > 2.25.1 > >
On 2023/6/1 13:27, Alistair Francis wrote: > On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >> Normally, MPRV can be set to 1 only in M mode (It will be cleared >> when returning to lower-privilege mode by MRET/SRET). >> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> --- >> target/riscv/cpu_helper.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index bd892c05d4..45baf95c77 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) >> if (!ifetch) { >> uint64_t status = env->mstatus; >> >> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { >> + if (get_field(status, MSTATUS_MPRV)) { > The original check is correct though, why remove it? Yeah. As described in the commit message, I think MPRV can only be set to 1 in M mode normally which means check on MPRV is enough in this case. So I remove the check on mode here. Regards, Weiwei Li > > Alistair > >> mode = get_field(env->mstatus, MSTATUS_MPP); >> virt = get_field(env->mstatus, MSTATUS_MPV) && >> (mode != PRV_M); >> -- >> 2.25.1 >> >>
On Thu, Jun 1, 2023 at 4:43 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > > > On 2023/6/1 13:27, Alistair Francis wrote: > > On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > >> Normally, MPRV can be set to 1 only in M mode (It will be cleared > >> when returning to lower-privilege mode by MRET/SRET). > >> > >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > >> --- > >> target/riscv/cpu_helper.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > >> index bd892c05d4..45baf95c77 100644 > >> --- a/target/riscv/cpu_helper.c > >> +++ b/target/riscv/cpu_helper.c > >> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > >> if (!ifetch) { > >> uint64_t status = env->mstatus; > >> > >> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { > >> + if (get_field(status, MSTATUS_MPRV)) { > > The original check is correct though, why remove it? > > Yeah. As described in the commit message, I think MPRV can only be set > to 1 in M mode normally That's true. I do feel that keeping the check makes the code easier to follow. Otherwise future developers need to check to see how MPRV can be set. The current code is explicit and obviously follows the spec. For a performance gain I think it's worth making the trade off, but it doesn't sound like we really get any gain here. Alistair > > which means check on MPRV is enough in this case. So I remove the check > on mode here. > > Regards, > > Weiwei Li > > > > > Alistair > > > >> mode = get_field(env->mstatus, MSTATUS_MPP); > >> virt = get_field(env->mstatus, MSTATUS_MPV) && > >> (mode != PRV_M); > >> -- > >> 2.25.1 > >> > >> >
On 2023/6/2 07:03, Alistair Francis wrote: > On Thu, Jun 1, 2023 at 4:43 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >> >> On 2023/6/1 13:27, Alistair Francis wrote: >>> On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >>>> Normally, MPRV can be set to 1 only in M mode (It will be cleared >>>> when returning to lower-privilege mode by MRET/SRET). >>>> >>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>>> --- >>>> target/riscv/cpu_helper.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>>> index bd892c05d4..45baf95c77 100644 >>>> --- a/target/riscv/cpu_helper.c >>>> +++ b/target/riscv/cpu_helper.c >>>> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) >>>> if (!ifetch) { >>>> uint64_t status = env->mstatus; >>>> >>>> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { >>>> + if (get_field(status, MSTATUS_MPRV)) { >>> The original check is correct though, why remove it? >> Yeah. As described in the commit message, I think MPRV can only be set >> to 1 in M mode normally > That's true. I do feel that keeping the check makes the code easier to > follow. Otherwise future developers need to check to see how MPRV can > be set. The current code is explicit and obviously follows the spec. > > For a performance gain I think it's worth making the trade off, but it > doesn't sound like we really get any gain here. Yeah. It's acceptable to me. Just another question: whether MPRV is truly limited to work on M mode? I can only find following description in the note: "The MPRV and MXR mechanisms *were* conceived to improve the efficiency of M-mode routines that emulate missing hardware features, e.g., misaligned loads and stores." To some degree, It seems not limit them to work on other mode. Even though MPRV normally can be set to 1 in M mode, it seems possible to set it to 1 in other mode by gdbstub. Regards, Weiwei Li > Alistair > >> which means check on MPRV is enough in this case. So I remove the check >> on mode here. >> >> Regards, >> >> Weiwei Li >> >>> Alistair >>> >>>> mode = get_field(env->mstatus, MSTATUS_MPP); >>>> virt = get_field(env->mstatus, MSTATUS_MPV) && >>>> (mode != PRV_M); >>>> -- >>>> 2.25.1 >>>> >>>>
On 6/1/23 18:31, Weiwei Li wrote: > Even though MPRV normally can be set to 1 in M mode, it seems possible to set it to 1 in > other mode by gdbstub. That would seem to be a gdbstub bug, since it is cleared on exit from M-mode, and cannot be set again until we re-enter M-mode. r~
On 2023/6/3 05:01, Richard Henderson wrote: > On 6/1/23 18:31, Weiwei Li wrote: >> Even though MPRV normally can be set to 1 in M mode, it seems >> possible to set it to 1 in other mode by gdbstub. > > That would seem to be a gdbstub bug, since it is cleared on exit from > M-mode, and cannot be set again until we re-enter M-mode. Yeah, MPRV normally can be set only in M mode. However, gdbstub can bypass the check on privilege mode for CSRs access inriscv_csrrw_check. Regards, Weiwei Li > > > r~
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index bd892c05d4..45baf95c77 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) if (!ifetch) { uint64_t status = env->mstatus; - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) { + if (get_field(status, MSTATUS_MPRV)) { mode = get_field(env->mstatus, MSTATUS_MPP); virt = get_field(env->mstatus, MSTATUS_MPV) && (mode != PRV_M);