[v2] target/riscv: Smepmp fixes to match specification

[PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode

Posted by Loïc Lefort 2 weeks, 6 days ago

With Machine Mode Lockdown (mseccfg.MML) set and RLB not set, checks on pmpcfg
writes would match the wrong cases of Smepmp truth table.

The existing code allows writes for the following cases:
- L=1, X=0: cases 8, 10, 12, 14
- L=0, RWX!=WX: cases 0-2, 4-6
This leaves cases 3, 7, 9, 11, 13, 15 for which writes are ignored.

From the Smepmp specification: "Adding a rule with executable privileges that
either is M-mode-only or a locked Shared-Region is not possible (...)" This
description matches cases 9-11, 13 of the truth table.

This commit implements an explicit check for these cases by using
pmp_get_epmp_operation to convert between PMP configuration and Smepmp truth
table cases.

Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/pmp.c | 79 +++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 7d65dc24a5..c5f6cdaccb 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -75,6 +75,44 @@ static int pmp_is_readonly(CPURISCVState *env, uint32_t pmp_index)
     return pmp_is_locked(env, pmp_index) && !MSECCFG_RLB_ISSET(env);
 }
 
+/*
+ * Check whether `val` is an invalid Smepmp config value
+ */
+static int pmp_is_invalid_smepmp_cfg(CPURISCVState *env, uint8_t val)
+{
+    /* No check if mseccfg.MML is not set or if mseccfg.RLB is set */
+    if (!MSECCFG_MML_ISSET(env) || MSECCFG_RLB_ISSET(env)) {
+        return 0;
+    }
+
+    /*
+     * Adding a rule with executable privileges that either is M-mode-only
+     * or a locked Shared-Region is not possible
+     */
+    switch (pmp_get_smepmp_operation(val)) {
+    case 0:
+    case 1:
+    case 2:
+    case 3:
+    case 4:
+    case 5:
+    case 6:
+    case 7:
+    case 8:
+    case 12:
+    case 14:
+    case 15:
+        return 0;
+    case 9:
+    case 10:
+    case 11:
+    case 13:
+        return 1;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 /*
  * Count the number of active rules.
  */
@@ -103,44 +141,13 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
 static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
 {
     if (pmp_index < MAX_RISCV_PMPS) {
-        bool readonly = true;
-
-        if (riscv_cpu_cfg(env)->ext_smepmp) {
-            /* mseccfg.RLB is set */
-            if (MSECCFG_RLB_ISSET(env)) {
-                readonly = false;
-            }
-
-            /* mseccfg.MML is not set */
-            if (!MSECCFG_MML_ISSET(env) && !pmp_is_readonly(env, pmp_index)) {
-                readonly = false;
-            }
-
-            /* mseccfg.MML is set */
-            if (MSECCFG_MML_ISSET(env)) {
-                /* not adding execute bit */
-                if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) != PMP_EXEC) {
-                    readonly = false;
-                }
-                /* shared region and not adding X bit */
-                if ((val & PMP_LOCK) != PMP_LOCK &&
-                    (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
-                    readonly = false;
-                }
-            }
-        } else {
-            readonly = pmp_is_readonly(env, pmp_index);
-        }
-
-        if (readonly) {
+        if (pmp_is_readonly(env, pmp_index)) {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "ignoring pmpcfg write - read only\n");
-        } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
-            /* If !mseccfg.MML then ignore writes with encoding RW=01 */
-            if ((val & PMP_WRITE) && !(val & PMP_READ) &&
-                !MSECCFG_MML_ISSET(env)) {
-                return false;
-            }
+        } else if (pmp_is_invalid_smepmp_cfg(env, val)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "ignoring pmpcfg write - invalid\n");
+        } else {
             env->pmp_state.pmp[pmp_index].cfg_reg = val;
             pmp_update_rule_addr(env, pmp_index);
             return true;
-- 
2.47.2

Re: [PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode

Posted by LIU Zhiwei 5 days, 1 hour ago

On 2025/3/14 03:30, Loïc Lefort wrote:
> With Machine Mode Lockdown (mseccfg.MML) set and RLB not set, checks on pmpcfg
> writes would match the wrong cases of Smepmp truth table.
>
> The existing code allows writes for the following cases:
> - L=1, X=0: cases 8, 10, 12, 14
> - L=0, RWX!=WX: cases 0-2, 4-6
> This leaves cases 3, 7, 9, 11, 13, 15 for which writes are ignored.
Good catch.
>
>  From the Smepmp specification: "Adding a rule with executable privileges that
> either is M-mode-only or a locked Shared-Region is not possible (...)" This
> description matches cases 9-11, 13 of the truth table.
>
> This commit implements an explicit check for these cases by using
> pmp_get_epmp_operation to convert between PMP configuration and Smepmp truth
> table cases.
>
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/pmp.c | 79 +++++++++++++++++++++++++---------------------
>   1 file changed, 43 insertions(+), 36 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 7d65dc24a5..c5f6cdaccb 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -75,6 +75,44 @@ static int pmp_is_readonly(CPURISCVState *env, uint32_t pmp_index)
>       return pmp_is_locked(env, pmp_index) && !MSECCFG_RLB_ISSET(env);
>   }
>   
> +/*
> + * Check whether `val` is an invalid Smepmp config value
> + */
> +static int pmp_is_invalid_smepmp_cfg(CPURISCVState *env, uint8_t val)
> +{
> +    /* No check if mseccfg.MML is not set or if mseccfg.RLB is set */
> +    if (!MSECCFG_MML_ISSET(env) || MSECCFG_RLB_ISSET(env)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Adding a rule with executable privileges that either is M-mode-only
> +     * or a locked Shared-Region is not possible
> +     */
> +    switch (pmp_get_smepmp_operation(val)) {
> +    case 0:
> +    case 1:
> +    case 2:
> +    case 3:
> +    case 4:
> +    case 5:
> +    case 6:
> +    case 7:
> +    case 8:
> +    case 12:
> +    case 14:
> +    case 15:
> +        return 0;
> +    case 9:
> +    case 10:
> +    case 11:
> +    case 13:
> +        return 1;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}

I think we should define a truth table for smepmp when mml is 1.

One reason is we can avoid the magic numbers here in the switch case. 
Another reason is we have already used this truth table twice in 
pmp_hart_has_privs.


Otherwise,
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

> +
>   /*
>    * Count the number of active rules.
>    */
> @@ -103,44 +141,13 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
>   static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>   {
>       if (pmp_index < MAX_RISCV_PMPS) {
> -        bool readonly = true;
> -
> -        if (riscv_cpu_cfg(env)->ext_smepmp) {
> -            /* mseccfg.RLB is set */
> -            if (MSECCFG_RLB_ISSET(env)) {
> -                readonly = false;
> -            }
> -
> -            /* mseccfg.MML is not set */
> -            if (!MSECCFG_MML_ISSET(env) && !pmp_is_readonly(env, pmp_index)) {
> -                readonly = false;
> -            }
> -
> -            /* mseccfg.MML is set */
> -            if (MSECCFG_MML_ISSET(env)) {
> -                /* not adding execute bit */
> -                if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) != PMP_EXEC) {
> -                    readonly = false;
> -                }
> -                /* shared region and not adding X bit */
> -                if ((val & PMP_LOCK) != PMP_LOCK &&
> -                    (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
> -                    readonly = false;
> -                }
> -            }
> -        } else {
> -            readonly = pmp_is_readonly(env, pmp_index);
> -        }
> -
> -        if (readonly) {
> +        if (pmp_is_readonly(env, pmp_index)) {
>               qemu_log_mask(LOG_GUEST_ERROR,
>                             "ignoring pmpcfg write - read only\n");
> -        } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
> -            /* If !mseccfg.MML then ignore writes with encoding RW=01 */
> -            if ((val & PMP_WRITE) && !(val & PMP_READ) &&
> -                !MSECCFG_MML_ISSET(env)) {
> -                return false;
> -            }
> +        } else if (pmp_is_invalid_smepmp_cfg(env, val)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "ignoring pmpcfg write - invalid\n");
> +        } else {
>               env->pmp_state.pmp[pmp_index].cfg_reg = val;
>               pmp_update_rule_addr(env, pmp_index);
>               return true;

[PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges
[PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function
[PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode
[PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed
[PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked