[PATCH v2 1/2] target/riscv: FIX xATP_MODE validation

[PATCH v2 1/2] target/riscv: FIX xATP_MODE validation

[Irina Ryapolova]

[Changes since v1]
used satp_mode.map instead of satp_mode.supported

[Original cover]
The SATP register is an SXLEN-bit read/write WARL register. It means that CSR fields are only defined
for a subset of bit encodings, but allow any value to be written while guaranteeing to return a legal
value whenever read (See riscv-privileged-20211203, SATP CSR).

For example on rv64 we are trying to write to SATP CSR val = 0x1000000000000000 (SATP_MODE = 1 - Reserved for standard use)
and after that we are trying to read SATP_CSR. We read from the SATP CSR value = 0x1000000000000000, which is not a correct
operation (return illegal value).

Signed-off-by: Irina Ryapolova <irina.ryapolova@syntacore.com>
---
 target/riscv/csr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index fde7ce1a53..735fb27be7 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1278,8 +1278,8 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
 
 static bool validate_vm(CPURISCVState *env, target_ulong vm)
 {
-    return (vm & 0xf) <=
-           satp_mode_max_from_map(riscv_cpu_cfg(env)->satp_mode.map);
+    uint64_t mode_supported = riscv_cpu_cfg(env)->satp_mode.map;
+    return get_field(mode_supported, (1 << vm));
 }
 
 static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,
-- 
2.25.1

[PATCH v2 2/2] target/riscv: UPDATE xATP write CSR

[Irina Ryapolova]

Added xATP_MODE validation for vsatp/hgatp CSRs.
The xATP register is an SXLEN-bit read/write WARL register, so
the legal value must be returned (See riscv-privileged-20211203, SATP/VSATP/HGATP CSRs).

Signed-off-by: Irina Ryapolova <irina.ryapolova@syntacore.com>
---
 target/riscv/csr.c | 52 ++++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 735fb27be7..6d7a3dd9aa 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1282,6 +1282,32 @@ static bool validate_vm(CPURISCVState *env, target_ulong vm)
     return get_field(mode_supported, (1 << vm));
 }
 
+static target_ulong legalize_xatp(CPURISCVState *env, target_ulong old_xatp,
+                                  target_ulong val)
+{
+    target_ulong mask;
+    bool vm;
+    if (riscv_cpu_mxl(env) == MXL_RV32) {
+        vm = validate_vm(env, get_field(val, SATP32_MODE));
+        mask = (val ^ old_xatp) & (SATP32_MODE | SATP32_ASID | SATP32_PPN);
+    } else {
+        vm = validate_vm(env, get_field(val, SATP64_MODE));
+        mask = (val ^ old_xatp) & (SATP64_MODE | SATP64_ASID | SATP64_PPN);
+    }
+
+    if (vm && mask) {
+        /*
+         * The ISA defines SATP.MODE=Bare as "no translation", but we still
+         * pass these through QEMU's TLB emulation as it improves
+         * performance.  Flushing the TLB on SATP writes with paging
+         * enabled avoids leaking those invalid cached mappings.
+         */
+        tlb_flush(env_cpu(env));
+        return val;
+    }
+    return old_xatp;
+}
+
 static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,
                                  target_ulong val)
 {
@@ -2997,31 +3023,11 @@ static RISCVException read_satp(CPURISCVState *env, int csrno,
 static RISCVException write_satp(CPURISCVState *env, int csrno,
                                  target_ulong val)
 {
-    target_ulong mask;
-    bool vm;
-
     if (!riscv_cpu_cfg(env)->mmu) {
         return RISCV_EXCP_NONE;
     }
 
-    if (riscv_cpu_mxl(env) == MXL_RV32) {
-        vm = validate_vm(env, get_field(val, SATP32_MODE));
-        mask = (val ^ env->satp) & (SATP32_MODE | SATP32_ASID | SATP32_PPN);
-    } else {
-        vm = validate_vm(env, get_field(val, SATP64_MODE));
-        mask = (val ^ env->satp) & (SATP64_MODE | SATP64_ASID | SATP64_PPN);
-    }
-
-    if (vm && mask) {
-        /*
-         * The ISA defines SATP.MODE=Bare as "no translation", but we still
-         * pass these through QEMU's TLB emulation as it improves
-         * performance.  Flushing the TLB on SATP writes with paging
-         * enabled avoids leaking those invalid cached mappings.
-         */
-        tlb_flush(env_cpu(env));
-        env->satp = val;
-    }
+    env->satp = legalize_xatp(env, env->satp, val);
     return RISCV_EXCP_NONE;
 }
 
@@ -3506,7 +3512,7 @@ static RISCVException read_hgatp(CPURISCVState *env, int csrno,
 static RISCVException write_hgatp(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
-    env->hgatp = val;
+    env->hgatp = legalize_xatp(env, env->hgatp, val);
     return RISCV_EXCP_NONE;
 }
 
@@ -3772,7 +3778,7 @@ static RISCVException read_vsatp(CPURISCVState *env, int csrno,
 static RISCVException write_vsatp(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
-    env->vsatp = val;
+    env->vsatp = legalize_xatp(env, env->vsatp, val);
     return RISCV_EXCP_NONE;
 }
 
-- 
2.25.1

Re: [PATCH v2 1/2] target/riscv: FIX xATP_MODE validation

[Daniel Henrique Barboza]

Hi,

On 12/12/23 08:03, Irina Ryapolova wrote:
> [Changes since v1]
> used satp_mode.map instead of satp_mode.supported
> 
> [Original cover]
> The SATP register is an SXLEN-bit read/write WARL register. It means that CSR fields are only defined
> for a subset of bit encodings, but allow any value to be written while guaranteeing to return a legal
> value whenever read (See riscv-privileged-20211203, SATP CSR).
> 
> For example on rv64 we are trying to write to SATP CSR val = 0x1000000000000000 (SATP_MODE = 1 - Reserved for standard use)
> and after that we are trying to read SATP_CSR. We read from the SATP CSR value = 0x1000000000000000, which is not a correct
> operation (return illegal value).
> 
> Signed-off-by: Irina Ryapolova <irina.ryapolova@syntacore.com>
> ---


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

>   target/riscv/csr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fde7ce1a53..735fb27be7 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1278,8 +1278,8 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
>   
>   static bool validate_vm(CPURISCVState *env, target_ulong vm)
>   {
> -    return (vm & 0xf) <=
> -           satp_mode_max_from_map(riscv_cpu_cfg(env)->satp_mode.map);
> +    uint64_t mode_supported = riscv_cpu_cfg(env)->satp_mode.map;
> +    return get_field(mode_supported, (1 << vm));
>   }
>   
>   static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,

Re: [PATCH v2 2/2] target/riscv: UPDATE xATP write CSR

[Daniel Henrique Barboza]

On 12/12/23 08:03, Irina Ryapolova wrote:
> Added xATP_MODE validation for vsatp/hgatp CSRs.
> The xATP register is an SXLEN-bit read/write WARL register, so
> the legal value must be returned (See riscv-privileged-20211203, SATP/VSATP/HGATP CSRs).
> 
> Signed-off-by: Irina Ryapolova <irina.ryapolova@syntacore.com>
> ---

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

>   target/riscv/csr.c | 52 ++++++++++++++++++++++++++--------------------
>   1 file changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 735fb27be7..6d7a3dd9aa 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1282,6 +1282,32 @@ static bool validate_vm(CPURISCVState *env, target_ulong vm)
>       return get_field(mode_supported, (1 << vm));
>   }
>   
> +static target_ulong legalize_xatp(CPURISCVState *env, target_ulong old_xatp,
> +                                  target_ulong val)
> +{
> +    target_ulong mask;
> +    bool vm;
> +    if (riscv_cpu_mxl(env) == MXL_RV32) {
> +        vm = validate_vm(env, get_field(val, SATP32_MODE));
> +        mask = (val ^ old_xatp) & (SATP32_MODE | SATP32_ASID | SATP32_PPN);
> +    } else {
> +        vm = validate_vm(env, get_field(val, SATP64_MODE));
> +        mask = (val ^ old_xatp) & (SATP64_MODE | SATP64_ASID | SATP64_PPN);
> +    }
> +
> +    if (vm && mask) {
> +        /*
> +         * The ISA defines SATP.MODE=Bare as "no translation", but we still
> +         * pass these through QEMU's TLB emulation as it improves
> +         * performance.  Flushing the TLB on SATP writes with paging
> +         * enabled avoids leaking those invalid cached mappings.
> +         */
> +        tlb_flush(env_cpu(env));
> +        return val;
> +    }
> +    return old_xatp;
> +}
> +
>   static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,
>                                    target_ulong val)
>   {
> @@ -2997,31 +3023,11 @@ static RISCVException read_satp(CPURISCVState *env, int csrno,
>   static RISCVException write_satp(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>   {
> -    target_ulong mask;
> -    bool vm;
> -
>       if (!riscv_cpu_cfg(env)->mmu) {
>           return RISCV_EXCP_NONE;
>       }
>   
> -    if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        vm = validate_vm(env, get_field(val, SATP32_MODE));
> -        mask = (val ^ env->satp) & (SATP32_MODE | SATP32_ASID | SATP32_PPN);
> -    } else {
> -        vm = validate_vm(env, get_field(val, SATP64_MODE));
> -        mask = (val ^ env->satp) & (SATP64_MODE | SATP64_ASID | SATP64_PPN);
> -    }
> -
> -    if (vm && mask) {
> -        /*
> -         * The ISA defines SATP.MODE=Bare as "no translation", but we still
> -         * pass these through QEMU's TLB emulation as it improves
> -         * performance.  Flushing the TLB on SATP writes with paging
> -         * enabled avoids leaking those invalid cached mappings.
> -         */
> -        tlb_flush(env_cpu(env));
> -        env->satp = val;
> -    }
> +    env->satp = legalize_xatp(env, env->satp, val);
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -3506,7 +3512,7 @@ static RISCVException read_hgatp(CPURISCVState *env, int csrno,
>   static RISCVException write_hgatp(CPURISCVState *env, int csrno,
>                                     target_ulong val)
>   {
> -    env->hgatp = val;
> +    env->hgatp = legalize_xatp(env, env->hgatp, val);
>       return RISCV_EXCP_NONE;
>   }
>   
> @@ -3772,7 +3778,7 @@ static RISCVException read_vsatp(CPURISCVState *env, int csrno,
>   static RISCVException write_vsatp(CPURISCVState *env, int csrno,
>                                     target_ulong val)
>   {
> -    env->vsatp = val;
> +    env->vsatp = legalize_xatp(env, env->vsatp, val);
>       return RISCV_EXCP_NONE;
>   }
>   

Re: [PATCH v2 1/2] target/riscv: FIX xATP_MODE validation

[Alistair Francis]

On Tue, Dec 12, 2023 at 9:04 PM Irina Ryapolova
<irina.ryapolova@syntacore.com> wrote:
>
> [Changes since v1]
> used satp_mode.map instead of satp_mode.supported

The changelog needs to go

>
> [Original cover]
> The SATP register is an SXLEN-bit read/write WARL register. It means that CSR fields are only defined
> for a subset of bit encodings, but allow any value to be written while guaranteeing to return a legal
> value whenever read (See riscv-privileged-20211203, SATP CSR).
>
> For example on rv64 we are trying to write to SATP CSR val = 0x1000000000000000 (SATP_MODE = 1 - Reserved for standard use)
> and after that we are trying to read SATP_CSR. We read from the SATP CSR value = 0x1000000000000000, which is not a correct
> operation (return illegal value).
>
> Signed-off-by: Irina Ryapolova <irina.ryapolova@syntacore.com>
> ---

Below this line.

Otherwise it will be included in the git history, which we don't want

Alistair

>  target/riscv/csr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index fde7ce1a53..735fb27be7 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1278,8 +1278,8 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
>
>  static bool validate_vm(CPURISCVState *env, target_ulong vm)
>  {
> -    return (vm & 0xf) <=
> -           satp_mode_max_from_map(riscv_cpu_cfg(env)->satp_mode.map);
> +    uint64_t mode_supported = riscv_cpu_cfg(env)->satp_mode.map;
> +    return get_field(mode_supported, (1 << vm));
>  }
>
>  static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,
> --
> 2.25.1
>
>