diff mbox series

[2/6] target/riscv: Fix the relationship of PBMTE/STCE fields between menvcfg and henvcfg

Message ID 20230224040852.37109-3-liweiwei@iscas.ac.cn
State New
Headers show
Series target/riscv: Add support for Svadu extension | expand

Commit Message

Weiwei Li Feb. 24, 2023, 4:08 a.m. UTC 
henvcfg.PBMTE/STCE are read-only zero if menvcfg.PBMTE/STCE are zero.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/csr.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Daniel Henrique Barboza Feb. 24, 2023, 11:44 a.m. UTC | #1 
On 2/24/23 01:08, Weiwei Li wrote:
> henvcfg.PBMTE/STCE are read-only zero if menvcfg.PBMTE/STCE are zero.
> 
> 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/csr.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index feae23cab0..02cb2c2bb7 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1956,7 +1956,11 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>           return ret;
>       }
>   
> -    *val = env->henvcfg;
> +    /*
> +     * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
> +     * henvcfg.stce is read_only 0 when menvcfg.stce = 0
> +     */
> +    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) | env->menvcfg);
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -1972,7 +1976,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>       }
>   
>       if (riscv_cpu_mxl(env) == MXL_RV64) {
> -        mask |= HENVCFG_PBMTE | HENVCFG_STCE;
> +        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
>       }
>   
>       env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> @@ -1990,14 +1994,15 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>           return ret;
>       }
>   
> -    *val = env->henvcfg >> 32;
> +    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) |
> +                            env->menvcfg)) >> 32;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>                                     target_ulong val)
>   {
> -    uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
> +    uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
>       uint64_t valh = (uint64_t)val << 32;
>       RISCVException ret;
>
Andrew Jones Feb. 24, 2023, 12:19 p.m. UTC | #2 
On Fri, Feb 24, 2023 at 12:08:48PM +0800, Weiwei Li wrote:
> henvcfg.PBMTE/STCE are read-only zero if menvcfg.PBMTE/STCE are zero.
> 
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>  target/riscv/csr.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index feae23cab0..02cb2c2bb7 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1956,7 +1956,11 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>          return ret;
>      }
>  
> -    *val = env->henvcfg;
> +    /*
> +     * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
> +     * henvcfg.stce is read_only 0 when menvcfg.stce = 0
> +     */
> +    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) | env->menvcfg);
>      return RISCV_EXCP_NONE;
>  }
>  
> @@ -1972,7 +1976,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>      }
>  
>      if (riscv_cpu_mxl(env) == MXL_RV64) {
> -        mask |= HENVCFG_PBMTE | HENVCFG_STCE;
> +        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);

nit:

  While HENVCFG_PBMTE == MENVCFG_PBMTE, I'd prefer we use
  MENVCFG_* with menvcfg and HENVCFG_* with henvcfg.

>      }
>  
>      env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> @@ -1990,14 +1994,15 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>          return ret;
>      }
>  
> -    *val = env->henvcfg >> 32;
> +    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) |
> +                            env->menvcfg)) >> 32;
>      return RISCV_EXCP_NONE;
>  }
>  
>  static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>  {
> -    uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
> +    uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
>      uint64_t valh = (uint64_t)val << 32;
>      RISCVException ret;
>  
> -- 
> 2.25.1
> 
> 

Thanks,
drew
Weiwei Li Feb. 24, 2023, 12:36 p.m. UTC | #3 
On 2023/2/24 20:19, Andrew Jones wrote:
> On Fri, Feb 24, 2023 at 12:08:48PM +0800, Weiwei Li wrote:
>> henvcfg.PBMTE/STCE are read-only zero if menvcfg.PBMTE/STCE are zero.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>>   target/riscv/csr.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index feae23cab0..02cb2c2bb7 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1956,7 +1956,11 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>>           return ret;
>>       }
>>   
>> -    *val = env->henvcfg;
>> +    /*
>> +     * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
>> +     * henvcfg.stce is read_only 0 when menvcfg.stce = 0
>> +     */
>> +    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) | env->menvcfg);
>>       return RISCV_EXCP_NONE;
>>   }
>>   
>> @@ -1972,7 +1976,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>       }
>>   
>>       if (riscv_cpu_mxl(env) == MXL_RV64) {
>> -        mask |= HENVCFG_PBMTE | HENVCFG_STCE;
>> +        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
> nit:
>
>    While HENVCFG_PBMTE == MENVCFG_PBMTE, I'd prefer we use
>    MENVCFG_* with menvcfg and HENVCFG_* with henvcfg.

Yeah. I agree. However, I think this mask is finally used for henvcfg. 
We just use menvcfg to mask the  bits

when the same bits are zero. So I didn't modify HENVCFG_* here.

Regards,

Weiwei Li

>
>>       }
>>   
>>       env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
>> @@ -1990,14 +1994,15 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>>           return ret;
>>       }
>>   
>> -    *val = env->henvcfg >> 32;
>> +    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) |
>> +                            env->menvcfg)) >> 32;
>>       return RISCV_EXCP_NONE;
>>   }
>>   
>>   static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>>                                     target_ulong val)
>>   {
>> -    uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
>> +    uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
>>       uint64_t valh = (uint64_t)val << 32;
>>       RISCVException ret;
>>   
>> -- 
>> 2.25.1
>>
>>
> Thanks,
> drew
Palmer Dabbelt March 2, 2023, 1:36 a.m. UTC | #4 
On Fri, 24 Feb 2023 04:36:43 PST (-0800), liweiwei@iscas.ac.cn wrote:
>
> On 2023/2/24 20:19, Andrew Jones wrote:
>> On Fri, Feb 24, 2023 at 12:08:48PM +0800, Weiwei Li wrote:
>>> henvcfg.PBMTE/STCE are read-only zero if menvcfg.PBMTE/STCE are zero.
>>>
>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>> ---
>>>   target/riscv/csr.c | 13 +++++++++----
>>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index feae23cab0..02cb2c2bb7 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -1956,7 +1956,11 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
>>>           return ret;
>>>       }
>>>
>>> -    *val = env->henvcfg;
>>> +    /*
>>> +     * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
>>> +     * henvcfg.stce is read_only 0 when menvcfg.stce = 0
>>> +     */
>>> +    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) | env->menvcfg);
>>>       return RISCV_EXCP_NONE;
>>>   }
>>>
>>> @@ -1972,7 +1976,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>>       }
>>>
>>>       if (riscv_cpu_mxl(env) == MXL_RV64) {
>>> -        mask |= HENVCFG_PBMTE | HENVCFG_STCE;
>>> +        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
>> nit:
>>
>>    While HENVCFG_PBMTE == MENVCFG_PBMTE, I'd prefer we use
>>    MENVCFG_* with menvcfg and HENVCFG_* with henvcfg.
>
> Yeah. I agree. However, I think this mask is finally used for henvcfg.
> We just use menvcfg to mask the  bits
>
> when the same bits are zero. So I didn't modify HENVCFG_* here.

I guess it's kind of bikeshedding because the bits are the same, but 
what's in the patch seems cleaner to me: we're writing the H state 
masked by the M state, so we should use the H definitions (even if it 
doesn't matter).

>
> Regards,
>
> Weiwei Li
>
>>
>>>       }
>>>
>>>       env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
>>> @@ -1990,14 +1994,15 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
>>>           return ret;
>>>       }
>>>
>>> -    *val = env->henvcfg >> 32;
>>> +    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) |
>>> +                            env->menvcfg)) >> 32;
>>>       return RISCV_EXCP_NONE;
>>>   }
>>>
>>>   static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>>>                                     target_ulong val)
>>>   {
>>> -    uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
>>> +    uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
>>>       uint64_t valh = (uint64_t)val << 32;
>>>       RISCVException ret;
>>>
>>> --
>>> 2.25.1
>>>
>>>
>> Thanks,
>> drew
diff mbox series

Patch

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index feae23cab0..02cb2c2bb7 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1956,7 +1956,11 @@  static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
         return ret;
     }
 
-    *val = env->henvcfg;
+    /*
+     * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
+     * henvcfg.stce is read_only 0 when menvcfg.stce = 0
+     */
+    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) | env->menvcfg);
     return RISCV_EXCP_NONE;
 }
 
@@ -1972,7 +1976,7 @@  static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
     }
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
-        mask |= HENVCFG_PBMTE | HENVCFG_STCE;
+        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
     }
 
     env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
@@ -1990,14 +1994,15 @@  static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
         return ret;
     }
 
-    *val = env->henvcfg >> 32;
+    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE) |
+                            env->menvcfg)) >> 32;
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
-    uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
+    uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE);
     uint64_t valh = (uint64_t)val << 32;
     RISCVException ret;