1. Patchew
  2. QEMU
  3. [PATCH 0/4] target/riscv: Fix mstatus related problems
  4. View patch

[PATCH 2/4] target/riscv: Remove check on mode for MPRV

Weiwei Li posted 4 patches 2 years, 8 months ago
There is a newer version of this series
[PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Weiwei Li 2 years, 8 months ago 
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)) {
             mode = get_field(env->mstatus, MSTATUS_MPP);
             virt = get_field(env->mstatus, MSTATUS_MPV) &&
                    (mode != PRV_M);
-- 
2.25.1
Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Alistair Francis 2 years, 8 months ago 
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
>
>
Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Weiwei Li 2 years, 8 months ago 
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
>>
>>
Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Alistair Francis 2 years, 8 months ago 
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
> >>
> >>
>
Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Weiwei Li 2 years, 8 months ago 
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
>>>>
>>>>
Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Richard Henderson 2 years, 8 months ago 
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~
Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Weiwei Li 2 years, 8 months ago 
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~
Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Daniel Henrique Barboza 2 years, 8 months ago 

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);
Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
Posted by Daniel Henrique Barboza 2 years, 8 months ago 

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);

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.