* [PATCH 0/6] target/riscv: Fix PMP related problem
@ 2023-04-13 9:01 Weiwei Li
2023-04-13 9:01 ` [PATCH 1/6] target/riscv: Update pmp_get_tlb_size() Weiwei Li
` (6 more replies)
0 siblings, 7 replies; 29+ messages in thread
From: Weiwei Li @ 2023-04-13 9:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
richard.henderson, wangjunqiang, lazyparser, Weiwei Li
This patchset tries to fix the PMP bypass problem issue https://gitlab.com/qemu-project/qemu/-/issues/1542
The port is available here:
https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
Weiwei Li (6):
target/riscv: Update pmp_get_tlb_size()
target/riscv: Move pmp_get_tlb_size apart from
get_physical_address_pmp
target/riscv: flush tlb when pmpaddr is updated
target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
target/riscv: flush tb when PMP entry changes
accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
re-filled
accel/tcg/cputlb.c | 7 -----
target/riscv/cpu_helper.c | 19 ++++---------
target/riscv/pmp.c | 60 ++++++++++++++++++++++++++-------------
target/riscv/pmp.h | 3 +-
4 files changed, 47 insertions(+), 42 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/6] target/riscv: Update pmp_get_tlb_size()
2023-04-13 9:01 [PATCH 0/6] target/riscv: Fix PMP related problem Weiwei Li
@ 2023-04-13 9:01 ` Weiwei Li
2023-04-18 2:53 ` Alistair Francis
2023-04-13 9:01 ` [PATCH 2/6] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp Weiwei Li
` (5 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Weiwei Li @ 2023-04-13 9:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
richard.henderson, wangjunqiang, lazyparser, Weiwei Li
Not only the matched PMP entry, Any PMP entry that overlap with partial of
the tlb page may make the regions in that page have different permission
rights. So all of them should be taken into consideration.
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/cpu_helper.c | 7 ++-----
target/riscv/pmp.c | 34 +++++++++++++++++++++-------------
target/riscv/pmp.h | 3 +--
3 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 433ea529b0..075fc0538a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -703,11 +703,8 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
}
*prot = pmp_priv_to_page_prot(pmp_priv);
- if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) {
- target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
- target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
-
- *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa, tlb_ea);
+ if (tlb_size != NULL) {
+ *tlb_size = pmp_get_tlb_size(env, addr);
}
return TRANSLATE_SUCCESS;
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 1f5aca42e8..4f9389e73c 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -601,28 +601,36 @@ target_ulong mseccfg_csr_read(CPURISCVState *env)
}
/*
- * Calculate the TLB size if the start address or the end address of
+ * Calculate the TLB size if any start address or the end address of
* PMP entry is presented in the TLB page.
*/
-target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
- target_ulong tlb_sa, target_ulong tlb_ea)
+target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr)
{
- target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa;
- target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea;
+ target_ulong pmp_sa;
+ target_ulong pmp_ea;
+ target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
+ target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
+ int i;
+
+ for (i = 0; i < MAX_RISCV_PMPS; i++) {
+ pmp_sa = env->pmp_state.addr[i].sa;
+ pmp_ea = env->pmp_state.addr[i].ea;
- if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) {
- return TARGET_PAGE_SIZE;
- } else {
/*
- * At this point we have a tlb_size that is the smallest possible size
- * That fits within a TARGET_PAGE_SIZE and the PMP region.
- *
- * If the size is less then TARGET_PAGE_SIZE we drop the size to 1.
+ * If any start address or the end address of PMP entry is presented
+ * in the TLB page and cannot override the whole TLB page we drop the
+ * size to 1.
* This means the result isn't cached in the TLB and is only used for
* a single translation.
*/
- return 1;
+ if (((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) ||
+ (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) &&
+ !(pmp_sa == tlb_sa && pmp_ea == tlb_ea)) {
+ return 1;
+ }
}
+
+ return TARGET_PAGE_SIZE;
}
/*
diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
index b296ea1fc6..0a7e24750b 100644
--- a/target/riscv/pmp.h
+++ b/target/riscv/pmp.h
@@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
target_ulong size, pmp_priv_t privs,
pmp_priv_t *allowed_privs,
target_ulong mode);
-target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
- target_ulong tlb_sa, target_ulong tlb_ea);
+target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr);
void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
void pmp_update_rule_nums(CPURISCVState *env);
uint32_t pmp_get_num_rules(CPURISCVState *env);
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/6] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp
2023-04-13 9:01 [PATCH 0/6] target/riscv: Fix PMP related problem Weiwei Li
2023-04-13 9:01 ` [PATCH 1/6] target/riscv: Update pmp_get_tlb_size() Weiwei Li
@ 2023-04-13 9:01 ` Weiwei Li
2023-04-18 2:54 ` Alistair Francis
2023-04-13 9:01 ` [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated Weiwei Li
` (4 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Weiwei Li @ 2023-04-13 9:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
richard.henderson, wangjunqiang, lazyparser, Weiwei Li
pmp_get_tlb_size have no relationship with pmp-related permission check
currently.
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/cpu_helper.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 075fc0538a..83c9699a6d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -676,14 +676,11 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
*
* @env: CPURISCVState
* @prot: The returned protection attributes
- * @tlb_size: TLB page size containing addr. It could be modified after PMP
- * permission checking. NULL if not set TLB page for addr.
* @addr: The physical address to be checked permission
* @access_type: The type of MMU access
* @mode: Indicates current privilege level.
*/
-static int get_physical_address_pmp(CPURISCVState *env, int *prot,
- target_ulong *tlb_size, hwaddr addr,
+static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
int size, MMUAccessType access_type,
int mode)
{
@@ -703,9 +700,6 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
}
*prot = pmp_priv_to_page_prot(pmp_priv);
- if (tlb_size != NULL) {
- *tlb_size = pmp_get_tlb_size(env, addr);
- }
return TRANSLATE_SUCCESS;
}
@@ -905,7 +899,7 @@ restart:
}
int pmp_prot;
- int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, pte_addr,
+ int pmp_ret = get_physical_address_pmp(env, &pmp_prot, pte_addr,
sizeof(target_ulong),
MMU_DATA_LOAD, PRV_S);
if (pmp_ret != TRANSLATE_SUCCESS) {
@@ -1300,8 +1294,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
prot &= prot2;
if (ret == TRANSLATE_SUCCESS) {
- ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
+ ret = get_physical_address_pmp(env, &prot_pmp, pa,
size, access_type, mode);
+ tlb_size = pmp_get_tlb_size(env, pa);
qemu_log_mask(CPU_LOG_MMU,
"%s PMP address=" HWADDR_FMT_plx " ret %d prot"
@@ -1333,8 +1328,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
__func__, address, ret, pa, prot);
if (ret == TRANSLATE_SUCCESS) {
- ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
+ ret = get_physical_address_pmp(env, &prot_pmp, pa,
size, access_type, mode);
+ tlb_size = pmp_get_tlb_size(env, pa);
qemu_log_mask(CPU_LOG_MMU,
"%s PMP address=" HWADDR_FMT_plx " ret %d prot"
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated
2023-04-13 9:01 [PATCH 0/6] target/riscv: Fix PMP related problem Weiwei Li
2023-04-13 9:01 ` [PATCH 1/6] target/riscv: Update pmp_get_tlb_size() Weiwei Li
2023-04-13 9:01 ` [PATCH 2/6] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp Weiwei Li
@ 2023-04-13 9:01 ` Weiwei Li
2023-04-18 2:36 ` Alistair Francis
2023-04-18 7:11 ` LIU Zhiwei
2023-04-13 9:01 ` [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes Weiwei Li
` (3 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Weiwei Li @ 2023-04-13 9:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
richard.henderson, wangjunqiang, lazyparser, Weiwei Li
TLB should be flushed not only for pmpcfg csr changes, but also for
pmpaddr csr changes.
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/pmp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 4f9389e73c..6d4813806b 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -537,6 +537,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
if (!pmp_is_locked(env, addr_index)) {
env->pmp_state.pmp[addr_index].addr_reg = val;
pmp_update_rule(env, addr_index);
+ tlb_flush(env_cpu(env));
} else {
qemu_log_mask(LOG_GUEST_ERROR,
"ignoring pmpaddr write - locked\n");
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
2023-04-13 9:01 [PATCH 0/6] target/riscv: Fix PMP related problem Weiwei Li
` (2 preceding siblings ...)
2023-04-13 9:01 ` [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated Weiwei Li
@ 2023-04-13 9:01 ` Weiwei Li
2023-04-18 2:39 ` Alistair Francis
2023-04-18 7:14 ` LIU Zhiwei
2023-04-13 9:01 ` [PATCH 5/6] target/riscv: flush tb when PMP entry changes Weiwei Li
` (2 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Weiwei Li @ 2023-04-13 9:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
richard.henderson, wangjunqiang, lazyparser, Weiwei Li
TLB needn't be flushed when pmpcfg/pmpaddr don't changes.
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/pmp.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 6d4813806b..aced23c4d5 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -26,7 +26,7 @@
#include "trace.h"
#include "exec/exec-all.h"
-static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
+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);
static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index);
@@ -83,7 +83,7 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
* Accessor to set the cfg reg for a specific PMP/HART
* Bounds checks and relevant lock bit.
*/
-static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
+static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
{
if (pmp_index < MAX_RISCV_PMPS) {
bool locked = true;
@@ -119,14 +119,17 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
if (locked) {
qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
- } else {
+ } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
env->pmp_state.pmp[pmp_index].cfg_reg = val;
pmp_update_rule(env, pmp_index);
+ return true;
}
} else {
qemu_log_mask(LOG_GUEST_ERROR,
"ignoring pmpcfg write - out of bounds\n");
}
+
+ return false;
}
static void pmp_decode_napot(target_ulong a, target_ulong *sa,
@@ -477,16 +480,19 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
int i;
uint8_t cfg_val;
int pmpcfg_nums = 2 << riscv_cpu_mxl(env);
+ bool modified = false;
trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
for (i = 0; i < pmpcfg_nums; i++) {
cfg_val = (val >> 8 * i) & 0xff;
- pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
+ modified |= pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
}
/* If PMP permission of any addr has been changed, flush TLB pages. */
- tlb_flush(env_cpu(env));
+ if (modified) {
+ tlb_flush(env_cpu(env));
+ }
}
@@ -535,9 +541,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
}
if (!pmp_is_locked(env, addr_index)) {
- env->pmp_state.pmp[addr_index].addr_reg = val;
- pmp_update_rule(env, addr_index);
- tlb_flush(env_cpu(env));
+ if (env->pmp_state.pmp[addr_index].addr_reg != val) {
+ env->pmp_state.pmp[addr_index].addr_reg = val;
+ pmp_update_rule(env, addr_index);
+ tlb_flush(env_cpu(env));
+ }
} else {
qemu_log_mask(LOG_GUEST_ERROR,
"ignoring pmpaddr write - locked\n");
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 5/6] target/riscv: flush tb when PMP entry changes
2023-04-13 9:01 [PATCH 0/6] target/riscv: Fix PMP related problem Weiwei Li
` (3 preceding siblings ...)
2023-04-13 9:01 ` [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes Weiwei Li
@ 2023-04-13 9:01 ` Weiwei Li
2023-04-18 7:28 ` LIU Zhiwei
2023-04-13 9:01 ` [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled Weiwei Li
2023-04-18 3:07 ` [PATCH 0/6] target/riscv: Fix PMP related problem LIU Zhiwei
6 siblings, 1 reply; 29+ messages in thread
From: Weiwei Li @ 2023-04-13 9:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
richard.henderson, wangjunqiang, lazyparser, Weiwei Li
The translation block may also be affected when PMP entry changes.
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/pmp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index aced23c4d5..c2db52361f 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -25,6 +25,7 @@
#include "cpu.h"
#include "trace.h"
#include "exec/exec-all.h"
+#include "exec/tb-flush.h"
static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
uint8_t val);
@@ -492,6 +493,7 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
/* If PMP permission of any addr has been changed, flush TLB pages. */
if (modified) {
tlb_flush(env_cpu(env));
+ tb_flush(env_cpu(env));
}
}
@@ -545,6 +547,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
env->pmp_state.pmp[addr_index].addr_reg = val;
pmp_update_rule(env, addr_index);
tlb_flush(env_cpu(env));
+ tb_flush(env_cpu(env));
}
} else {
qemu_log_mask(LOG_GUEST_ERROR,
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled
2023-04-13 9:01 [PATCH 0/6] target/riscv: Fix PMP related problem Weiwei Li
` (4 preceding siblings ...)
2023-04-13 9:01 ` [PATCH 5/6] target/riscv: flush tb when PMP entry changes Weiwei Li
@ 2023-04-13 9:01 ` Weiwei Li
2023-04-17 16:25 ` Daniel Henrique Barboza
2023-04-18 3:07 ` [PATCH 0/6] target/riscv: Fix PMP related problem LIU Zhiwei
6 siblings, 1 reply; 29+ messages in thread
From: Weiwei Li @ 2023-04-13 9:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
richard.henderson, wangjunqiang, lazyparser, Weiwei Li
When PMP entry overlap part of the page, we'll set the tlb_size to 1, and
this will make the address set with TLB_INVALID_MASK to make the page
un-cached. However, if we clear TLB_INVALID_MASK when TLB is re-filled, then
the TLB host address will be cached, and the following instructions can use
this host address directly which may lead to the bypass of PMP related check.
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
accel/tcg/cputlb.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index e984a98dc4..d0bf996405 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr,
/* TLB resize via tlb_fill may have moved the entry. */
index = tlb_index(env, mmu_idx, addr);
entry = tlb_entry(env, mmu_idx, addr);
-
- /*
- * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
- * to force the next access through tlb_fill. We've just
- * called tlb_fill, so we know that this entry *is* valid.
- */
- flags &= ~TLB_INVALID_MASK;
}
tlb_addr = tlb_read_ofs(entry, elt_ofs);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled
2023-04-13 9:01 ` [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled Weiwei Li
@ 2023-04-17 16:25 ` Daniel Henrique Barboza
2023-04-18 0:48 ` Weiwei Li
2023-04-18 7:18 ` Richard Henderson
0 siblings, 2 replies; 29+ messages in thread
From: Daniel Henrique Barboza @ 2023-04-17 16:25 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, richard.henderson,
wangjunqiang, lazyparser
On 4/13/23 06:01, Weiwei Li wrote:
> When PMP entry overlap part of the page, we'll set the tlb_size to 1, and
> this will make the address set with TLB_INVALID_MASK to make the page
> un-cached. However, if we clear TLB_INVALID_MASK when TLB is re-filled, then
> the TLB host address will be cached, and the following instructions can use
> this host address directly which may lead to the bypass of PMP related check.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
For this commit I believe it's worth mentioning that it's partially reverting
commit c3c8bf579b431b6b ("accel/tcg: Suppress auto-invalidate in
probe_access_internal") that was made to handle a particularity/quirk that was
present in s390x code.
At first glance this patch seems benign but we must make sure that no other
assumptions were made with this particular change in probe_access_internal().
Thanks,
Daniel
> accel/tcg/cputlb.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index e984a98dc4..d0bf996405 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr,
> /* TLB resize via tlb_fill may have moved the entry. */
> index = tlb_index(env, mmu_idx, addr);
> entry = tlb_entry(env, mmu_idx, addr);
> -
> - /*
> - * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
> - * to force the next access through tlb_fill. We've just
> - * called tlb_fill, so we know that this entry *is* valid.
> - */
> - flags &= ~TLB_INVALID_MASK;
> }
> tlb_addr = tlb_read_ofs(entry, elt_ofs);
> }
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled
2023-04-17 16:25 ` Daniel Henrique Barboza
@ 2023-04-18 0:48 ` Weiwei Li
2023-04-18 7:18 ` Richard Henderson
1 sibling, 0 replies; 29+ messages in thread
From: Weiwei Li @ 2023-04-18 0:48 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-riscv, qemu-devel
Cc: liweiwei, palmer, alistair.francis, bin.meng, zhiwei_liu,
richard.henderson, wangjunqiang, lazyparser
On 2023/4/18 00:25, Daniel Henrique Barboza wrote:
>
>
> On 4/13/23 06:01, Weiwei Li wrote:
>> When PMP entry overlap part of the page, we'll set the tlb_size to 1,
>> and
>> this will make the address set with TLB_INVALID_MASK to make the page
>> un-cached. However, if we clear TLB_INVALID_MASK when TLB is
>> re-filled, then
>> the TLB host address will be cached, and the following instructions
>> can use
>> this host address directly which may lead to the bypass of PMP
>> related check.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>
> For this commit I believe it's worth mentioning that it's partially
> reverting
> commit c3c8bf579b431b6b ("accel/tcg: Suppress auto-invalidate in
> probe_access_internal") that was made to handle a particularity/quirk
> that was
> present in s390x code.
>
> At first glance this patch seems benign but we must make sure that no
> other
> assumptions were made with this particular change in
> probe_access_internal().
I think this change will introduce no external function change except
that we should
always walk the page table(fill_tlb) for memory access to that page. And
this is needed
for pages that are partially overlapped by PMP region.
Regards,
Weiwei Li
>
>
> Thanks,
>
> Daniel
>
>> accel/tcg/cputlb.c | 7 -------
>> 1 file changed, 7 deletions(-)
>>
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index e984a98dc4..d0bf996405 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState
>> *env, target_ulong addr,
>> /* TLB resize via tlb_fill may have moved the entry. */
>> index = tlb_index(env, mmu_idx, addr);
>> entry = tlb_entry(env, mmu_idx, addr);
>> -
>> - /*
>> - * With PAGE_WRITE_INV, we set TLB_INVALID_MASK
>> immediately,
>> - * to force the next access through tlb_fill. We've just
>> - * called tlb_fill, so we know that this entry *is* valid.
>> - */
>> - flags &= ~TLB_INVALID_MASK;
>> }
>> tlb_addr = tlb_read_ofs(entry, elt_ofs);
>> }
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated
2023-04-13 9:01 ` [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated Weiwei Li
@ 2023-04-18 2:36 ` Alistair Francis
2023-04-18 7:11 ` LIU Zhiwei
1 sibling, 0 replies; 29+ messages in thread
From: Alistair Francis @ 2023-04-18 2:36 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, richard.henderson, wangjunqiang, lazyparser
On Thu, Apr 13, 2023 at 7:03 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> TLB should be flushed not only for pmpcfg csr changes, but also for
> pmpaddr csr changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/pmp.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 4f9389e73c..6d4813806b 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -537,6 +537,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> if (!pmp_is_locked(env, addr_index)) {
> env->pmp_state.pmp[addr_index].addr_reg = val;
> pmp_update_rule(env, addr_index);
> + tlb_flush(env_cpu(env));
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpaddr write - locked\n");
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
2023-04-13 9:01 ` [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes Weiwei Li
@ 2023-04-18 2:39 ` Alistair Francis
2023-04-18 7:14 ` LIU Zhiwei
1 sibling, 0 replies; 29+ messages in thread
From: Alistair Francis @ 2023-04-18 2:39 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, richard.henderson, wangjunqiang, lazyparser
On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> TLB needn't be flushed when pmpcfg/pmpaddr don't changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/pmp.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 6d4813806b..aced23c4d5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -26,7 +26,7 @@
> #include "trace.h"
> #include "exec/exec-all.h"
>
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> +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);
> static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index);
> @@ -83,7 +83,7 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
> * Accessor to set the cfg reg for a specific PMP/HART
> * Bounds checks and relevant lock bit.
> */
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> +static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> {
> if (pmp_index < MAX_RISCV_PMPS) {
> bool locked = true;
> @@ -119,14 +119,17 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>
> if (locked) {
> qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
> - } else {
> + } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
> env->pmp_state.pmp[pmp_index].cfg_reg = val;
> pmp_update_rule(env, pmp_index);
> + return true;
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpcfg write - out of bounds\n");
> }
> +
> + return false;
> }
>
> static void pmp_decode_napot(target_ulong a, target_ulong *sa,
> @@ -477,16 +480,19 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
> int i;
> uint8_t cfg_val;
> int pmpcfg_nums = 2 << riscv_cpu_mxl(env);
> + bool modified = false;
>
> trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
>
> for (i = 0; i < pmpcfg_nums; i++) {
> cfg_val = (val >> 8 * i) & 0xff;
> - pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
> + modified |= pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
> }
>
> /* If PMP permission of any addr has been changed, flush TLB pages. */
> - tlb_flush(env_cpu(env));
> + if (modified) {
> + tlb_flush(env_cpu(env));
> + }
> }
>
>
> @@ -535,9 +541,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> }
>
> if (!pmp_is_locked(env, addr_index)) {
> - env->pmp_state.pmp[addr_index].addr_reg = val;
> - pmp_update_rule(env, addr_index);
> - tlb_flush(env_cpu(env));
> + if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> + env->pmp_state.pmp[addr_index].addr_reg = val;
> + pmp_update_rule(env, addr_index);
> + tlb_flush(env_cpu(env));
> + }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpaddr write - locked\n");
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] target/riscv: Update pmp_get_tlb_size()
2023-04-13 9:01 ` [PATCH 1/6] target/riscv: Update pmp_get_tlb_size() Weiwei Li
@ 2023-04-18 2:53 ` Alistair Francis
2023-04-18 3:05 ` Weiwei Li
0 siblings, 1 reply; 29+ messages in thread
From: Alistair Francis @ 2023-04-18 2:53 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, richard.henderson, wangjunqiang, lazyparser
On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Not only the matched PMP entry, Any PMP entry that overlap with partial of
> the tlb page may make the regions in that page have different permission
> rights. So all of them should be taken into consideration.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/cpu_helper.c | 7 ++-----
> target/riscv/pmp.c | 34 +++++++++++++++++++++-------------
> target/riscv/pmp.h | 3 +--
> 3 files changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 433ea529b0..075fc0538a 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -703,11 +703,8 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
> }
>
> *prot = pmp_priv_to_page_prot(pmp_priv);
> - if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) {
> - target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
> - target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
> -
> - *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa, tlb_ea);
> + if (tlb_size != NULL) {
> + *tlb_size = pmp_get_tlb_size(env, addr);
> }
>
> return TRANSLATE_SUCCESS;
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 1f5aca42e8..4f9389e73c 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -601,28 +601,36 @@ target_ulong mseccfg_csr_read(CPURISCVState *env)
> }
>
> /*
> - * Calculate the TLB size if the start address or the end address of
> + * Calculate the TLB size if any start address or the end address of
> * PMP entry is presented in the TLB page.
> */
> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
> - target_ulong tlb_sa, target_ulong tlb_ea)
> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr)
> {
> - target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa;
> - target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea;
> + target_ulong pmp_sa;
> + target_ulong pmp_ea;
> + target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
> + target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
> + int i;
> +
> + for (i = 0; i < MAX_RISCV_PMPS; i++) {
> + pmp_sa = env->pmp_state.addr[i].sa;
> + pmp_ea = env->pmp_state.addr[i].ea;
>
> - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) {
> - return TARGET_PAGE_SIZE;
> - } else {
> /*
> - * At this point we have a tlb_size that is the smallest possible size
> - * That fits within a TARGET_PAGE_SIZE and the PMP region.
This comment points out that we should have the smallest region, so
I'm not clear why we need this change. Can you update the commit
description to be clear on why this change is needed and what it
fixes?
Alistair
> - *
> - * If the size is less then TARGET_PAGE_SIZE we drop the size to 1.
> + * If any start address or the end address of PMP entry is presented
> + * in the TLB page and cannot override the whole TLB page we drop the
> + * size to 1.
> * This means the result isn't cached in the TLB and is only used for
> * a single translation.
> */
> - return 1;
> + if (((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) ||
> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) &&
> + !(pmp_sa == tlb_sa && pmp_ea == tlb_ea)) {
> + return 1;
> + }
> }
> +
> + return TARGET_PAGE_SIZE;
> }
>
> /*
> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> index b296ea1fc6..0a7e24750b 100644
> --- a/target/riscv/pmp.h
> +++ b/target/riscv/pmp.h
> @@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> target_ulong size, pmp_priv_t privs,
> pmp_priv_t *allowed_privs,
> target_ulong mode);
> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
> - target_ulong tlb_sa, target_ulong tlb_ea);
> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr);
> void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
> void pmp_update_rule_nums(CPURISCVState *env);
> uint32_t pmp_get_num_rules(CPURISCVState *env);
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/6] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp
2023-04-13 9:01 ` [PATCH 2/6] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp Weiwei Li
@ 2023-04-18 2:54 ` Alistair Francis
0 siblings, 0 replies; 29+ messages in thread
From: Alistair Francis @ 2023-04-18 2:54 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, richard.henderson, wangjunqiang, lazyparser
On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> pmp_get_tlb_size have no relationship with pmp-related permission check
> currently.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu_helper.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 075fc0538a..83c9699a6d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -676,14 +676,11 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
> *
> * @env: CPURISCVState
> * @prot: The returned protection attributes
> - * @tlb_size: TLB page size containing addr. It could be modified after PMP
> - * permission checking. NULL if not set TLB page for addr.
> * @addr: The physical address to be checked permission
> * @access_type: The type of MMU access
> * @mode: Indicates current privilege level.
> */
> -static int get_physical_address_pmp(CPURISCVState *env, int *prot,
> - target_ulong *tlb_size, hwaddr addr,
> +static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
> int size, MMUAccessType access_type,
> int mode)
> {
> @@ -703,9 +700,6 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
> }
>
> *prot = pmp_priv_to_page_prot(pmp_priv);
> - if (tlb_size != NULL) {
> - *tlb_size = pmp_get_tlb_size(env, addr);
> - }
>
> return TRANSLATE_SUCCESS;
> }
> @@ -905,7 +899,7 @@ restart:
> }
>
> int pmp_prot;
> - int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, pte_addr,
> + int pmp_ret = get_physical_address_pmp(env, &pmp_prot, pte_addr,
> sizeof(target_ulong),
> MMU_DATA_LOAD, PRV_S);
> if (pmp_ret != TRANSLATE_SUCCESS) {
> @@ -1300,8 +1294,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> prot &= prot2;
>
> if (ret == TRANSLATE_SUCCESS) {
> - ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
> + ret = get_physical_address_pmp(env, &prot_pmp, pa,
> size, access_type, mode);
> + tlb_size = pmp_get_tlb_size(env, pa);
>
> qemu_log_mask(CPU_LOG_MMU,
> "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> @@ -1333,8 +1328,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> __func__, address, ret, pa, prot);
>
> if (ret == TRANSLATE_SUCCESS) {
> - ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
> + ret = get_physical_address_pmp(env, &prot_pmp, pa,
> size, access_type, mode);
> + tlb_size = pmp_get_tlb_size(env, pa);
>
> qemu_log_mask(CPU_LOG_MMU,
> "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] target/riscv: Update pmp_get_tlb_size()
2023-04-18 2:53 ` Alistair Francis
@ 2023-04-18 3:05 ` Weiwei Li
2023-04-18 5:18 ` LIU Zhiwei
0 siblings, 1 reply; 29+ messages in thread
From: Weiwei Li @ 2023-04-18 3:05 UTC (permalink / raw)
To: Alistair Francis
Cc: liweiwei, qemu-riscv, qemu-devel, palmer, alistair.francis,
bin.meng, dbarboza, zhiwei_liu, richard.henderson, wangjunqiang,
lazyparser
On 2023/4/18 10:53, Alistair Francis wrote:
> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>> Not only the matched PMP entry, Any PMP entry that overlap with partial of
>> the tlb page may make the regions in that page have different permission
>> rights. So all of them should be taken into consideration.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>> target/riscv/cpu_helper.c | 7 ++-----
>> target/riscv/pmp.c | 34 +++++++++++++++++++++-------------
>> target/riscv/pmp.h | 3 +--
>> 3 files changed, 24 insertions(+), 20 deletions(-)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 433ea529b0..075fc0538a 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -703,11 +703,8 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
>> }
>>
>> *prot = pmp_priv_to_page_prot(pmp_priv);
>> - if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) {
>> - target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>> - target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>> -
>> - *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa, tlb_ea);
>> + if (tlb_size != NULL) {
>> + *tlb_size = pmp_get_tlb_size(env, addr);
>> }
>>
>> return TRANSLATE_SUCCESS;
>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>> index 1f5aca42e8..4f9389e73c 100644
>> --- a/target/riscv/pmp.c
>> +++ b/target/riscv/pmp.c
>> @@ -601,28 +601,36 @@ target_ulong mseccfg_csr_read(CPURISCVState *env)
>> }
>>
>> /*
>> - * Calculate the TLB size if the start address or the end address of
>> + * Calculate the TLB size if any start address or the end address of
>> * PMP entry is presented in the TLB page.
>> */
>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>> - target_ulong tlb_sa, target_ulong tlb_ea)
>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr)
>> {
>> - target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa;
>> - target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea;
>> + target_ulong pmp_sa;
>> + target_ulong pmp_ea;
>> + target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>> + target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>> + int i;
>> +
>> + for (i = 0; i < MAX_RISCV_PMPS; i++) {
>> + pmp_sa = env->pmp_state.addr[i].sa;
>> + pmp_ea = env->pmp_state.addr[i].ea;
>>
>> - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) {
>> - return TARGET_PAGE_SIZE;
>> - } else {
>> /*
>> - * At this point we have a tlb_size that is the smallest possible size
>> - * That fits within a TARGET_PAGE_SIZE and the PMP region.
> This comment points out that we should have the smallest region, so
> I'm not clear why we need this change. Can you update the commit
> description to be clear on why this change is needed and what it
> fixes?
This function return tlb_size to 1 to make the tlb uncached. However, In
previous implementation,
only the matched PMP entry of current access address is taken into
consideration. Then, if other PMP entry
that match other address in the same page, we may also cannot cache the
tlb, since different address
in that page may have different permission rights.
Regards,
Weiwei Li
> Alistair
>
>> - *
>> - * If the size is less then TARGET_PAGE_SIZE we drop the size to 1.
>> + * If any start address or the end address of PMP entry is presented
>> + * in the TLB page and cannot override the whole TLB page we drop the
>> + * size to 1.
>> * This means the result isn't cached in the TLB and is only used for
>> * a single translation.
>> */
>> - return 1;
>> + if (((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) ||
>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) &&
>> + !(pmp_sa == tlb_sa && pmp_ea == tlb_ea)) {
>> + return 1;
>> + }
>> }
>> +
>> + return TARGET_PAGE_SIZE;
>> }
>>
>> /*
>> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
>> index b296ea1fc6..0a7e24750b 100644
>> --- a/target/riscv/pmp.h
>> +++ b/target/riscv/pmp.h
>> @@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>> target_ulong size, pmp_priv_t privs,
>> pmp_priv_t *allowed_privs,
>> target_ulong mode);
>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>> - target_ulong tlb_sa, target_ulong tlb_ea);
>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr);
>> void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
>> void pmp_update_rule_nums(CPURISCVState *env);
>> uint32_t pmp_get_num_rules(CPURISCVState *env);
>> --
>> 2.25.1
>>
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/6] target/riscv: Fix PMP related problem
2023-04-13 9:01 [PATCH 0/6] target/riscv: Fix PMP related problem Weiwei Li
` (5 preceding siblings ...)
2023-04-13 9:01 ` [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled Weiwei Li
@ 2023-04-18 3:07 ` LIU Zhiwei
2023-04-18 3:36 ` Weiwei Li
6 siblings, 1 reply; 29+ messages in thread
From: LIU Zhiwei @ 2023-04-18 3:07 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, richard.henderson,
wangjunqiang, lazyparser
On 2023/4/13 17:01, Weiwei Li wrote:
> This patchset tries to fix the PMP bypass problem issue https://gitlab.com/qemu-project/qemu/-/issues/1542
Please add your analysis of this issue here.
By the way, I think this problem is introduced by
https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html
I have commented on how to correct this patch. But by accident, it has
been merged.
Zhiwei
>
> The port is available here:
> https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
>
> Weiwei Li (6):
> target/riscv: Update pmp_get_tlb_size()
> target/riscv: Move pmp_get_tlb_size apart from
> get_physical_address_pmp
> target/riscv: flush tlb when pmpaddr is updated
> target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
> target/riscv: flush tb when PMP entry changes
> accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
> re-filled
>
> accel/tcg/cputlb.c | 7 -----
> target/riscv/cpu_helper.c | 19 ++++---------
> target/riscv/pmp.c | 60 ++++++++++++++++++++++++++-------------
> target/riscv/pmp.h | 3 +-
> 4 files changed, 47 insertions(+), 42 deletions(-)
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/6] target/riscv: Fix PMP related problem
2023-04-18 3:07 ` [PATCH 0/6] target/riscv: Fix PMP related problem LIU Zhiwei
@ 2023-04-18 3:36 ` Weiwei Li
2023-04-18 4:47 ` LIU Zhiwei
0 siblings, 1 reply; 29+ messages in thread
From: Weiwei Li @ 2023-04-18 3:36 UTC (permalink / raw)
To: LIU Zhiwei, Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, richard.henderson,
wangjunqiang, lazyparser
On 2023/4/18 11:07, LIU Zhiwei wrote:
>
> On 2023/4/13 17:01, Weiwei Li wrote:
>> This patchset tries to fix the PMP bypass problem issue
>> https://gitlab.com/qemu-project/qemu/-/issues/1542
>
> Please add your analysis of this issue here.
>
> By the way, I think this problem is introduced by
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html
It seems have no relationship with this commit.
I think there are several problems for this issue:
1. TLB will not be cached only when the access address have matched PMP
entry. So the other address access may hit the TLB(if first access of
the page didn't hit the PMP entry)
and bypass the pmp check. This is fixed by patch 1.
2. Writing to pmpaddr didn't trigger tlb flush. This is fixed by patch 3.
3. The tb isn't flushed when PMP permission changes, so It also may hit
the tb and bypass the changed PMP check for instruction fetch. This is
fixed by patch 5.
4. We set the tlb_size to 1 to make the TLB_INVALID_MASK set. However
this flag will be cleared after fill_tlb, and this will make the host
address be cached, and let the following instruction fetch in the same
tb bypass the PMP check. This is fixed by patch 6.
Regards,
Weiwei Li
>
> I have commented on how to correct this patch. But by accident, it has
> been merged.
>
> Zhiwei
>
>>
>> The port is available here:
>> https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
>>
>> Weiwei Li (6):
>> target/riscv: Update pmp_get_tlb_size()
>> target/riscv: Move pmp_get_tlb_size apart from
>> get_physical_address_pmp
>> target/riscv: flush tlb when pmpaddr is updated
>> target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
>> target/riscv: flush tb when PMP entry changes
>> accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
>> re-filled
>>
>> accel/tcg/cputlb.c | 7 -----
>> target/riscv/cpu_helper.c | 19 ++++---------
>> target/riscv/pmp.c | 60 ++++++++++++++++++++++++++-------------
>> target/riscv/pmp.h | 3 +-
>> 4 files changed, 47 insertions(+), 42 deletions(-)
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/6] target/riscv: Fix PMP related problem
2023-04-18 3:36 ` Weiwei Li
@ 2023-04-18 4:47 ` LIU Zhiwei
2023-04-18 6:11 ` Weiwei Li
0 siblings, 1 reply; 29+ messages in thread
From: LIU Zhiwei @ 2023-04-18 4:47 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, richard.henderson,
wangjunqiang, lazyparser
On 2023/4/18 11:36, Weiwei Li wrote:
>
> On 2023/4/18 11:07, LIU Zhiwei wrote:
>>
>> On 2023/4/13 17:01, Weiwei Li wrote:
>>> This patchset tries to fix the PMP bypass problem issue
>>> https://gitlab.com/qemu-project/qemu/-/issues/1542
>>
>> Please add your analysis of this issue here.
>>
>> By the way, I think this problem is introduced by
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html
>
> It seems have no relationship with this commit.
>
> I think there are several problems for this issue:
>
> 1. TLB will not be cached only when the access address have matched
> PMP entry.
TLB will be filled only when PMP check and PTW check pass.
> So the other address access may hit the TLB(if first access of the
> page didn't hit the PMP entry)
This page will not be filled to TLB if the first access of the page
didn't pass the PMP check.
>
> and bypass the pmp check. This is fixed by patch 1.
Never it should be.
Zhiwei
>
> 2. Writing to pmpaddr didn't trigger tlb flush. This is fixed by
> patch 3.
>
> 3. The tb isn't flushed when PMP permission changes, so It also may
> hit the tb and bypass the changed PMP check for instruction fetch.
> This is fixed by patch 5.
>
> 4. We set the tlb_size to 1 to make the TLB_INVALID_MASK set. However
> this flag will be cleared after fill_tlb, and this will make the host
> address be cached, and let the following instruction fetch in the same
> tb bypass the PMP check. This is fixed by patch 6.
>
> Regards,
>
> Weiwei Li
>
>>
>> I have commented on how to correct this patch. But by accident, it
>> has been merged.
>>
>> Zhiwei
>>
>>>
>>> The port is available here:
>>> https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
>>>
>>> Weiwei Li (6):
>>> target/riscv: Update pmp_get_tlb_size()
>>> target/riscv: Move pmp_get_tlb_size apart from
>>> get_physical_address_pmp
>>> target/riscv: flush tlb when pmpaddr is updated
>>> target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
>>> target/riscv: flush tb when PMP entry changes
>>> accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
>>> re-filled
>>>
>>> accel/tcg/cputlb.c | 7 -----
>>> target/riscv/cpu_helper.c | 19 ++++---------
>>> target/riscv/pmp.c | 60
>>> ++++++++++++++++++++++++++-------------
>>> target/riscv/pmp.h | 3 +-
>>> 4 files changed, 47 insertions(+), 42 deletions(-)
>>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] target/riscv: Update pmp_get_tlb_size()
2023-04-18 3:05 ` Weiwei Li
@ 2023-04-18 5:18 ` LIU Zhiwei
2023-04-18 6:09 ` Weiwei Li
0 siblings, 1 reply; 29+ messages in thread
From: LIU Zhiwei @ 2023-04-18 5:18 UTC (permalink / raw)
To: Weiwei Li, Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, richard.henderson, wangjunqiang, lazyparser
On 2023/4/18 11:05, Weiwei Li wrote:
>
> On 2023/4/18 10:53, Alistair Francis wrote:
>> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>>> Not only the matched PMP entry, Any PMP entry that overlap with
>>> partial of
>>> the tlb page may make the regions in that page have different
>>> permission
>>> rights. So all of them should be taken into consideration.
>>>
>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>> ---
>>> target/riscv/cpu_helper.c | 7 ++-----
>>> target/riscv/pmp.c | 34 +++++++++++++++++++++-------------
>>> target/riscv/pmp.h | 3 +--
>>> 3 files changed, 24 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index 433ea529b0..075fc0538a 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -703,11 +703,8 @@ static int
>>> get_physical_address_pmp(CPURISCVState *env, int *prot,
>>> }
>>>
>>> *prot = pmp_priv_to_page_prot(pmp_priv);
>>> - if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) {
>>> - target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>>> - target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>>> -
>>> - *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa, tlb_ea);
>>> + if (tlb_size != NULL) {
>>> + *tlb_size = pmp_get_tlb_size(env, addr);
>>> }
>>>
>>> return TRANSLATE_SUCCESS;
>>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>>> index 1f5aca42e8..4f9389e73c 100644
>>> --- a/target/riscv/pmp.c
>>> +++ b/target/riscv/pmp.c
>>> @@ -601,28 +601,36 @@ target_ulong mseccfg_csr_read(CPURISCVState *env)
>>> }
>>>
>>> /*
>>> - * Calculate the TLB size if the start address or the end address of
>>> + * Calculate the TLB size if any start address or the end address of
>>> * PMP entry is presented in the TLB page.
>>> */
>>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>>> - target_ulong tlb_sa, target_ulong
>>> tlb_ea)
>>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr)
>>> {
>>> - target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa;
>>> - target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea;
>>> + target_ulong pmp_sa;
>>> + target_ulong pmp_ea;
>>> + target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>>> + target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>>> + int i;
>>> +
>>> + for (i = 0; i < MAX_RISCV_PMPS; i++) {
>>> + pmp_sa = env->pmp_state.addr[i].sa;
>>> + pmp_ea = env->pmp_state.addr[i].ea;
>>>
>>> - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) {
>>> - return TARGET_PAGE_SIZE;
>>> - } else {
>>> /*
>>> - * At this point we have a tlb_size that is the smallest
>>> possible size
>>> - * That fits within a TARGET_PAGE_SIZE and the PMP region.
>> This comment points out that we should have the smallest region, so
>> I'm not clear why we need this change. Can you update the commit
>> description to be clear on why this change is needed and what it
>> fixes?
>
> This function return tlb_size to 1 to make the tlb uncached. However,
> In previous implementation,
>
> only the matched PMP entry of current access address is taken into
> consideration. Then, if other PMP entry
>
> that match other address in the same page, we may also cannot cache
> the tlb, since different address
>
> in that page may have different permission rights.
It doesn't matter. As the tlb size < page size, this tlb will have a
TLB_INVALID_MASK flag and never match.
For this page, every access will repeat the MMU check and TLB fill.
It is not fast, but with no error.
Zhiwei
>
> Regards,
>
> Weiwei Li
>
>> Alistair
>>
>>> - *
>>> - * If the size is less then TARGET_PAGE_SIZE we drop the
>>> size to 1.
>>> + * If any start address or the end address of PMP entry is
>>> presented
>>> + * in the TLB page and cannot override the whole TLB page
>>> we drop the
>>> + * size to 1.
>>> * This means the result isn't cached in the TLB and is
>>> only used for
>>> * a single translation.
>>> */
>>> - return 1;
>>> + if (((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) ||
>>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) &&
>>> + !(pmp_sa == tlb_sa && pmp_ea == tlb_ea)) {
>>> + return 1;
>>> + }
>>> }
>>> +
>>> + return TARGET_PAGE_SIZE;
>>> }
>>>
>>> /*
>>> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
>>> index b296ea1fc6..0a7e24750b 100644
>>> --- a/target/riscv/pmp.h
>>> +++ b/target/riscv/pmp.h
>>> @@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env,
>>> target_ulong addr,
>>> target_ulong size, pmp_priv_t privs,
>>> pmp_priv_t *allowed_privs,
>>> target_ulong mode);
>>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>>> - target_ulong tlb_sa, target_ulong
>>> tlb_ea);
>>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr);
>>> void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
>>> void pmp_update_rule_nums(CPURISCVState *env);
>>> uint32_t pmp_get_num_rules(CPURISCVState *env);
>>> --
>>> 2.25.1
>>>
>>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] target/riscv: Update pmp_get_tlb_size()
2023-04-18 5:18 ` LIU Zhiwei
@ 2023-04-18 6:09 ` Weiwei Li
2023-04-18 7:08 ` LIU Zhiwei
0 siblings, 1 reply; 29+ messages in thread
From: Weiwei Li @ 2023-04-18 6:09 UTC (permalink / raw)
To: LIU Zhiwei, Alistair Francis
Cc: liweiwei, qemu-riscv, qemu-devel, palmer, alistair.francis,
bin.meng, dbarboza, richard.henderson, wangjunqiang, lazyparser
On 2023/4/18 13:18, LIU Zhiwei wrote:
>
> On 2023/4/18 11:05, Weiwei Li wrote:
>>
>> On 2023/4/18 10:53, Alistair Francis wrote:
>>> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>>>> Not only the matched PMP entry, Any PMP entry that overlap with
>>>> partial of
>>>> the tlb page may make the regions in that page have different
>>>> permission
>>>> rights. So all of them should be taken into consideration.
>>>>
>>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>>> ---
>>>> target/riscv/cpu_helper.c | 7 ++-----
>>>> target/riscv/pmp.c | 34 +++++++++++++++++++++-------------
>>>> target/riscv/pmp.h | 3 +--
>>>> 3 files changed, 24 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>> index 433ea529b0..075fc0538a 100644
>>>> --- a/target/riscv/cpu_helper.c
>>>> +++ b/target/riscv/cpu_helper.c
>>>> @@ -703,11 +703,8 @@ static int
>>>> get_physical_address_pmp(CPURISCVState *env, int *prot,
>>>> }
>>>>
>>>> *prot = pmp_priv_to_page_prot(pmp_priv);
>>>> - if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) {
>>>> - target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>>>> - target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>>>> -
>>>> - *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa, tlb_ea);
>>>> + if (tlb_size != NULL) {
>>>> + *tlb_size = pmp_get_tlb_size(env, addr);
>>>> }
>>>>
>>>> return TRANSLATE_SUCCESS;
>>>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>>>> index 1f5aca42e8..4f9389e73c 100644
>>>> --- a/target/riscv/pmp.c
>>>> +++ b/target/riscv/pmp.c
>>>> @@ -601,28 +601,36 @@ target_ulong mseccfg_csr_read(CPURISCVState
>>>> *env)
>>>> }
>>>>
>>>> /*
>>>> - * Calculate the TLB size if the start address or the end address of
>>>> + * Calculate the TLB size if any start address or the end address of
>>>> * PMP entry is presented in the TLB page.
>>>> */
>>>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>>>> - target_ulong tlb_sa, target_ulong
>>>> tlb_ea)
>>>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr)
>>>> {
>>>> - target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa;
>>>> - target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea;
>>>> + target_ulong pmp_sa;
>>>> + target_ulong pmp_ea;
>>>> + target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>>>> + target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < MAX_RISCV_PMPS; i++) {
>>>> + pmp_sa = env->pmp_state.addr[i].sa;
>>>> + pmp_ea = env->pmp_state.addr[i].ea;
>>>>
>>>> - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) {
>>>> - return TARGET_PAGE_SIZE;
>>>> - } else {
>>>> /*
>>>> - * At this point we have a tlb_size that is the smallest
>>>> possible size
>>>> - * That fits within a TARGET_PAGE_SIZE and the PMP region.
>>> This comment points out that we should have the smallest region, so
>>> I'm not clear why we need this change. Can you update the commit
>>> description to be clear on why this change is needed and what it
>>> fixes?
>>
>> This function return tlb_size to 1 to make the tlb uncached. However,
>> In previous implementation,
>>
>> only the matched PMP entry of current access address is taken into
>> consideration. Then, if other PMP entry
>>
>> that match other address in the same page, we may also cannot cache
>> the tlb, since different address
>>
>> in that page may have different permission rights.
>
> It doesn't matter. As the tlb size < page size, this tlb will have a
> TLB_INVALID_MASK flag and never match.
This is what I want. However, tlb size will be page size without this
patch in some cases.
Assuming:
PMP0: sa: 0x80000008 ea: 0x8000000f, rights: R
PMP1: sa: 0, ea: 0xffffffff, rights: RWX
If we try to write data to 0x80000000, PMP1 will be matched, In
previous implementation,
tlb_size will be PMP1 TARGET_PAGE_SIZE and this will be cached, since
only matched PMP is checked ,
and PMP1 covers the whole page. Then when we try to write data to
0x80000008, the tlb will be hit,
and this access bypass the PMP check of PMP0.
Regards,
Weiwei Li
>
> For this page, every access will repeat the MMU check and TLB fill.
>
> It is not fast, but with no error.
>
> Zhiwei
>
>>
>> Regards,
>>
>> Weiwei Li
>>
>>> Alistair
>>>
>>>> - *
>>>> - * If the size is less then TARGET_PAGE_SIZE we drop the
>>>> size to 1.
>>>> + * If any start address or the end address of PMP entry is
>>>> presented
>>>> + * in the TLB page and cannot override the whole TLB page
>>>> we drop the
>>>> + * size to 1.
>>>> * This means the result isn't cached in the TLB and is
>>>> only used for
>>>> * a single translation.
>>>> */
>>>> - return 1;
>>>> + if (((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) ||
>>>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) &&
>>>> + !(pmp_sa == tlb_sa && pmp_ea == tlb_ea)) {
>>>> + return 1;
>>>> + }
>>>> }
>>>> +
>>>> + return TARGET_PAGE_SIZE;
>>>> }
>>>>
>>>> /*
>>>> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
>>>> index b296ea1fc6..0a7e24750b 100644
>>>> --- a/target/riscv/pmp.h
>>>> +++ b/target/riscv/pmp.h
>>>> @@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env,
>>>> target_ulong addr,
>>>> target_ulong size, pmp_priv_t privs,
>>>> pmp_priv_t *allowed_privs,
>>>> target_ulong mode);
>>>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>>>> - target_ulong tlb_sa, target_ulong
>>>> tlb_ea);
>>>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr);
>>>> void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
>>>> void pmp_update_rule_nums(CPURISCVState *env);
>>>> uint32_t pmp_get_num_rules(CPURISCVState *env);
>>>> --
>>>> 2.25.1
>>>>
>>>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/6] target/riscv: Fix PMP related problem
2023-04-18 4:47 ` LIU Zhiwei
@ 2023-04-18 6:11 ` Weiwei Li
0 siblings, 0 replies; 29+ messages in thread
From: Weiwei Li @ 2023-04-18 6:11 UTC (permalink / raw)
To: LIU Zhiwei, qemu-riscv, qemu-devel
Cc: liweiwei, palmer, alistair.francis, bin.meng, dbarboza,
richard.henderson, wangjunqiang, lazyparser
On 2023/4/18 12:47, LIU Zhiwei wrote:
>
> On 2023/4/18 11:36, Weiwei Li wrote:
>>
>> On 2023/4/18 11:07, LIU Zhiwei wrote:
>>>
>>> On 2023/4/13 17:01, Weiwei Li wrote:
>>>> This patchset tries to fix the PMP bypass problem issue
>>>> https://gitlab.com/qemu-project/qemu/-/issues/1542
>>>
>>> Please add your analysis of this issue here.
>>>
>>> By the way, I think this problem is introduced by
>>>
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html
>>
>> It seems have no relationship with this commit.
>>
>> I think there are several problems for this issue:
>>
>> 1. TLB will not be cached only when the access address have matched
>> PMP entry.
> TLB will be filled only when PMP check and PTW check pass.
>> So the other address access may hit the TLB(if first access of the
>> page didn't hit the PMP entry)
> This page will not be filled to TLB if the first access of the page
> didn't pass the PMP check.
I have given an example for this bypass in the replied email of patch 1.
Regards,
Weiwei Li
>>
>> and bypass the pmp check. This is fixed by patch 1.
>
> Never it should be.
>
> Zhiwei
>
>>
>> 2. Writing to pmpaddr didn't trigger tlb flush. This is fixed by
>> patch 3.
>>
>> 3. The tb isn't flushed when PMP permission changes, so It also may
>> hit the tb and bypass the changed PMP check for instruction fetch.
>> This is fixed by patch 5.
>>
>> 4. We set the tlb_size to 1 to make the TLB_INVALID_MASK set. However
>> this flag will be cleared after fill_tlb, and this will make the host
>> address be cached, and let the following instruction fetch in the
>> same tb bypass the PMP check. This is fixed by patch 6.
>>
>> Regards,
>>
>> Weiwei Li
>>
>>>
>>> I have commented on how to correct this patch. But by accident, it
>>> has been merged.
>>>
>>> Zhiwei
>>>
>>>>
>>>> The port is available here:
>>>> https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
>>>>
>>>> Weiwei Li (6):
>>>> target/riscv: Update pmp_get_tlb_size()
>>>> target/riscv: Move pmp_get_tlb_size apart from
>>>> get_physical_address_pmp
>>>> target/riscv: flush tlb when pmpaddr is updated
>>>> target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
>>>> target/riscv: flush tb when PMP entry changes
>>>> accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
>>>> re-filled
>>>>
>>>> accel/tcg/cputlb.c | 7 -----
>>>> target/riscv/cpu_helper.c | 19 ++++---------
>>>> target/riscv/pmp.c | 60
>>>> ++++++++++++++++++++++++++-------------
>>>> target/riscv/pmp.h | 3 +-
>>>> 4 files changed, 47 insertions(+), 42 deletions(-)
>>>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] target/riscv: Update pmp_get_tlb_size()
2023-04-18 6:09 ` Weiwei Li
@ 2023-04-18 7:08 ` LIU Zhiwei
2023-04-18 8:01 ` Weiwei Li
0 siblings, 1 reply; 29+ messages in thread
From: LIU Zhiwei @ 2023-04-18 7:08 UTC (permalink / raw)
To: Weiwei Li, Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, richard.henderson, wangjunqiang, lazyparser
On 2023/4/18 14:09, Weiwei Li wrote:
>
> On 2023/4/18 13:18, LIU Zhiwei wrote:
>>
>> On 2023/4/18 11:05, Weiwei Li wrote:
>>>
>>> On 2023/4/18 10:53, Alistair Francis wrote:
>>>> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn>
>>>> wrote:
>>>>> Not only the matched PMP entry, Any PMP entry that overlap with
>>>>> partial of
>>>>> the tlb page may make the regions in that page have different
>>>>> permission
>>>>> rights. So all of them should be taken into consideration.
>>>>>
>>>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>>>> ---
>>>>> target/riscv/cpu_helper.c | 7 ++-----
>>>>> target/riscv/pmp.c | 34 +++++++++++++++++++++-------------
>>>>> target/riscv/pmp.h | 3 +--
>>>>> 3 files changed, 24 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>>> index 433ea529b0..075fc0538a 100644
>>>>> --- a/target/riscv/cpu_helper.c
>>>>> +++ b/target/riscv/cpu_helper.c
>>>>> @@ -703,11 +703,8 @@ static int
>>>>> get_physical_address_pmp(CPURISCVState *env, int *prot,
>>>>> }
>>>>>
>>>>> *prot = pmp_priv_to_page_prot(pmp_priv);
>>>>> - if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) {
>>>>> - target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>>>>> - target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>>>>> -
>>>>> - *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa,
>>>>> tlb_ea);
>>>>> + if (tlb_size != NULL) {
>>>>> + *tlb_size = pmp_get_tlb_size(env, addr);
>>>>> }
>>>>>
>>>>> return TRANSLATE_SUCCESS;
>>>>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>>>>> index 1f5aca42e8..4f9389e73c 100644
>>>>> --- a/target/riscv/pmp.c
>>>>> +++ b/target/riscv/pmp.c
>>>>> @@ -601,28 +601,36 @@ target_ulong mseccfg_csr_read(CPURISCVState
>>>>> *env)
>>>>> }
>>>>>
>>>>> /*
>>>>> - * Calculate the TLB size if the start address or the end address of
>>>>> + * Calculate the TLB size if any start address or the end address of
>>>>> * PMP entry is presented in the TLB page.
>>>>> */
>>>>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>>>>> - target_ulong tlb_sa, target_ulong
>>>>> tlb_ea)
>>>>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr)
>>>>> {
>>>>> - target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa;
>>>>> - target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea;
>>>>> + target_ulong pmp_sa;
>>>>> + target_ulong pmp_ea;
>>>>> + target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>>>>> + target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>>>>> + int i;
>>>>> +
>>>>> + for (i = 0; i < MAX_RISCV_PMPS; i++) {
>>>>> + pmp_sa = env->pmp_state.addr[i].sa;
>>>>> + pmp_ea = env->pmp_state.addr[i].ea;
>>>>>
>>>>> - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) {
>>>>> - return TARGET_PAGE_SIZE;
>>>>> - } else {
>>>>> /*
>>>>> - * At this point we have a tlb_size that is the smallest
>>>>> possible size
>>>>> - * That fits within a TARGET_PAGE_SIZE and the PMP region.
>>>> This comment points out that we should have the smallest region, so
>>>> I'm not clear why we need this change. Can you update the commit
>>>> description to be clear on why this change is needed and what it
>>>> fixes?
>>>
>>> This function return tlb_size to 1 to make the tlb uncached.
>>> However, In previous implementation,
>>>
>>> only the matched PMP entry of current access address is taken into
>>> consideration. Then, if other PMP entry
>>>
>>> that match other address in the same page, we may also cannot cache
>>> the tlb, since different address
>>>
>>> in that page may have different permission rights.
>>
>> It doesn't matter. As the tlb size < page size, this tlb will have a
>> TLB_INVALID_MASK flag and never match.
>
> This is what I want. However, tlb size will be page size without this
> patch in some cases.
>
> Assuming:
>
> PMP0: sa: 0x80000008 ea: 0x8000000f, rights: R
>
> PMP1: sa: 0, ea: 0xffffffff, rights: RWX
>
> If we try to write data to 0x80000000, PMP1 will be matched, In
> previous implementation,
>
> tlb_size will be PMP1 TARGET_PAGE_SIZE and this will be cached, since
> only matched PMP is checked ,
>
> and PMP1 covers the whole page. Then when we try to write data to
> 0x80000008, the tlb will be hit,
>
> and this access bypass the PMP check of PMP0.
I see. You are fixing the priority of PMP check rule.
You can still pass the matched index to pmp_get_tlb_size. And only
check first match index PMP rules.
>
> Regards,
>
> Weiwei Li
>
>>
>> For this page, every access will repeat the MMU check and TLB fill.
>>
>> It is not fast, but with no error.
>>
>> Zhiwei
>>
>>>
>>> Regards,
>>>
>>> Weiwei Li
>>>
>>>> Alistair
>>>>
>>>>> - *
>>>>> - * If the size is less then TARGET_PAGE_SIZE we drop the
>>>>> size to 1.
>>>>> + * If any start address or the end address of PMP entry
>>>>> is presented
>>>>> + * in the TLB page and cannot override the whole TLB page
>>>>> we drop the
>>>>> + * size to 1.
>>>>> * This means the result isn't cached in the TLB and is
>>>>> only used for
>>>>> * a single translation.
>>>>> */
>>>>> - return 1;
>>>>> + if (((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) ||
>>>>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) &&
>>>>> + !(pmp_sa == tlb_sa && pmp_ea == tlb_ea)) {
>>>>> + return 1;
>>>>> + }
>>>>> }
>>>>> +
>>>>> + return TARGET_PAGE_SIZE;
>>>>> }
>>>>>
>>>>> /*
>>>>> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
>>>>> index b296ea1fc6..0a7e24750b 100644
>>>>> --- a/target/riscv/pmp.h
>>>>> +++ b/target/riscv/pmp.h
>>>>> @@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env,
>>>>> target_ulong addr,
>>>>> target_ulong size, pmp_priv_t privs,
>>>>> pmp_priv_t *allowed_privs,
>>>>> target_ulong mode);
>>>>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>>>>> - target_ulong tlb_sa, target_ulong
>>>>> tlb_ea);
>>>>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong
>>>>> addr);
In this way, you need not to change the declaration of pmp_get_tlb_size.
Otherwise,
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Zhiwei
>>>>> void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
>>>>> void pmp_update_rule_nums(CPURISCVState *env);
>>>>> uint32_t pmp_get_num_rules(CPURISCVState *env);
>>>>> --
>>>>> 2.25.1
>>>>>
>>>>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated
2023-04-13 9:01 ` [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated Weiwei Li
2023-04-18 2:36 ` Alistair Francis
@ 2023-04-18 7:11 ` LIU Zhiwei
2023-04-18 8:13 ` Weiwei Li
1 sibling, 1 reply; 29+ messages in thread
From: LIU Zhiwei @ 2023-04-18 7:11 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, richard.henderson,
wangjunqiang, lazyparser
On 2023/4/13 17:01, Weiwei Li wrote:
> TLB should be flushed not only for pmpcfg csr changes, but also for
> pmpaddr csr changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/pmp.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 4f9389e73c..6d4813806b 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -537,6 +537,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> if (!pmp_is_locked(env, addr_index)) {
> env->pmp_state.pmp[addr_index].addr_reg = val;
> pmp_update_rule(env, addr_index);
> + tlb_flush(env_cpu(env));
Can we always flush tlb in pmp_update_rule?
Zhiwei
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpaddr write - locked\n");
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
2023-04-13 9:01 ` [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes Weiwei Li
2023-04-18 2:39 ` Alistair Francis
@ 2023-04-18 7:14 ` LIU Zhiwei
1 sibling, 0 replies; 29+ messages in thread
From: LIU Zhiwei @ 2023-04-18 7:14 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, richard.henderson,
wangjunqiang, lazyparser
On 2023/4/13 17:01, Weiwei Li wrote:
> TLB needn't be flushed when pmpcfg/pmpaddr don't changes.
If we flush the tlb in pmp_update_rules, we don't need this patch.
Zhiwei
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/pmp.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 6d4813806b..aced23c4d5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -26,7 +26,7 @@
> #include "trace.h"
> #include "exec/exec-all.h"
>
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> +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);
> static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index);
> @@ -83,7 +83,7 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
> * Accessor to set the cfg reg for a specific PMP/HART
> * Bounds checks and relevant lock bit.
> */
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> +static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> {
> if (pmp_index < MAX_RISCV_PMPS) {
> bool locked = true;
> @@ -119,14 +119,17 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>
> if (locked) {
> qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
> - } else {
> + } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
> env->pmp_state.pmp[pmp_index].cfg_reg = val;
> pmp_update_rule(env, pmp_index);
> + return true;
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpcfg write - out of bounds\n");
> }
> +
> + return false;
> }
>
> static void pmp_decode_napot(target_ulong a, target_ulong *sa,
> @@ -477,16 +480,19 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
> int i;
> uint8_t cfg_val;
> int pmpcfg_nums = 2 << riscv_cpu_mxl(env);
> + bool modified = false;
>
> trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
>
> for (i = 0; i < pmpcfg_nums; i++) {
> cfg_val = (val >> 8 * i) & 0xff;
> - pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
> + modified |= pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
> }
>
> /* If PMP permission of any addr has been changed, flush TLB pages. */
> - tlb_flush(env_cpu(env));
> + if (modified) {
> + tlb_flush(env_cpu(env));
> + }
> }
>
>
> @@ -535,9 +541,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> }
>
> if (!pmp_is_locked(env, addr_index)) {
> - env->pmp_state.pmp[addr_index].addr_reg = val;
> - pmp_update_rule(env, addr_index);
> - tlb_flush(env_cpu(env));
> + if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> + env->pmp_state.pmp[addr_index].addr_reg = val;
> + pmp_update_rule(env, addr_index);
> + tlb_flush(env_cpu(env));
> + }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
> "ignoring pmpaddr write - locked\n");
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled
2023-04-17 16:25 ` Daniel Henrique Barboza
2023-04-18 0:48 ` Weiwei Li
@ 2023-04-18 7:18 ` Richard Henderson
2023-04-18 7:36 ` Richard Henderson
1 sibling, 1 reply; 29+ messages in thread
From: Richard Henderson @ 2023-04-18 7:18 UTC (permalink / raw)
To: Daniel Henrique Barboza, Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang,
lazyparser
On 4/17/23 18:25, Daniel Henrique Barboza wrote:
>
>
> On 4/13/23 06:01, Weiwei Li wrote:
>> When PMP entry overlap part of the page, we'll set the tlb_size to 1, and
>> this will make the address set with TLB_INVALID_MASK to make the page
>> un-cached. However, if we clear TLB_INVALID_MASK when TLB is re-filled, then
>> the TLB host address will be cached, and the following instructions can use
>> this host address directly which may lead to the bypass of PMP related check.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>
> For this commit I believe it's worth mentioning that it's partially reverting
> commit c3c8bf579b431b6b ("accel/tcg: Suppress auto-invalidate in
> probe_access_internal") that was made to handle a particularity/quirk that was
> present in s390x code.
>
> At first glance this patch seems benign but we must make sure that no other
> assumptions were made with this particular change in probe_access_internal().
>
>
> Thanks,
>
> Daniel
>
>> accel/tcg/cputlb.c | 7 -------
>> 1 file changed, 7 deletions(-)
>>
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index e984a98dc4..d0bf996405 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState *env, target_ulong
>> addr,
>> /* TLB resize via tlb_fill may have moved the entry. */
>> index = tlb_index(env, mmu_idx, addr);
>> entry = tlb_entry(env, mmu_idx, addr);
>> -
>> - /*
>> - * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
>> - * to force the next access through tlb_fill. We've just
>> - * called tlb_fill, so we know that this entry *is* valid.
>> - */
>> - flags &= ~TLB_INVALID_MASK;
I missed the original patch, but this is definitely wrong.
Clearing this bit locally (!) is correct because we want to inform the caller of
probe_access_* that the access is valid. We know that it is valid because we have just
queried tlb_fill (and thus for riscv, PMP).
Clearing the bit locally does *not* cause the tlb entry to be cached -- the INVALID bit is
still set within the tlb entry. The next access will again go through tlb_fill.
What is the original problem you are seeing? The commit message does not say.
r~
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] target/riscv: flush tb when PMP entry changes
2023-04-13 9:01 ` [PATCH 5/6] target/riscv: flush tb when PMP entry changes Weiwei Li
@ 2023-04-18 7:28 ` LIU Zhiwei
0 siblings, 0 replies; 29+ messages in thread
From: LIU Zhiwei @ 2023-04-18 7:28 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, richard.henderson,
wangjunqiang, lazyparser
On 2023/4/13 17:01, Weiwei Li wrote:
> The translation block may also be affected when PMP entry changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/pmp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index aced23c4d5..c2db52361f 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -25,6 +25,7 @@
> #include "cpu.h"
> #include "trace.h"
> #include "exec/exec-all.h"
> +#include "exec/tb-flush.h"
>
> static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> uint8_t val);
> @@ -492,6 +493,7 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
> /* If PMP permission of any addr has been changed, flush TLB pages. */
> if (modified) {
> tlb_flush(env_cpu(env));
> + tb_flush(env_cpu(env));
Move this to pmp_update_rules.
Zhiwei
> }
> }
>
> @@ -545,6 +547,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> env->pmp_state.pmp[addr_index].addr_reg = val;
> pmp_update_rule(env, addr_index);
> tlb_flush(env_cpu(env));
> + tb_flush(env_cpu(env));
> }
> } else {
> qemu_log_mask(LOG_GUEST_ERROR,
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled
2023-04-18 7:18 ` Richard Henderson
@ 2023-04-18 7:36 ` Richard Henderson
2023-04-18 8:18 ` Weiwei Li
0 siblings, 1 reply; 29+ messages in thread
From: Richard Henderson @ 2023-04-18 7:36 UTC (permalink / raw)
To: Daniel Henrique Barboza, Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang,
lazyparser
On 4/18/23 09:18, Richard Henderson wrote:
>>> - /*
>>> - * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
>>> - * to force the next access through tlb_fill. We've just
>>> - * called tlb_fill, so we know that this entry *is* valid.
>>> - */
>>> - flags &= ~TLB_INVALID_MASK;
>
>
> I missed the original patch, but this is definitely wrong.
>
> Clearing this bit locally (!) is correct because we want to inform the caller of
> probe_access_* that the access is valid. We know that it is valid because we have just
> queried tlb_fill (and thus for riscv, PMP).
>
> Clearing the bit locally does *not* cause the tlb entry to be cached -- the INVALID bit is
> still set within the tlb entry. The next access will again go through tlb_fill.
>
> What is the original problem you are seeing? The commit message does not say.
From https://lore.kernel.org/qemu-devel/3ace9e9e-91cf-36e6-a18f-494fd44dffab@iscas.ac.cn/
I see that it is a problem with execution.
By eye, it appears that get_page_addr_code_hostp needs adjustment, e.g.
(void)probe_access_internal(env, addr, 1, MMU_INST_FETCH,
cpu_mmu_index(env, true), false, &p, &full, 0);
if (p == NULL) {
return -1;
}
+ if (full->lg_page_size < TARGET_PAGE_BITS) {
+ return -1;
+ }
if (hostp) {
*hostp = p;
}
It seems like we could do slightly better than this, perhaps by single-stepping through
such a page, but surely this edge case is so uncommon as to not make it worthwhile to
consider.
r~
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] target/riscv: Update pmp_get_tlb_size()
2023-04-18 7:08 ` LIU Zhiwei
@ 2023-04-18 8:01 ` Weiwei Li
0 siblings, 0 replies; 29+ messages in thread
From: Weiwei Li @ 2023-04-18 8:01 UTC (permalink / raw)
To: LIU Zhiwei, Alistair Francis
Cc: liweiwei, qemu-riscv, qemu-devel, palmer, alistair.francis,
bin.meng, dbarboza, richard.henderson, wangjunqiang, lazyparser
On 2023/4/18 15:08, LIU Zhiwei wrote:
>
> On 2023/4/18 14:09, Weiwei Li wrote:
>>
>> On 2023/4/18 13:18, LIU Zhiwei wrote:
>>>
>>> On 2023/4/18 11:05, Weiwei Li wrote:
>>>>
>>>> On 2023/4/18 10:53, Alistair Francis wrote:
>>>>> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn>
>>>>> wrote:
>>>>>> Not only the matched PMP entry, Any PMP entry that overlap with
>>>>>> partial of
>>>>>> the tlb page may make the regions in that page have different
>>>>>> permission
>>>>>> rights. So all of them should be taken into consideration.
>>>>>>
>>>>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>>>>> ---
>>>>>> target/riscv/cpu_helper.c | 7 ++-----
>>>>>> target/riscv/pmp.c | 34 +++++++++++++++++++++-------------
>>>>>> target/riscv/pmp.h | 3 +--
>>>>>> 3 files changed, 24 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>>>> index 433ea529b0..075fc0538a 100644
>>>>>> --- a/target/riscv/cpu_helper.c
>>>>>> +++ b/target/riscv/cpu_helper.c
>>>>>> @@ -703,11 +703,8 @@ static int
>>>>>> get_physical_address_pmp(CPURISCVState *env, int *prot,
>>>>>> }
>>>>>>
>>>>>> *prot = pmp_priv_to_page_prot(pmp_priv);
>>>>>> - if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) {
>>>>>> - target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>>>>>> - target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>>>>>> -
>>>>>> - *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa,
>>>>>> tlb_ea);
>>>>>> + if (tlb_size != NULL) {
>>>>>> + *tlb_size = pmp_get_tlb_size(env, addr);
>>>>>> }
>>>>>>
>>>>>> return TRANSLATE_SUCCESS;
>>>>>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>>>>>> index 1f5aca42e8..4f9389e73c 100644
>>>>>> --- a/target/riscv/pmp.c
>>>>>> +++ b/target/riscv/pmp.c
>>>>>> @@ -601,28 +601,36 @@ target_ulong mseccfg_csr_read(CPURISCVState
>>>>>> *env)
>>>>>> }
>>>>>>
>>>>>> /*
>>>>>> - * Calculate the TLB size if the start address or the end
>>>>>> address of
>>>>>> + * Calculate the TLB size if any start address or the end
>>>>>> address of
>>>>>> * PMP entry is presented in the TLB page.
>>>>>> */
>>>>>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>>>>>> - target_ulong tlb_sa, target_ulong
>>>>>> tlb_ea)
>>>>>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong
>>>>>> addr)
>>>>>> {
>>>>>> - target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa;
>>>>>> - target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea;
>>>>>> + target_ulong pmp_sa;
>>>>>> + target_ulong pmp_ea;
>>>>>> + target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1);
>>>>>> + target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1;
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < MAX_RISCV_PMPS; i++) {
>>>>>> + pmp_sa = env->pmp_state.addr[i].sa;
>>>>>> + pmp_ea = env->pmp_state.addr[i].ea;
>>>>>>
>>>>>> - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) {
>>>>>> - return TARGET_PAGE_SIZE;
>>>>>> - } else {
>>>>>> /*
>>>>>> - * At this point we have a tlb_size that is the smallest
>>>>>> possible size
>>>>>> - * That fits within a TARGET_PAGE_SIZE and the PMP region.
>>>>> This comment points out that we should have the smallest region, so
>>>>> I'm not clear why we need this change. Can you update the commit
>>>>> description to be clear on why this change is needed and what it
>>>>> fixes?
>>>>
>>>> This function return tlb_size to 1 to make the tlb uncached.
>>>> However, In previous implementation,
>>>>
>>>> only the matched PMP entry of current access address is taken into
>>>> consideration. Then, if other PMP entry
>>>>
>>>> that match other address in the same page, we may also cannot
>>>> cache the tlb, since different address
>>>>
>>>> in that page may have different permission rights.
>>>
>>> It doesn't matter. As the tlb size < page size, this tlb will have a
>>> TLB_INVALID_MASK flag and never match.
>>
>> This is what I want. However, tlb size will be page size without
>> this patch in some cases.
>>
>> Assuming:
>>
>> PMP0: sa: 0x80000008 ea: 0x8000000f, rights: R
>>
>> PMP1: sa: 0, ea: 0xffffffff, rights: RWX
>>
>> If we try to write data to 0x80000000, PMP1 will be matched, In
>> previous implementation,
>>
>> tlb_size will be PMP1 TARGET_PAGE_SIZE and this will be cached, since
>> only matched PMP is checked ,
>>
>> and PMP1 covers the whole page. Then when we try to write data to
>> 0x80000008, the tlb will be hit,
>>
>> and this access bypass the PMP check of PMP0.
>
> I see. You are fixing the priority of PMP check rule.
Yeah. You can see it as a priority problem.
>
> You can still pass the matched index to pmp_get_tlb_size. And only
> check first match index PMP rules.
Yeah. only the PMP rules before the matched PMP need be checked.
However, I prefers separate the matched index from pmp_get_tlb_size,
then we can separate
this function from get_physical_address_pmp (not all pmp check needs
caculate the tlb size).
Maybe we can improve the check to following code:
if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) {
return TARGET_PAGE_SIZE;
} else if ((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) || (pmp_ea >=
tlb_sa && pmp_ea <= tlb_ea)) {
return 1;
}
then the checked index will not be larger than matched index(the matched
case will match one of the above conditions).
Regards,
Weiwei Li
>
>>
>> Regards,
>>
>> Weiwei Li
>>
>>>
>>> For this page, every access will repeat the MMU check and TLB fill.
>>>
>>> It is not fast, but with no error.
>>>
>>> Zhiwei
>>>
>>>>
>>>> Regards,
>>>>
>>>> Weiwei Li
>>>>
>>>>> Alistair
>>>>>
>>>>>> - *
>>>>>> - * If the size is less then TARGET_PAGE_SIZE we drop the
>>>>>> size to 1.
>>>>>> + * If any start address or the end address of PMP entry
>>>>>> is presented
>>>>>> + * in the TLB page and cannot override the whole TLB
>>>>>> page we drop the
>>>>>> + * size to 1.
>>>>>> * This means the result isn't cached in the TLB and is
>>>>>> only used for
>>>>>> * a single translation.
>>>>>> */
>>>>>> - return 1;
>>>>>> + if (((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) ||
>>>>>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) &&
>>>>>> + !(pmp_sa == tlb_sa && pmp_ea == tlb_ea)) {
>>>>>> + return 1;
>>>>>> + }
>>>>>> }
>>>>>> +
>>>>>> + return TARGET_PAGE_SIZE;
>>>>>> }
>>>>>>
>>>>>> /*
>>>>>> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
>>>>>> index b296ea1fc6..0a7e24750b 100644
>>>>>> --- a/target/riscv/pmp.h
>>>>>> +++ b/target/riscv/pmp.h
>>>>>> @@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env,
>>>>>> target_ulong addr,
>>>>>> target_ulong size, pmp_priv_t privs,
>>>>>> pmp_priv_t *allowed_privs,
>>>>>> target_ulong mode);
>>>>>> -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index,
>>>>>> - target_ulong tlb_sa, target_ulong
>>>>>> tlb_ea);
>>>>>> +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong
>>>>>> addr);
>
> In this way, you need not to change the declaration of pmp_get_tlb_size.
>
> Otherwise,
> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>
> Zhiwei
>
>>>>>> void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index);
>>>>>> void pmp_update_rule_nums(CPURISCVState *env);
>>>>>> uint32_t pmp_get_num_rules(CPURISCVState *env);
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated
2023-04-18 7:11 ` LIU Zhiwei
@ 2023-04-18 8:13 ` Weiwei Li
0 siblings, 0 replies; 29+ messages in thread
From: Weiwei Li @ 2023-04-18 8:13 UTC (permalink / raw)
To: LIU Zhiwei, qemu-riscv, qemu-devel
Cc: liweiwei, palmer, alistair.francis, bin.meng, dbarboza,
richard.henderson, wangjunqiang, lazyparser
On 2023/4/18 15:11, LIU Zhiwei wrote:
>
> On 2023/4/13 17:01, Weiwei Li wrote:
>> TLB should be flushed not only for pmpcfg csr changes, but also for
>> pmpaddr csr changes.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>> target/riscv/pmp.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>> index 4f9389e73c..6d4813806b 100644
>> --- a/target/riscv/pmp.c
>> +++ b/target/riscv/pmp.c
>> @@ -537,6 +537,7 @@ void pmpaddr_csr_write(CPURISCVState *env,
>> uint32_t addr_index,
>> if (!pmp_is_locked(env, addr_index)) {
>> env->pmp_state.pmp[addr_index].addr_reg = val;
>> pmp_update_rule(env, addr_index);
>> + tlb_flush(env_cpu(env));
>
> Can we always flush tlb in pmp_update_rule?
I considered this way, However this may lead to tlb being flushed
several times when we update pmpcfg csrs.
Regards,
Weiwei Li
>
> Zhiwei
>
>> } else {
>> qemu_log_mask(LOG_GUEST_ERROR,
>> "ignoring pmpaddr write - locked\n");
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled
2023-04-18 7:36 ` Richard Henderson
@ 2023-04-18 8:18 ` Weiwei Li
0 siblings, 0 replies; 29+ messages in thread
From: Weiwei Li @ 2023-04-18 8:18 UTC (permalink / raw)
To: Richard Henderson, Daniel Henrique Barboza, qemu-riscv,
qemu-devel
Cc: liweiwei, palmer, alistair.francis, bin.meng, zhiwei_liu,
wangjunqiang, lazyparser
On 2023/4/18 15:36, Richard Henderson wrote:
> On 4/18/23 09:18, Richard Henderson wrote:
>>>> - /*
>>>> - * With PAGE_WRITE_INV, we set TLB_INVALID_MASK
>>>> immediately,
>>>> - * to force the next access through tlb_fill. We've just
>>>> - * called tlb_fill, so we know that this entry *is*
>>>> valid.
>>>> - */
>>>> - flags &= ~TLB_INVALID_MASK;
>>
>>
>> I missed the original patch, but this is definitely wrong.
>>
>> Clearing this bit locally (!) is correct because we want to inform
>> the caller of probe_access_* that the access is valid. We know that
>> it is valid because we have just queried tlb_fill (and thus for
>> riscv, PMP).
>>
>> Clearing the bit locally does *not* cause the tlb entry to be cached
>> -- the INVALID bit is still set within the tlb entry. The next access
>> will again go through tlb_fill.
>>
>> What is the original problem you are seeing? The commit message does
>> not say.
>
> From
> https://lore.kernel.org/qemu-devel/3ace9e9e-91cf-36e6-a18f-494fd44dffab@iscas.ac.cn/
> I see that it is a problem with execution.
Yeah. I found this problem in PMP check for instruction fetch.
>
> By eye, it appears that get_page_addr_code_hostp needs adjustment, e.g.
>
> (void)probe_access_internal(env, addr, 1, MMU_INST_FETCH,
> cpu_mmu_index(env, true), false, &p,
> &full, 0);
> if (p == NULL) {
> return -1;
> }
> + if (full->lg_page_size < TARGET_PAGE_BITS) {
> + return -1;
> + }
> if (hostp) {
> *hostp = p;
> }
>
> It seems like we could do slightly better than this, perhaps by
> single-stepping through such a page, but surely this edge case is so
> uncommon as to not make it worthwhile to consider.
OK. I'll update and test it later.
Regards,
Weiwei Li
>
>
> r~
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2023-04-18 8:19 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-13 9:01 [PATCH 0/6] target/riscv: Fix PMP related problem Weiwei Li
2023-04-13 9:01 ` [PATCH 1/6] target/riscv: Update pmp_get_tlb_size() Weiwei Li
2023-04-18 2:53 ` Alistair Francis
2023-04-18 3:05 ` Weiwei Li
2023-04-18 5:18 ` LIU Zhiwei
2023-04-18 6:09 ` Weiwei Li
2023-04-18 7:08 ` LIU Zhiwei
2023-04-18 8:01 ` Weiwei Li
2023-04-13 9:01 ` [PATCH 2/6] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp Weiwei Li
2023-04-18 2:54 ` Alistair Francis
2023-04-13 9:01 ` [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated Weiwei Li
2023-04-18 2:36 ` Alistair Francis
2023-04-18 7:11 ` LIU Zhiwei
2023-04-18 8:13 ` Weiwei Li
2023-04-13 9:01 ` [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes Weiwei Li
2023-04-18 2:39 ` Alistair Francis
2023-04-18 7:14 ` LIU Zhiwei
2023-04-13 9:01 ` [PATCH 5/6] target/riscv: flush tb when PMP entry changes Weiwei Li
2023-04-18 7:28 ` LIU Zhiwei
2023-04-13 9:01 ` [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled Weiwei Li
2023-04-17 16:25 ` Daniel Henrique Barboza
2023-04-18 0:48 ` Weiwei Li
2023-04-18 7:18 ` Richard Henderson
2023-04-18 7:36 ` Richard Henderson
2023-04-18 8:18 ` Weiwei Li
2023-04-18 3:07 ` [PATCH 0/6] target/riscv: Fix PMP related problem LIU Zhiwei
2023-04-18 3:36 ` Weiwei Li
2023-04-18 4:47 ` LIU Zhiwei
2023-04-18 6:11 ` Weiwei Li
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).