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

2023-03-01 Thread Palmer Dabbelt 

On Fri, 24 Feb 2023 04:36:43 PST (-0800), liwei...@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 
Signed-off-by: Junqiang Wang 
---
  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

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

2023-02-24 Thread liweiwei 



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 
Signed-off-by: Junqiang Wang 
---
  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

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

2023-02-24 Thread Andrew Jones 
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 
> Signed-off-by: Junqiang Wang 
> ---
>  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

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

2023-02-24 Thread Daniel Henrique Barboza 




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 
Signed-off-by: Junqiang Wang 
---


Reviewed-by: Daniel Henrique Barboza 


  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;

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

2023-02-23 Thread Weiwei Li 
henvcfg.PBMTE/STCE are read-only zero if menvcfg.PBMTE/STCE are zero.

Signed-off-by: Weiwei Li 
Signed-off-by: Junqiang Wang 
---
 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;
 
-- 
2.25.1