1. Patchew
  2. QEMU
  3. [PATCH v6 0/9] target/riscv: Add support for Smdbltrp and Ssdbltrp extensions
  4. View patch

[PATCH v6 1/9] target/riscv: fix henvcfg potentially containing stale bits

Clément Léger posted 9 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH v6 1/9] target/riscv: fix henvcfg potentially containing stale bits
Posted by Clément Léger 1 year, 2 months ago 
With the current implementation, if we had the following scenario:
- Set bit x in menvcfg
- Set bit x in henvcfg
- Clear bit x in menvcfg
then, the internal variable env->henvcfg would still contain bit x due
to both a wrong menvcfg mask used in write_henvcfg() as well as a
missing update of henvcfg upon menvcfg update.
This can lead to some wrong interpretation of the context. In order to
update henvcfg upon menvcfg writing, call write_henvcfg() after writing
menvcfg. Clearing henvcfg upon writing the new value is also needed in
write_henvcfg() as well as clearing henvcfg upper part when writing it
with write_henvcfgh().

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/csr.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 5d8d0d7514..98c683df60 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2383,6 +2383,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
+                                    target_ulong val);
 static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
@@ -2403,6 +2405,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
         }
     }
     env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
+    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
 
     return RISCV_EXCP_NONE;
 }
@@ -2414,6 +2417,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
+                                    target_ulong val);
 static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
                                      target_ulong val)
 {
@@ -2424,6 +2429,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
     uint64_t valh = (uint64_t)val << 32;
 
     env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
+    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
 
     return RISCV_EXCP_NONE;
 }
@@ -2513,7 +2519,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
         }
     }
 
-    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
+    env->henvcfg = val & mask;
 
     return RISCV_EXCP_NONE;
 }
@@ -2546,7 +2552,7 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
         return ret;
     }
 
-    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
+    env->henvcfg = (env->henvcfg & 0xFFFFFFFF) | (valh & mask);
     return RISCV_EXCP_NONE;
 }
 
-- 
2.45.2
Re: [PATCH v6 1/9] target/riscv: fix henvcfg potentially containing stale bits
Posted by Alistair Francis 1 year, 1 month ago 
On Fri, Nov 29, 2024 at 12:15 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> With the current implementation, if we had the following scenario:
> - Set bit x in menvcfg
> - Set bit x in henvcfg
> - Clear bit x in menvcfg
> then, the internal variable env->henvcfg would still contain bit x due
> to both a wrong menvcfg mask used in write_henvcfg() as well as a
> missing update of henvcfg upon menvcfg update.
> This can lead to some wrong interpretation of the context. In order to
> update henvcfg upon menvcfg writing, call write_henvcfg() after writing
> menvcfg. Clearing henvcfg upon writing the new value is also needed in
> write_henvcfg() as well as clearing henvcfg upper part when writing it
> with write_henvcfgh().
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 5d8d0d7514..98c683df60 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2383,6 +2383,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>
> +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> +                                    target_ulong val);
>  static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>                                      target_ulong val)
>  {
> @@ -2403,6 +2405,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>          }
>      }
>      env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
> +    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
>
>      return RISCV_EXCP_NONE;
>  }
> @@ -2414,6 +2417,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>
> +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> +                                    target_ulong val);
>  static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>                                       target_ulong val)
>  {
> @@ -2424,6 +2429,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>      uint64_t valh = (uint64_t)val << 32;
>
>      env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> +    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
>
>      return RISCV_EXCP_NONE;
>  }
> @@ -2513,7 +2519,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>          }
>      }
>
> -    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> +    env->henvcfg = val & mask;
>
>      return RISCV_EXCP_NONE;
>  }
> @@ -2546,7 +2552,7 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>          return ret;
>      }
>
> -    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> +    env->henvcfg = (env->henvcfg & 0xFFFFFFFF) | (valh & mask);
>      return RISCV_EXCP_NONE;
>  }
>
> --
> 2.45.2
>
>
Re: [PATCH v6 1/9] target/riscv: fix henvcfg potentially containing stale bits
Posted by Daniel Henrique Barboza 1 year, 2 months ago 

On 11/28/24 11:12 AM, Clément Léger wrote:
> With the current implementation, if we had the following scenario:
> - Set bit x in menvcfg
> - Set bit x in henvcfg
> - Clear bit x in menvcfg
> then, the internal variable env->henvcfg would still contain bit x due
> to both a wrong menvcfg mask used in write_henvcfg() as well as a
> missing update of henvcfg upon menvcfg update.
> This can lead to some wrong interpretation of the context. In order to
> update henvcfg upon menvcfg writing, call write_henvcfg() after writing
> menvcfg. Clearing henvcfg upon writing the new value is also needed in
> write_henvcfg() as well as clearing henvcfg upper part when writing it
> with write_henvcfgh().
> 
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>   target/riscv/csr.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 5d8d0d7514..98c683df60 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2383,6 +2383,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> +                                    target_ulong val);
>   static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>                                       target_ulong val)
>   {
> @@ -2403,6 +2405,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>           }
>       }
>       env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
> +    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
>   
>       return RISCV_EXCP_NONE;
>   }
> @@ -2414,6 +2417,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> +                                    target_ulong val);
>   static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>                                        target_ulong val)
>   {
> @@ -2424,6 +2429,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>       uint64_t valh = (uint64_t)val << 32;
>   
>       env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> +    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
>   
>       return RISCV_EXCP_NONE;
>   }
> @@ -2513,7 +2519,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>           }
>       }
>   
> -    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> +    env->henvcfg = val & mask;
>   
>       return RISCV_EXCP_NONE;
>   }
> @@ -2546,7 +2552,7 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>           return ret;
>       }
>   
> -    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
> +    env->henvcfg = (env->henvcfg & 0xFFFFFFFF) | (valh & mask);
>       return RISCV_EXCP_NONE;
>   }
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.