* [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification
@ 2025-03-13 19:30 Loïc Lefort
2025-03-13 19:30 ` [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges Loïc Lefort
` (6 more replies)
0 siblings, 7 replies; 20+ messages in thread
From: Loïc Lefort @ 2025-03-13 19:30 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza, Loïc Lefort
These patches fix Smepmp implementation to make it compliant with the spec.
First patch limits RLB to CSR changes since RLB should not affect privilege
evaluation. Patch 2 extracts some common code into a function (to be used in
patch 3). Patch 3 fixes validation of pmpcfg CSR writes in order to match Smepmp
specification. Patch 4 is a small optimization and last patch is just removing
redundant code.
---
Changes in v2:
- rebased to latest riscv-to-apply.next
- addressed Daniel comments on patch 1
Link to v1:
https://lore.kernel.org/qemu-riscv/20250225160052.39564-1-loic@rivosinc.com/
Loïc Lefort (5):
target/riscv: pmp: don't allow RLB to bypass rule privileges
target/riscv: pmp: move Smepmp operation conversion into a function
target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode
target/riscv: pmp: exit csr writes early if value was not changed
target/riscv: pmp: remove redundant check in pmp_is_locked
target/riscv/pmp.c | 151 +++++++++++++++++++++++++--------------------
1 file changed, 83 insertions(+), 68 deletions(-)
--
2.47.2
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges
2025-03-13 19:30 [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
@ 2025-03-13 19:30 ` Loïc Lefort
2025-03-27 17:30 ` Daniel Henrique Barboza
` (2 more replies)
2025-03-13 19:30 ` [PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function Loïc Lefort
` (5 subsequent siblings)
6 siblings, 3 replies; 20+ messages in thread
From: Loïc Lefort @ 2025-03-13 19:30 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza, Loïc Lefort
When Smepmp is supported, mseccfg.RLB allows bypassing locks when writing CSRs
but should not affect interpretation of actual PMP rules.
This is not the case with the current implementation where pmp_hart_has_privs
calls pmp_is_locked which implements mseccfg.RLB bypass.
This commit implements the correct behavior by removing mseccfg.RLB bypass from
pmp_is_locked.
RLB bypass when writing CSRs is implemented by adding a new pmp_is_readonly
function that calls pmp_is_locked and check mseccfg.RLB. pmp_write_cfg and
pmpaddr_csr_write are changed to use this new function.
Signed-off-by: Loïc Lefort <loic@rivosinc.com>
---
target/riscv/pmp.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index b0841d44f4..e1e5ca589e 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -45,11 +45,6 @@ static inline uint8_t pmp_get_a_field(uint8_t cfg)
*/
static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
{
- /* mseccfg.RLB is set */
- if (MSECCFG_RLB_ISSET(env)) {
- return 0;
- }
-
if (env->pmp_state.pmp[pmp_index].cfg_reg & PMP_LOCK) {
return 1;
}
@@ -62,6 +57,15 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
return 0;
}
+/*
+ * Check whether a PMP is locked for writing or not.
+ * (i.e. has LOCK flag and mseccfg.RLB is unset)
+ */
+static int pmp_is_readonly(CPURISCVState *env, uint32_t pmp_index)
+{
+ return pmp_is_locked(env, pmp_index) && !MSECCFG_RLB_ISSET(env);
+}
+
/*
* Count the number of active rules.
*/
@@ -90,39 +94,38 @@ 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 locked = true;
+ bool readonly = true;
if (riscv_cpu_cfg(env)->ext_smepmp) {
/* mseccfg.RLB is set */
if (MSECCFG_RLB_ISSET(env)) {
- locked = false;
+ readonly = false;
}
/* mseccfg.MML is not set */
- if (!MSECCFG_MML_ISSET(env) && !pmp_is_locked(env, pmp_index)) {
- locked = false;
+ 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) {
- locked = false;
+ readonly = false;
}
/* shared region and not adding X bit */
if ((val & PMP_LOCK) != PMP_LOCK &&
(val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
- locked = false;
+ readonly = false;
}
}
} else {
- if (!pmp_is_locked(env, pmp_index)) {
- locked = false;
- }
+ readonly = pmp_is_readonly(env, pmp_index);
}
- if (locked) {
- qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
+ if (readonly) {
+ 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) &&
@@ -524,14 +527,14 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
uint8_t pmp_cfg = env->pmp_state.pmp[addr_index + 1].cfg_reg;
is_next_cfg_tor = PMP_AMATCH_TOR == pmp_get_a_field(pmp_cfg);
- if (pmp_is_locked(env, addr_index + 1) && is_next_cfg_tor) {
+ if (pmp_is_readonly(env, addr_index + 1) && is_next_cfg_tor) {
qemu_log_mask(LOG_GUEST_ERROR,
- "ignoring pmpaddr write - pmpcfg + 1 locked\n");
+ "ignoring pmpaddr write - pmpcfg+1 read only\n");
return;
}
}
- if (!pmp_is_locked(env, addr_index)) {
+ if (!pmp_is_readonly(env, addr_index)) {
if (env->pmp_state.pmp[addr_index].addr_reg != val) {
env->pmp_state.pmp[addr_index].addr_reg = val;
pmp_update_rule_addr(env, addr_index);
@@ -542,7 +545,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
}
} else {
qemu_log_mask(LOG_GUEST_ERROR,
- "ignoring pmpaddr write - locked\n");
+ "ignoring pmpaddr write - read only\n");
}
} else {
qemu_log_mask(LOG_GUEST_ERROR,
--
2.47.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function
2025-03-13 19:30 [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
2025-03-13 19:30 ` [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges Loïc Lefort
@ 2025-03-13 19:30 ` Loïc Lefort
2025-03-29 8:53 ` LIU Zhiwei
2025-04-04 1:02 ` Alistair Francis
2025-03-13 19:30 ` [PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode Loïc Lefort
` (4 subsequent siblings)
6 siblings, 2 replies; 20+ messages in thread
From: Loïc Lefort @ 2025-03-13 19:30 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza, Loïc Lefort
Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
target/riscv/pmp.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index e1e5ca589e..7d65dc24a5 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -31,6 +31,15 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
uint8_t val);
static uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t addr_index);
+/*
+ * Convert the PMP permissions to match the truth table in the Smepmp spec.
+ */
+static inline uint8_t pmp_get_smepmp_operation(uint8_t cfg)
+{
+ return ((cfg & PMP_LOCK) >> 4) | ((cfg & PMP_READ) << 2) |
+ (cfg & PMP_WRITE) | ((cfg & PMP_EXEC) >> 2);
+}
+
/*
* Accessor method to extract address matching type 'a field' from cfg reg
*/
@@ -355,16 +364,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, hwaddr addr,
const uint8_t a_field =
pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
- /*
- * Convert the PMP permissions to match the truth table in the
- * Smepmp spec.
- */
- const uint8_t smepmp_operation =
- ((env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) >> 4) |
- ((env->pmp_state.pmp[i].cfg_reg & PMP_READ) << 2) |
- (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) |
- ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2);
-
if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
/*
* If the PMP entry is not off and the address is in range,
@@ -383,6 +382,9 @@ bool pmp_hart_has_privs(CPURISCVState *env, hwaddr addr,
/*
* If mseccfg.MML Bit set, do the enhanced pmp priv check
*/
+ const uint8_t smepmp_operation =
+ pmp_get_smepmp_operation(env->pmp_state.pmp[i].cfg_reg);
+
if (mode == PRV_M) {
switch (smepmp_operation) {
case 0:
--
2.47.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode
2025-03-13 19:30 [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
2025-03-13 19:30 ` [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges Loïc Lefort
2025-03-13 19:30 ` [PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function Loïc Lefort
@ 2025-03-13 19:30 ` Loïc Lefort
2025-03-29 9:01 ` LIU Zhiwei
2025-03-13 19:30 ` [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed Loïc Lefort
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Loïc Lefort @ 2025-03-13 19:30 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza, Loïc Lefort
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed
2025-03-13 19:30 [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
` (2 preceding siblings ...)
2025-03-13 19:30 ` [PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode Loïc Lefort
@ 2025-03-13 19:30 ` Loïc Lefort
2025-03-29 9:03 ` LIU Zhiwei
2025-04-04 1:05 ` Alistair Francis
2025-03-13 19:30 ` [PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked Loïc Lefort
` (2 subsequent siblings)
6 siblings, 2 replies; 20+ messages in thread
From: Loïc Lefort @ 2025-03-13 19:30 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza, Loïc Lefort
Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
target/riscv/pmp.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index c5f6cdaccb..845915e0c8 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -141,6 +141,11 @@ 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) {
+ if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
+ /* no change */
+ return false;
+ }
+
if (pmp_is_readonly(env, pmp_index)) {
qemu_log_mask(LOG_GUEST_ERROR,
"ignoring pmpcfg write - read only\n");
@@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
bool is_next_cfg_tor = false;
if (addr_index < MAX_RISCV_PMPS) {
+ if (env->pmp_state.pmp[addr_index].addr_reg == val) {
+ /* no change */
+ return;
+ }
+
/*
* In TOR mode, need to check the lock bit of the next pmp
* (if there is a next).
@@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
}
if (!pmp_is_readonly(env, addr_index)) {
- if (env->pmp_state.pmp[addr_index].addr_reg != val) {
- env->pmp_state.pmp[addr_index].addr_reg = val;
- pmp_update_rule_addr(env, addr_index);
- if (is_next_cfg_tor) {
- pmp_update_rule_addr(env, addr_index + 1);
- }
- tlb_flush(env_cpu(env));
+ env->pmp_state.pmp[addr_index].addr_reg = val;
+ pmp_update_rule_addr(env, addr_index);
+ if (is_next_cfg_tor) {
+ pmp_update_rule_addr(env, addr_index + 1);
}
+ tlb_flush(env_cpu(env));
} else {
qemu_log_mask(LOG_GUEST_ERROR,
"ignoring pmpaddr write - read only\n");
--
2.47.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked
2025-03-13 19:30 [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
` (3 preceding siblings ...)
2025-03-13 19:30 ` [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed Loïc Lefort
@ 2025-03-13 19:30 ` Loïc Lefort
2025-03-29 9:29 ` LIU Zhiwei
2025-04-04 1:06 ` Alistair Francis
2025-03-27 16:47 ` [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
2025-04-04 1:23 ` Alistair Francis
6 siblings, 2 replies; 20+ messages in thread
From: Loïc Lefort @ 2025-03-13 19:30 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza, Loïc Lefort
Remove useless check in pmp_is_locked, the function will return 0 in either
case.
Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
target/riscv/pmp.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 845915e0c8..c685f7f2c5 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -58,11 +58,6 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
return 1;
}
- /* Top PMP has no 'next' to check */
- if ((pmp_index + 1u) >= MAX_RISCV_PMPS) {
- return 0;
- }
-
return 0;
}
--
2.47.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification
2025-03-13 19:30 [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
` (4 preceding siblings ...)
2025-03-13 19:30 ` [PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked Loïc Lefort
@ 2025-03-27 16:47 ` Loïc Lefort
2025-04-04 1:23 ` Alistair Francis
6 siblings, 0 replies; 20+ messages in thread
From: Loïc Lefort @ 2025-03-27 16:47 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza
[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]
Ping
On Thu, Mar 13, 2025 at 8:30 PM Loïc Lefort <loic@rivosinc.com> wrote:
> These patches fix Smepmp implementation to make it compliant with the spec.
>
> First patch limits RLB to CSR changes since RLB should not affect privilege
> evaluation. Patch 2 extracts some common code into a function (to be used
> in
> patch 3). Patch 3 fixes validation of pmpcfg CSR writes in order to match
> Smepmp
> specification. Patch 4 is a small optimization and last patch is just
> removing
> redundant code.
>
> ---
> Changes in v2:
> - rebased to latest riscv-to-apply.next
> - addressed Daniel comments on patch 1
>
> Link to v1:
>
> https://lore.kernel.org/qemu-riscv/20250225160052.39564-1-loic@rivosinc.com/
>
> Loïc Lefort (5):
> target/riscv: pmp: don't allow RLB to bypass rule privileges
> target/riscv: pmp: move Smepmp operation conversion into a function
> target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode
> target/riscv: pmp: exit csr writes early if value was not changed
> target/riscv: pmp: remove redundant check in pmp_is_locked
>
> target/riscv/pmp.c | 151 +++++++++++++++++++++++++--------------------
> 1 file changed, 83 insertions(+), 68 deletions(-)
>
> --
> 2.47.2
>
>
[-- Attachment #2: Type: text/html, Size: 1744 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges
2025-03-13 19:30 ` [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges Loïc Lefort
@ 2025-03-27 17:30 ` Daniel Henrique Barboza
2025-03-29 8:33 ` LIU Zhiwei
2025-04-04 1:01 ` Alistair Francis
2 siblings, 0 replies; 20+ messages in thread
From: Daniel Henrique Barboza @ 2025-03-27 17:30 UTC (permalink / raw)
To: Loïc Lefort, qemu-devel
Cc: Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis
On 3/13/25 4:30 PM, Loïc Lefort wrote:
> When Smepmp is supported, mseccfg.RLB allows bypassing locks when writing CSRs
> but should not affect interpretation of actual PMP rules.
>
> This is not the case with the current implementation where pmp_hart_has_privs
> calls pmp_is_locked which implements mseccfg.RLB bypass.
>
> This commit implements the correct behavior by removing mseccfg.RLB bypass from
> pmp_is_locked.
>
> RLB bypass when writing CSRs is implemented by adding a new pmp_is_readonly
> function that calls pmp_is_locked and check mseccfg.RLB. pmp_write_cfg and
> pmpaddr_csr_write are changed to use this new function.
>
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/pmp.c | 43 +++++++++++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index b0841d44f4..e1e5ca589e 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -45,11 +45,6 @@ static inline uint8_t pmp_get_a_field(uint8_t cfg)
> */
> static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> {
> - /* mseccfg.RLB is set */
> - if (MSECCFG_RLB_ISSET(env)) {
> - return 0;
> - }
> -
> if (env->pmp_state.pmp[pmp_index].cfg_reg & PMP_LOCK) {
> return 1;
> }
> @@ -62,6 +57,15 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> return 0;
> }
>
> +/*
> + * Check whether a PMP is locked for writing or not.
> + * (i.e. has LOCK flag and mseccfg.RLB is unset)
> + */
> +static int pmp_is_readonly(CPURISCVState *env, uint32_t pmp_index)
> +{
> + return pmp_is_locked(env, pmp_index) && !MSECCFG_RLB_ISSET(env);
> +}
> +
> /*
> * Count the number of active rules.
> */
> @@ -90,39 +94,38 @@ 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 locked = true;
> + bool readonly = true;
>
> if (riscv_cpu_cfg(env)->ext_smepmp) {
> /* mseccfg.RLB is set */
> if (MSECCFG_RLB_ISSET(env)) {
> - locked = false;
> + readonly = false;
> }
>
> /* mseccfg.MML is not set */
> - if (!MSECCFG_MML_ISSET(env) && !pmp_is_locked(env, pmp_index)) {
> - locked = false;
> + 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) {
> - locked = false;
> + readonly = false;
> }
> /* shared region and not adding X bit */
> if ((val & PMP_LOCK) != PMP_LOCK &&
> (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
> - locked = false;
> + readonly = false;
> }
> }
> } else {
> - if (!pmp_is_locked(env, pmp_index)) {
> - locked = false;
> - }
> + readonly = pmp_is_readonly(env, pmp_index);
> }
>
> - if (locked) {
> - qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
> + if (readonly) {
> + 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) &&
> @@ -524,14 +527,14 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> uint8_t pmp_cfg = env->pmp_state.pmp[addr_index + 1].cfg_reg;
> is_next_cfg_tor = PMP_AMATCH_TOR == pmp_get_a_field(pmp_cfg);
>
> - if (pmp_is_locked(env, addr_index + 1) && is_next_cfg_tor) {
> + if (pmp_is_readonly(env, addr_index + 1) && is_next_cfg_tor) {
> qemu_log_mask(LOG_GUEST_ERROR,
> - "ignoring pmpaddr write - pmpcfg + 1 locked\n");
> + "ignoring pmpaddr write - pmpcfg+1 read only\n");
> return;
> }
> }
>
> - if (!pmp_is_locked(env, addr_index)) {
> + if (!pmp_is_readonly(env, addr_index)) {
> if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> env->pmp_state.pmp[addr_index].addr_reg = val;
> pmp_update_rule_addr(env, addr_index);
> @@ -542,7 +545,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> - "ignoring pmpaddr write - locked\n");
> + "ignoring pmpaddr write - read only\n");
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges
2025-03-13 19:30 ` [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges Loïc Lefort
2025-03-27 17:30 ` Daniel Henrique Barboza
@ 2025-03-29 8:33 ` LIU Zhiwei
2025-04-04 1:01 ` Alistair Francis
2 siblings, 0 replies; 20+ messages in thread
From: LIU Zhiwei @ 2025-03-29 8:33 UTC (permalink / raw)
To: Loïc Lefort, qemu-devel
Cc: Weiwei Li, qemu-riscv, Palmer Dabbelt, Alistair Francis,
Daniel Henrique Barboza
On 2025/3/14 03:30, Loïc Lefort wrote:
> When Smepmp is supported, mseccfg.RLB allows bypassing locks when writing CSRs
> but should not affect interpretation of actual PMP rules.
>
> This is not the case with the current implementation where pmp_hart_has_privs
> calls pmp_is_locked which implements mseccfg.RLB bypass.
>
> This commit implements the correct behavior by removing mseccfg.RLB bypass from
> pmp_is_locked.
>
> RLB bypass when writing CSRs is implemented by adding a new pmp_is_readonly
> function that calls pmp_is_locked and check mseccfg.RLB. pmp_write_cfg and
> pmpaddr_csr_write are changed to use this new function.
>
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Zhiwei
> ---
> target/riscv/pmp.c | 43 +++++++++++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index b0841d44f4..e1e5ca589e 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -45,11 +45,6 @@ static inline uint8_t pmp_get_a_field(uint8_t cfg)
> */
> static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> {
> - /* mseccfg.RLB is set */
> - if (MSECCFG_RLB_ISSET(env)) {
> - return 0;
> - }
> -
> if (env->pmp_state.pmp[pmp_index].cfg_reg & PMP_LOCK) {
> return 1;
> }
> @@ -62,6 +57,15 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> return 0;
> }
>
> +/*
> + * Check whether a PMP is locked for writing or not.
> + * (i.e. has LOCK flag and mseccfg.RLB is unset)
> + */
> +static int pmp_is_readonly(CPURISCVState *env, uint32_t pmp_index)
> +{
> + return pmp_is_locked(env, pmp_index) && !MSECCFG_RLB_ISSET(env);
> +}
> +
> /*
> * Count the number of active rules.
> */
> @@ -90,39 +94,38 @@ 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 locked = true;
> + bool readonly = true;
>
> if (riscv_cpu_cfg(env)->ext_smepmp) {
> /* mseccfg.RLB is set */
> if (MSECCFG_RLB_ISSET(env)) {
> - locked = false;
> + readonly = false;
> }
>
> /* mseccfg.MML is not set */
> - if (!MSECCFG_MML_ISSET(env) && !pmp_is_locked(env, pmp_index)) {
> - locked = false;
> + 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) {
> - locked = false;
> + readonly = false;
> }
> /* shared region and not adding X bit */
> if ((val & PMP_LOCK) != PMP_LOCK &&
> (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
> - locked = false;
> + readonly = false;
> }
> }
> } else {
> - if (!pmp_is_locked(env, pmp_index)) {
> - locked = false;
> - }
> + readonly = pmp_is_readonly(env, pmp_index);
> }
>
> - if (locked) {
> - qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
> + if (readonly) {
> + 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) &&
> @@ -524,14 +527,14 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> uint8_t pmp_cfg = env->pmp_state.pmp[addr_index + 1].cfg_reg;
> is_next_cfg_tor = PMP_AMATCH_TOR == pmp_get_a_field(pmp_cfg);
>
> - if (pmp_is_locked(env, addr_index + 1) && is_next_cfg_tor) {
> + if (pmp_is_readonly(env, addr_index + 1) && is_next_cfg_tor) {
> qemu_log_mask(LOG_GUEST_ERROR,
> - "ignoring pmpaddr write - pmpcfg + 1 locked\n");
> + "ignoring pmpaddr write - pmpcfg+1 read only\n");
> return;
> }
> }
>
> - if (!pmp_is_locked(env, addr_index)) {
> + if (!pmp_is_readonly(env, addr_index)) {
> if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> env->pmp_state.pmp[addr_index].addr_reg = val;
> pmp_update_rule_addr(env, addr_index);
> @@ -542,7 +545,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> - "ignoring pmpaddr write - locked\n");
> + "ignoring pmpaddr write - read only\n");
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function
2025-03-13 19:30 ` [PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function Loïc Lefort
@ 2025-03-29 8:53 ` LIU Zhiwei
2025-04-04 1:02 ` Alistair Francis
1 sibling, 0 replies; 20+ messages in thread
From: LIU Zhiwei @ 2025-03-29 8:53 UTC (permalink / raw)
To: Loïc Lefort, qemu-devel
Cc: Weiwei Li, qemu-riscv, Palmer Dabbelt, Alistair Francis,
Daniel Henrique Barboza
On 2025/3/14 03:30, Loïc Lefort wrote:
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Zhiwei
> ---
> target/riscv/pmp.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index e1e5ca589e..7d65dc24a5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -31,6 +31,15 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> uint8_t val);
> static uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t addr_index);
>
> +/*
> + * Convert the PMP permissions to match the truth table in the Smepmp spec.
> + */
> +static inline uint8_t pmp_get_smepmp_operation(uint8_t cfg)
> +{
> + return ((cfg & PMP_LOCK) >> 4) | ((cfg & PMP_READ) << 2) |
> + (cfg & PMP_WRITE) | ((cfg & PMP_EXEC) >> 2);
> +}
> +
> /*
> * Accessor method to extract address matching type 'a field' from cfg reg
> */
> @@ -355,16 +364,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, hwaddr addr,
> const uint8_t a_field =
> pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
>
> - /*
> - * Convert the PMP permissions to match the truth table in the
> - * Smepmp spec.
> - */
> - const uint8_t smepmp_operation =
> - ((env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) >> 4) |
> - ((env->pmp_state.pmp[i].cfg_reg & PMP_READ) << 2) |
> - (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) |
> - ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2);
> -
> if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
> /*
> * If the PMP entry is not off and the address is in range,
> @@ -383,6 +382,9 @@ bool pmp_hart_has_privs(CPURISCVState *env, hwaddr addr,
> /*
> * If mseccfg.MML Bit set, do the enhanced pmp priv check
> */
> + const uint8_t smepmp_operation =
> + pmp_get_smepmp_operation(env->pmp_state.pmp[i].cfg_reg);
> +
> if (mode == PRV_M) {
> switch (smepmp_operation) {
> case 0:
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode
2025-03-13 19:30 ` [PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode Loïc Lefort
@ 2025-03-29 9:01 ` LIU Zhiwei
0 siblings, 0 replies; 20+ messages in thread
From: LIU Zhiwei @ 2025-03-29 9:01 UTC (permalink / raw)
To: Loïc Lefort, qemu-devel
Cc: Weiwei Li, qemu-riscv, Palmer Dabbelt, Alistair Francis,
Daniel Henrique Barboza
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;
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed
2025-03-13 19:30 ` [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed Loïc Lefort
@ 2025-03-29 9:03 ` LIU Zhiwei
2025-03-31 9:44 ` Loïc Lefort
2025-04-04 1:05 ` Alistair Francis
1 sibling, 1 reply; 20+ messages in thread
From: LIU Zhiwei @ 2025-03-29 9:03 UTC (permalink / raw)
To: Loïc Lefort, qemu-devel
Cc: Weiwei Li, qemu-riscv, Palmer Dabbelt, Alistair Francis,
Daniel Henrique Barboza
On 2025/3/14 03:30, Loïc Lefort wrote:
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> target/riscv/pmp.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index c5f6cdaccb..845915e0c8 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -141,6 +141,11 @@ 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) {
> + if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
> + /* no change */
> + return false;
> + }
> +
> if (pmp_is_readonly(env, pmp_index)) {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpcfg write - read only\n");
> @@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> bool is_next_cfg_tor = false;
>
> if (addr_index < MAX_RISCV_PMPS) {
> + if (env->pmp_state.pmp[addr_index].addr_reg == val) {
> + /* no change */
> + return;
> + }
> +
> /*
> * In TOR mode, need to check the lock bit of the next pmp
> * (if there is a next).
> @@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> }
>
> if (!pmp_is_readonly(env, addr_index)) {
> - if (env->pmp_state.pmp[addr_index].addr_reg != val) {
Is there some benefit removing this if sentence?
Zhiwei
> - env->pmp_state.pmp[addr_index].addr_reg = val;
> - pmp_update_rule_addr(env, addr_index);
> - if (is_next_cfg_tor) {
> - pmp_update_rule_addr(env, addr_index + 1);
> - }
> - tlb_flush(env_cpu(env));
> + env->pmp_state.pmp[addr_index].addr_reg = val;
> + pmp_update_rule_addr(env, addr_index);
> + if (is_next_cfg_tor) {
> + pmp_update_rule_addr(env, addr_index + 1);
> }
> + tlb_flush(env_cpu(env));
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpaddr write - read only\n");
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked
2025-03-13 19:30 ` [PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked Loïc Lefort
@ 2025-03-29 9:29 ` LIU Zhiwei
2025-04-04 1:06 ` Alistair Francis
1 sibling, 0 replies; 20+ messages in thread
From: LIU Zhiwei @ 2025-03-29 9:29 UTC (permalink / raw)
To: Loïc Lefort, qemu-devel
Cc: Weiwei Li, qemu-riscv, Palmer Dabbelt, Alistair Francis,
Daniel Henrique Barboza
On 2025/3/14 03:30, Loïc Lefort wrote:
> Remove useless check in pmp_is_locked, the function will return 0 in either
> case.
>
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Zhiwei
> ---
> target/riscv/pmp.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 845915e0c8..c685f7f2c5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -58,11 +58,6 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> return 1;
> }
>
> - /* Top PMP has no 'next' to check */
> - if ((pmp_index + 1u) >= MAX_RISCV_PMPS) {
> - return 0;
> - }
> -
> return 0;
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed
2025-03-29 9:03 ` LIU Zhiwei
@ 2025-03-31 9:44 ` Loïc Lefort
2025-03-31 11:31 ` LIU Zhiwei
0 siblings, 1 reply; 20+ messages in thread
From: Loïc Lefort @ 2025-03-31 9:44 UTC (permalink / raw)
To: LIU Zhiwei
Cc: qemu-devel, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza
[-- Attachment #1: Type: text/plain, Size: 3165 bytes --]
On Sat, Mar 29, 2025 at 10:03 AM LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
wrote:
>
> On 2025/3/14 03:30, Loïc Lefort wrote:
> > Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> > target/riscv/pmp.c | 22 +++++++++++++++-------
> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index c5f6cdaccb..845915e0c8 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -141,6 +141,11 @@ 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) {
> > + if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
> > + /* no change */
> > + return false;
> > + }
> > +
> > if (pmp_is_readonly(env, pmp_index)) {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "ignoring pmpcfg write - read only\n");
> > @@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t
> addr_index,
> > bool is_next_cfg_tor = false;
> >
> > if (addr_index < MAX_RISCV_PMPS) {
> > + if (env->pmp_state.pmp[addr_index].addr_reg == val) {
> > + /* no change */
> > + return;
> > + }
> > +
> > /*
> > * In TOR mode, need to check the lock bit of the next pmp
> > * (if there is a next).
> > @@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env,
> uint32_t addr_index,
> > }
> >
> > if (!pmp_is_readonly(env, addr_index)) {
> > - if (env->pmp_state.pmp[addr_index].addr_reg != val) {
>
> Is there some benefit removing this if sentence?
>
>
> The if is not removed, it's moved to the top of the function. The goal is
to skip processing and warnings when the write would not change the value
already present in the register.For pmp_write_cfg, each pmpcfg register
contains 4 config values and some of them might be locked so we want to
avoid warnings when writing with unchanged values.
As you noted, the similar change in pmpaddr_csr_write has less benefit:
it's only a minor optimization and is done for symmetry with pmp_write_cfg.
Loïc.
>
> > - env->pmp_state.pmp[addr_index].addr_reg = val;
> > - pmp_update_rule_addr(env, addr_index);
> > - if (is_next_cfg_tor) {
> > - pmp_update_rule_addr(env, addr_index + 1);
> > - }
> > - tlb_flush(env_cpu(env));
> > + env->pmp_state.pmp[addr_index].addr_reg = val;
> > + pmp_update_rule_addr(env, addr_index);
> > + if (is_next_cfg_tor) {
> > + pmp_update_rule_addr(env, addr_index + 1);
> > }
> > + tlb_flush(env_cpu(env));
> > } else {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "ignoring pmpaddr write - read only\n");
>
[-- Attachment #2: Type: text/html, Size: 4369 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed
2025-03-31 9:44 ` Loïc Lefort
@ 2025-03-31 11:31 ` LIU Zhiwei
0 siblings, 0 replies; 20+ messages in thread
From: LIU Zhiwei @ 2025-03-31 11:31 UTC (permalink / raw)
To: Loïc Lefort
Cc: qemu-devel, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza
[-- Attachment #1: Type: text/plain, Size: 3712 bytes --]
On 2025/3/31 17:44, Loïc Lefort wrote:
>
>
> On Sat, Mar 29, 2025 at 10:03 AM LIU Zhiwei
> <zhiwei_liu@linux.alibaba.com> wrote:
>
>
> On 2025/3/14 03:30, Loïc Lefort wrote:
> > Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> > target/riscv/pmp.c | 22 +++++++++++++++-------
> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index c5f6cdaccb..845915e0c8 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -141,6 +141,11 @@ 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) {
> > + if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
> > + /* no change */
> > + return false;
> > + }
> > +
> > if (pmp_is_readonly(env, pmp_index)) {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "ignoring pmpcfg write - read only\n");
> > @@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env,
> uint32_t addr_index,
> > bool is_next_cfg_tor = false;
> >
> > if (addr_index < MAX_RISCV_PMPS) {
> > + if (env->pmp_state.pmp[addr_index].addr_reg == val) {
> > + /* no change */
> > + return;
> > + }
> > +
> > /*
> > * In TOR mode, need to check the lock bit of the next pmp
> > * (if there is a next).
> > @@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env,
> uint32_t addr_index,
> > }
> >
> > if (!pmp_is_readonly(env, addr_index)) {
> > - if (env->pmp_state.pmp[addr_index].addr_reg != val) {
>
> Is there some benefit removing this if sentence?
>
>
> The if is not removed, it's moved to the top of the function.
Oops! I miss it.
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Thanks,
Zhiwei
> The goal is to skip processing and warnings when the write would not
> change the value already present in the register.For pmp_write_cfg,
> each pmpcfg register contains 4 config values and some of them might
> be locked so we want to avoid warnings when writing with unchanged values.
> As you noted, the similar change in pmpaddr_csr_write has less
> benefit: it's only a minor optimization and is done for symmetry with
> pmp_write_cfg.
>
> Loïc.
>
>
> > - env->pmp_state.pmp[addr_index].addr_reg = val;
> > - pmp_update_rule_addr(env, addr_index);
> > - if (is_next_cfg_tor) {
> > - pmp_update_rule_addr(env, addr_index + 1);
> > - }
> > - tlb_flush(env_cpu(env));
> > + env->pmp_state.pmp[addr_index].addr_reg = val;
> > + pmp_update_rule_addr(env, addr_index);
> > + if (is_next_cfg_tor) {
> > + pmp_update_rule_addr(env, addr_index + 1);
> > }
> > + tlb_flush(env_cpu(env));
> > } else {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "ignoring pmpaddr write - read only\n");
>
[-- Attachment #2: Type: text/html, Size: 6865 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges
2025-03-13 19:30 ` [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges Loïc Lefort
2025-03-27 17:30 ` Daniel Henrique Barboza
2025-03-29 8:33 ` LIU Zhiwei
@ 2025-04-04 1:01 ` Alistair Francis
2 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2025-04-04 1:01 UTC (permalink / raw)
To: Loïc Lefort
Cc: qemu-devel, Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza
On Fri, Mar 14, 2025 at 5:32 AM Loïc Lefort <loic@rivosinc.com> wrote:
>
> When Smepmp is supported, mseccfg.RLB allows bypassing locks when writing CSRs
> but should not affect interpretation of actual PMP rules.
>
> This is not the case with the current implementation where pmp_hart_has_privs
> calls pmp_is_locked which implements mseccfg.RLB bypass.
>
> This commit implements the correct behavior by removing mseccfg.RLB bypass from
> pmp_is_locked.
>
> RLB bypass when writing CSRs is implemented by adding a new pmp_is_readonly
> function that calls pmp_is_locked and check mseccfg.RLB. pmp_write_cfg and
> pmpaddr_csr_write are changed to use this new function.
>
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/pmp.c | 43 +++++++++++++++++++++++--------------------
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index b0841d44f4..e1e5ca589e 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -45,11 +45,6 @@ static inline uint8_t pmp_get_a_field(uint8_t cfg)
> */
> static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> {
> - /* mseccfg.RLB is set */
> - if (MSECCFG_RLB_ISSET(env)) {
> - return 0;
> - }
> -
> if (env->pmp_state.pmp[pmp_index].cfg_reg & PMP_LOCK) {
> return 1;
> }
> @@ -62,6 +57,15 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> return 0;
> }
>
> +/*
> + * Check whether a PMP is locked for writing or not.
> + * (i.e. has LOCK flag and mseccfg.RLB is unset)
> + */
> +static int pmp_is_readonly(CPURISCVState *env, uint32_t pmp_index)
> +{
> + return pmp_is_locked(env, pmp_index) && !MSECCFG_RLB_ISSET(env);
> +}
> +
> /*
> * Count the number of active rules.
> */
> @@ -90,39 +94,38 @@ 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 locked = true;
> + bool readonly = true;
>
> if (riscv_cpu_cfg(env)->ext_smepmp) {
> /* mseccfg.RLB is set */
> if (MSECCFG_RLB_ISSET(env)) {
> - locked = false;
> + readonly = false;
> }
>
> /* mseccfg.MML is not set */
> - if (!MSECCFG_MML_ISSET(env) && !pmp_is_locked(env, pmp_index)) {
> - locked = false;
> + 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) {
> - locked = false;
> + readonly = false;
> }
> /* shared region and not adding X bit */
> if ((val & PMP_LOCK) != PMP_LOCK &&
> (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
> - locked = false;
> + readonly = false;
> }
> }
> } else {
> - if (!pmp_is_locked(env, pmp_index)) {
> - locked = false;
> - }
> + readonly = pmp_is_readonly(env, pmp_index);
> }
>
> - if (locked) {
> - qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
> + if (readonly) {
> + 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) &&
> @@ -524,14 +527,14 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> uint8_t pmp_cfg = env->pmp_state.pmp[addr_index + 1].cfg_reg;
> is_next_cfg_tor = PMP_AMATCH_TOR == pmp_get_a_field(pmp_cfg);
>
> - if (pmp_is_locked(env, addr_index + 1) && is_next_cfg_tor) {
> + if (pmp_is_readonly(env, addr_index + 1) && is_next_cfg_tor) {
> qemu_log_mask(LOG_GUEST_ERROR,
> - "ignoring pmpaddr write - pmpcfg + 1 locked\n");
> + "ignoring pmpaddr write - pmpcfg+1 read only\n");
> return;
> }
> }
>
> - if (!pmp_is_locked(env, addr_index)) {
> + if (!pmp_is_readonly(env, addr_index)) {
> if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> env->pmp_state.pmp[addr_index].addr_reg = val;
> pmp_update_rule_addr(env, addr_index);
> @@ -542,7 +545,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> - "ignoring pmpaddr write - locked\n");
> + "ignoring pmpaddr write - read only\n");
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> --
> 2.47.2
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function
2025-03-13 19:30 ` [PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function Loïc Lefort
2025-03-29 8:53 ` LIU Zhiwei
@ 2025-04-04 1:02 ` Alistair Francis
1 sibling, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2025-04-04 1:02 UTC (permalink / raw)
To: Loïc Lefort
Cc: qemu-devel, Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza
On Fri, Mar 14, 2025 at 5:33 AM Loïc Lefort <loic@rivosinc.com> wrote:
>
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/pmp.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index e1e5ca589e..7d65dc24a5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -31,6 +31,15 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> uint8_t val);
> static uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t addr_index);
>
> +/*
> + * Convert the PMP permissions to match the truth table in the Smepmp spec.
> + */
> +static inline uint8_t pmp_get_smepmp_operation(uint8_t cfg)
> +{
> + return ((cfg & PMP_LOCK) >> 4) | ((cfg & PMP_READ) << 2) |
> + (cfg & PMP_WRITE) | ((cfg & PMP_EXEC) >> 2);
> +}
> +
> /*
> * Accessor method to extract address matching type 'a field' from cfg reg
> */
> @@ -355,16 +364,6 @@ bool pmp_hart_has_privs(CPURISCVState *env, hwaddr addr,
> const uint8_t a_field =
> pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
>
> - /*
> - * Convert the PMP permissions to match the truth table in the
> - * Smepmp spec.
> - */
> - const uint8_t smepmp_operation =
> - ((env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) >> 4) |
> - ((env->pmp_state.pmp[i].cfg_reg & PMP_READ) << 2) |
> - (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) |
> - ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2);
> -
> if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
> /*
> * If the PMP entry is not off and the address is in range,
> @@ -383,6 +382,9 @@ bool pmp_hart_has_privs(CPURISCVState *env, hwaddr addr,
> /*
> * If mseccfg.MML Bit set, do the enhanced pmp priv check
> */
> + const uint8_t smepmp_operation =
> + pmp_get_smepmp_operation(env->pmp_state.pmp[i].cfg_reg);
> +
> if (mode == PRV_M) {
> switch (smepmp_operation) {
> case 0:
> --
> 2.47.2
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed
2025-03-13 19:30 ` [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed Loïc Lefort
2025-03-29 9:03 ` LIU Zhiwei
@ 2025-04-04 1:05 ` Alistair Francis
1 sibling, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2025-04-04 1:05 UTC (permalink / raw)
To: Loïc Lefort
Cc: qemu-devel, Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza
On Fri, Mar 14, 2025 at 5:33 AM Loïc Lefort <loic@rivosinc.com> wrote:
>
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/pmp.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index c5f6cdaccb..845915e0c8 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -141,6 +141,11 @@ 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) {
> + if (env->pmp_state.pmp[pmp_index].cfg_reg == val) {
> + /* no change */
> + return false;
> + }
> +
> if (pmp_is_readonly(env, pmp_index)) {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpcfg write - read only\n");
> @@ -528,6 +533,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> bool is_next_cfg_tor = false;
>
> if (addr_index < MAX_RISCV_PMPS) {
> + if (env->pmp_state.pmp[addr_index].addr_reg == val) {
> + /* no change */
> + return;
> + }
> +
> /*
> * In TOR mode, need to check the lock bit of the next pmp
> * (if there is a next).
> @@ -544,14 +554,12 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> }
>
> if (!pmp_is_readonly(env, addr_index)) {
> - if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> - env->pmp_state.pmp[addr_index].addr_reg = val;
> - pmp_update_rule_addr(env, addr_index);
> - if (is_next_cfg_tor) {
> - pmp_update_rule_addr(env, addr_index + 1);
> - }
> - tlb_flush(env_cpu(env));
> + env->pmp_state.pmp[addr_index].addr_reg = val;
> + pmp_update_rule_addr(env, addr_index);
> + if (is_next_cfg_tor) {
> + pmp_update_rule_addr(env, addr_index + 1);
> }
> + tlb_flush(env_cpu(env));
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpaddr write - read only\n");
> --
> 2.47.2
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked
2025-03-13 19:30 ` [PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked Loïc Lefort
2025-03-29 9:29 ` LIU Zhiwei
@ 2025-04-04 1:06 ` Alistair Francis
1 sibling, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2025-04-04 1:06 UTC (permalink / raw)
To: Loïc Lefort
Cc: qemu-devel, Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza
On Fri, Mar 14, 2025 at 5:35 AM Loïc Lefort <loic@rivosinc.com> wrote:
>
> Remove useless check in pmp_is_locked, the function will return 0 in either
> case.
>
> Signed-off-by: Loïc Lefort <loic@rivosinc.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/pmp.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 845915e0c8..c685f7f2c5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -58,11 +58,6 @@ static inline int pmp_is_locked(CPURISCVState *env, uint32_t pmp_index)
> return 1;
> }
>
> - /* Top PMP has no 'next' to check */
> - if ((pmp_index + 1u) >= MAX_RISCV_PMPS) {
> - return 0;
> - }
> -
> return 0;
> }
>
> --
> 2.47.2
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification
2025-03-13 19:30 [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
` (5 preceding siblings ...)
2025-03-27 16:47 ` [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
@ 2025-04-04 1:23 ` Alistair Francis
6 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2025-04-04 1:23 UTC (permalink / raw)
To: Loïc Lefort
Cc: qemu-devel, Liu Zhiwei, Weiwei Li, qemu-riscv, Palmer Dabbelt,
Alistair Francis, Daniel Henrique Barboza
On Fri, Mar 14, 2025 at 5:33 AM Loïc Lefort <loic@rivosinc.com> wrote:
>
> These patches fix Smepmp implementation to make it compliant with the spec.
>
> First patch limits RLB to CSR changes since RLB should not affect privilege
> evaluation. Patch 2 extracts some common code into a function (to be used in
> patch 3). Patch 3 fixes validation of pmpcfg CSR writes in order to match Smepmp
> specification. Patch 4 is a small optimization and last patch is just removing
> redundant code.
>
> ---
> Changes in v2:
> - rebased to latest riscv-to-apply.next
> - addressed Daniel comments on patch 1
>
> Link to v1:
> https://lore.kernel.org/qemu-riscv/20250225160052.39564-1-loic@rivosinc.com/
>
> Loïc Lefort (5):
> target/riscv: pmp: don't allow RLB to bypass rule privileges
> target/riscv: pmp: move Smepmp operation conversion into a function
> target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode
> target/riscv: pmp: exit csr writes early if value was not changed
> target/riscv: pmp: remove redundant check in pmp_is_locked
Thanks!
Applied to riscv-to-apply.next
Alistair
>
> target/riscv/pmp.c | 151 +++++++++++++++++++++++++--------------------
> 1 file changed, 83 insertions(+), 68 deletions(-)
>
> --
> 2.47.2
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2025-04-04 1:24 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-13 19:30 [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
2025-03-13 19:30 ` [PATCH v2 1/5] target/riscv: pmp: don't allow RLB to bypass rule privileges Loïc Lefort
2025-03-27 17:30 ` Daniel Henrique Barboza
2025-03-29 8:33 ` LIU Zhiwei
2025-04-04 1:01 ` Alistair Francis
2025-03-13 19:30 ` [PATCH v2 2/5] target/riscv: pmp: move Smepmp operation conversion into a function Loïc Lefort
2025-03-29 8:53 ` LIU Zhiwei
2025-04-04 1:02 ` Alistair Francis
2025-03-13 19:30 ` [PATCH v2 3/5] target/riscv: pmp: fix checks on writes to pmpcfg in Smepmp MML mode Loïc Lefort
2025-03-29 9:01 ` LIU Zhiwei
2025-03-13 19:30 ` [PATCH v2 4/5] target/riscv: pmp: exit csr writes early if value was not changed Loïc Lefort
2025-03-29 9:03 ` LIU Zhiwei
2025-03-31 9:44 ` Loïc Lefort
2025-03-31 11:31 ` LIU Zhiwei
2025-04-04 1:05 ` Alistair Francis
2025-03-13 19:30 ` [PATCH v2 5/5] target/riscv: pmp: remove redundant check in pmp_is_locked Loïc Lefort
2025-03-29 9:29 ` LIU Zhiwei
2025-04-04 1:06 ` Alistair Francis
2025-03-27 16:47 ` [PATCH v2 0/5] target/riscv: Smepmp fixes to match specification Loïc Lefort
2025-04-04 1:23 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).