* [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits
@ 2021-02-21 14:01 Jim Shu
2021-02-21 14:01 ` [PATCH 1/3] target/riscv: propagate PMP permission to TLB page Jim Shu
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Jim Shu @ 2021-02-21 14:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel; +Cc: listair.Francis, palmer, Jim Shu, sagark, kbastian
Sorry for sending this patch set again.
The cover letter of my previous mail doesn't add cc list.
---
Current implementation of PMP permission checking only has effect when
softmmu's TLB miss. PMP checking is bypassed when TLB hits because TLB page
permission isn't affected by PMP permission.
To fix this issue, this patch set addes the feature to propagate PMP
permission to the TLB page and flush TLB pages if PMP permission has
been changed.
The patch set is tested on Zephyr RTOS userspace testsuite on QEMU riscv32
virt machine.
Jim Shu (3):
target/riscv: propagate PMP permission to TLB page
target/riscv: add log of PMP permission checking
target/riscv: flush TLB pages if PMP permission has been changed
target/riscv/cpu_helper.c | 96 ++++++++++++++++++++++++++++++---------
target/riscv/pmp.c | 84 +++++++++++++++++++++++++---------
target/riscv/pmp.h | 4 +-
3 files changed, 141 insertions(+), 43 deletions(-)
--
2.30.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] target/riscv: propagate PMP permission to TLB page
2021-02-21 14:01 [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits Jim Shu
@ 2021-02-21 14:01 ` Jim Shu
2021-03-16 20:01 ` Alistair Francis
2021-02-21 14:01 ` [PATCH 2/3] target/riscv: add log of PMP permission checking Jim Shu
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Jim Shu @ 2021-02-21 14:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: Alistair Francis, Palmer Dabbelt, Jim Shu, Sagar Karandikar,
Bastian Koppelmann
Currently, PMP permission checking of TLB page is bypassed if TLB hits
Fix it by propagating PMP permission to TLB page permission.
PMP permission checking also use MMU-style API to change TLB permission
and size.
Signed-off-by: Jim Shu <cwshu@andestech.com>
---
target/riscv/cpu_helper.c | 84 +++++++++++++++++++++++++++++----------
target/riscv/pmp.c | 80 +++++++++++++++++++++++++++----------
target/riscv/pmp.h | 4 +-
3 files changed, 125 insertions(+), 43 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 2f43939fb6..f6ac63bf0e 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -280,6 +280,49 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
env->load_res = -1;
}
+/*
+ * get_physical_address_pmp - check PMP permission for this physical address
+ *
+ * Match the PMP region and check permission for this physical address and it's
+ * TLB page. Returns 0 if the permission checking was successful
+ *
+ * @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,
+ int size, MMUAccessType access_type,
+ int mode)
+{
+ pmp_priv_t pmp_priv;
+ target_ulong tlb_size_pmp = 0;
+
+ if (!riscv_feature(env, RISCV_FEATURE_PMP)) {
+ *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+ return TRANSLATE_SUCCESS;
+ }
+
+ if (!pmp_hart_has_privs(env, addr, size, 1 << access_type, &pmp_priv,
+ mode)) {
+ *prot = 0;
+ return TRANSLATE_PMP_FAIL;
+ }
+
+ *prot = pmp_priv_to_page_prot(pmp_priv);
+ if (tlb_size != NULL) {
+ if (pmp_is_range_in_tlb(env, addr & ~(*tlb_size - 1), &tlb_size_pmp)) {
+ *tlb_size = tlb_size_pmp;
+ }
+ }
+
+ return TRANSLATE_SUCCESS;
+}
+
/* get_physical_address - get the physical address for this virtual address
*
* Do a page table walk to obtain the physical address corresponding to a
@@ -442,9 +485,11 @@ restart:
pte_addr = base + idx * ptesize;
}
- if (riscv_feature(env, RISCV_FEATURE_PMP) &&
- !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
- 1 << MMU_DATA_LOAD, PRV_S)) {
+ int pmp_prot;
+ int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, pte_addr,
+ sizeof(target_ulong),
+ MMU_DATA_LOAD, PRV_S);
+ if (pmp_ret != TRANSLATE_SUCCESS) {
return TRANSLATE_PMP_FAIL;
}
@@ -682,13 +727,14 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
#ifndef CONFIG_USER_ONLY
vaddr im_address;
hwaddr pa = 0;
- int prot, prot2;
+ int prot, prot2, prot_pmp;
bool pmp_violation = false;
bool first_stage_error = true;
bool two_stage_lookup = false;
int ret = TRANSLATE_FAIL;
int mode = mmu_idx;
- target_ulong tlb_size = 0;
+ /* default TLB page size */
+ target_ulong tlb_size = TARGET_PAGE_SIZE;
env->guest_phys_fault_addr = 0;
@@ -745,10 +791,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
prot &= prot2;
- if (riscv_feature(env, RISCV_FEATURE_PMP) &&
- (ret == TRANSLATE_SUCCESS) &&
- !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
- ret = TRANSLATE_PMP_FAIL;
+ if (ret == TRANSLATE_SUCCESS) {
+ ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
+ size, access_type, mode);
+ prot &= prot_pmp;
}
if (ret != TRANSLATE_SUCCESS) {
@@ -771,25 +817,21 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
"%s address=%" VADDR_PRIx " ret %d physical "
TARGET_FMT_plx " prot %d\n",
__func__, address, ret, pa, prot);
- }
- if (riscv_feature(env, RISCV_FEATURE_PMP) &&
- (ret == TRANSLATE_SUCCESS) &&
- !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
- ret = TRANSLATE_PMP_FAIL;
+ if (ret == TRANSLATE_SUCCESS) {
+ ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
+ size, access_type, mode);
+ prot &= prot_pmp;
+ }
}
+
if (ret == TRANSLATE_PMP_FAIL) {
pmp_violation = true;
}
if (ret == TRANSLATE_SUCCESS) {
- if (pmp_is_range_in_tlb(env, pa & TARGET_PAGE_MASK, &tlb_size)) {
- tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
- prot, mmu_idx, tlb_size);
- } else {
- tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
- prot, mmu_idx, TARGET_PAGE_SIZE);
- }
+ tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
+ prot, mmu_idx, tlb_size);
return true;
} else if (probe) {
return false;
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 80d0334e1b..ebd874cde3 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -217,6 +217,35 @@ static int pmp_is_in_range(CPURISCVState *env, int pmp_index, target_ulong addr)
return result;
}
+/*
+ * Check if the address has required RWX privs when no PMP entry is matched.
+ */
+static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr,
+ target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
+ target_ulong mode)
+{
+ bool ret;
+
+ if ((!riscv_feature(env, RISCV_FEATURE_PMP)) || (mode == PRV_M)) {
+ /*
+ * Privileged spec v1.10 states if HW doesn't implement any PMP entry
+ * or no PMP entry matches an M-Mode access, the access succeeds.
+ */
+ ret = true;
+ *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
+ } else {
+ /*
+ * Other modes are not allowed to succeed if they don't * match a rule,
+ * but there are rules. We've checked for no rule earlier in this
+ * function.
+ */
+ ret = false;
+ *allowed_privs = 0;
+ }
+
+ return ret;
+}
+
/*
* Public Interface
@@ -226,18 +255,19 @@ static int pmp_is_in_range(CPURISCVState *env, int pmp_index, target_ulong addr)
* Check if the address has required RWX privs to complete desired operation
*/
bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
- target_ulong size, pmp_priv_t privs, target_ulong mode)
+ target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
+ target_ulong mode)
{
int i = 0;
int ret = -1;
int pmp_size = 0;
target_ulong s = 0;
target_ulong e = 0;
- pmp_priv_t allowed_privs = 0;
/* Short cut if no rules */
if (0 == pmp_get_num_rules(env)) {
- return (env->priv == PRV_M) ? true : false;
+ return pmp_hart_has_privs_default(env, addr, size, privs,
+ allowed_privs, mode);
}
if (size == 0) {
@@ -277,37 +307,25 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
* check
*/
if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
- allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
+ *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
if ((mode != PRV_M) || pmp_is_locked(env, i)) {
- allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
+ *allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
}
- if ((privs & allowed_privs) == privs) {
- ret = 1;
- break;
- } else {
- ret = 0;
- break;
- }
+ ret = ((privs & *allowed_privs) == privs);
+ break;
}
}
/* No rule matched */
if (ret == -1) {
- if (mode == PRV_M) {
- ret = 1; /* Privileged spec v1.10 states if no PMP entry matches an
- * M-Mode access, the access succeeds */
- } else {
- ret = 0; /* Other modes are not allowed to succeed if they don't
- * match a rule, but there are rules. We've checked for
- * no rule earlier in this function. */
- }
+ return pmp_hart_has_privs_default(env, addr, size, privs,
+ allowed_privs, mode);
}
return ret == 1 ? true : false;
}
-
/*
* Handle a write to a pmpcfg CSP
*/
@@ -442,3 +460,23 @@ bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa,
return false;
}
+
+/*
+ * Convert PMP privilege to TLB page privilege.
+ */
+int pmp_priv_to_page_prot(pmp_priv_t pmp_priv)
+{
+ int prot = 0;
+
+ if (pmp_priv & PMP_READ) {
+ prot |= PAGE_READ;
+ }
+ if (pmp_priv & PMP_WRITE) {
+ prot |= PAGE_WRITE;
+ }
+ if (pmp_priv & PMP_EXEC) {
+ prot |= PAGE_EXEC;
+ }
+
+ return prot;
+}
diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
index c8d5ef4a69..b82a30f0d5 100644
--- a/target/riscv/pmp.h
+++ b/target/riscv/pmp.h
@@ -59,11 +59,13 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
target_ulong val);
target_ulong pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index);
bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
- target_ulong size, pmp_priv_t priv, target_ulong mode);
+ target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
+ target_ulong mode);
bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa,
target_ulong *tlb_size);
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);
+int pmp_priv_to_page_prot(pmp_priv_t pmp_priv);
#endif
--
2.30.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] target/riscv: add log of PMP permission checking
2021-02-21 14:01 [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits Jim Shu
2021-02-21 14:01 ` [PATCH 1/3] target/riscv: propagate PMP permission to TLB page Jim Shu
@ 2021-02-21 14:01 ` Jim Shu
2021-02-21 14:01 ` [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed Jim Shu
2021-03-17 17:47 ` [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits Alistair Francis
3 siblings, 0 replies; 8+ messages in thread
From: Jim Shu @ 2021-02-21 14:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: Alistair Francis, Palmer Dabbelt, Jim Shu, Sagar Karandikar,
Bastian Koppelmann
Like MMU translation, add qemu log of PMP permission checking for
debugging.
Signed-off-by: Jim Shu <cwshu@andestech.com>
---
target/riscv/cpu_helper.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f6ac63bf0e..c1ecb8a710 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -794,6 +794,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
if (ret == TRANSLATE_SUCCESS) {
ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
size, access_type, mode);
+
+ qemu_log_mask(CPU_LOG_MMU,
+ "%s PMP address=" TARGET_FMT_plx " ret %d prot"
+ " %d tlb_size " TARGET_FMT_lu "\n",
+ __func__, pa, ret, prot_pmp, tlb_size);
+
prot &= prot_pmp;
}
@@ -821,6 +827,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
if (ret == TRANSLATE_SUCCESS) {
ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
size, access_type, mode);
+
+ qemu_log_mask(CPU_LOG_MMU,
+ "%s PMP address=" TARGET_FMT_plx " ret %d prot"
+ " %d tlb_size " TARGET_FMT_lu "\n",
+ __func__, pa, ret, prot_pmp, tlb_size);
+
prot &= prot_pmp;
}
}
--
2.30.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed
2021-02-21 14:01 [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits Jim Shu
2021-02-21 14:01 ` [PATCH 1/3] target/riscv: propagate PMP permission to TLB page Jim Shu
2021-02-21 14:01 ` [PATCH 2/3] target/riscv: add log of PMP permission checking Jim Shu
@ 2021-02-21 14:01 ` Jim Shu
2021-03-16 20:02 ` Alistair Francis
2021-03-17 17:47 ` [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits Alistair Francis
3 siblings, 1 reply; 8+ messages in thread
From: Jim Shu @ 2021-02-21 14:01 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: Alistair Francis, Palmer Dabbelt, Jim Shu, Sagar Karandikar,
Bastian Koppelmann
If PMP permission of any address has been changed by updating PMP entry,
flush all TLB pages to prevent from getting old permission.
Signed-off-by: Jim Shu <cwshu@andestech.com>
---
target/riscv/pmp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index ebd874cde3..cff020122a 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -28,6 +28,7 @@
#include "qapi/error.h"
#include "cpu.h"
#include "trace.h"
+#include "exec/exec-all.h"
static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
uint8_t val);
@@ -347,6 +348,9 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
cfg_val = (val >> 8 * i) & 0xff;
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));
}
--
2.30.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] target/riscv: propagate PMP permission to TLB page
2021-02-21 14:01 ` [PATCH 1/3] target/riscv: propagate PMP permission to TLB page Jim Shu
@ 2021-03-16 20:01 ` Alistair Francis
0 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2021-03-16 20:01 UTC (permalink / raw)
To: Jim Shu
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt
On Sun, Feb 21, 2021 at 10:35 AM Jim Shu <cwshu@andestech.com> wrote:
>
> Currently, PMP permission checking of TLB page is bypassed if TLB hits
> Fix it by propagating PMP permission to TLB page permission.
>
> PMP permission checking also use MMU-style API to change TLB permission
> and size.
>
> Signed-off-by: Jim Shu <cwshu@andestech.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu_helper.c | 84 +++++++++++++++++++++++++++++----------
> target/riscv/pmp.c | 80 +++++++++++++++++++++++++++----------
> target/riscv/pmp.h | 4 +-
> 3 files changed, 125 insertions(+), 43 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 2f43939fb6..f6ac63bf0e 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -280,6 +280,49 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
> env->load_res = -1;
> }
>
> +/*
> + * get_physical_address_pmp - check PMP permission for this physical address
> + *
> + * Match the PMP region and check permission for this physical address and it's
> + * TLB page. Returns 0 if the permission checking was successful
> + *
> + * @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,
> + int size, MMUAccessType access_type,
> + int mode)
> +{
> + pmp_priv_t pmp_priv;
> + target_ulong tlb_size_pmp = 0;
> +
> + if (!riscv_feature(env, RISCV_FEATURE_PMP)) {
> + *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> + return TRANSLATE_SUCCESS;
> + }
> +
> + if (!pmp_hart_has_privs(env, addr, size, 1 << access_type, &pmp_priv,
> + mode)) {
> + *prot = 0;
> + return TRANSLATE_PMP_FAIL;
> + }
> +
> + *prot = pmp_priv_to_page_prot(pmp_priv);
> + if (tlb_size != NULL) {
> + if (pmp_is_range_in_tlb(env, addr & ~(*tlb_size - 1), &tlb_size_pmp)) {
> + *tlb_size = tlb_size_pmp;
> + }
> + }
> +
> + return TRANSLATE_SUCCESS;
> +}
> +
> /* get_physical_address - get the physical address for this virtual address
> *
> * Do a page table walk to obtain the physical address corresponding to a
> @@ -442,9 +485,11 @@ restart:
> pte_addr = base + idx * ptesize;
> }
>
> - if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> - !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
> - 1 << MMU_DATA_LOAD, PRV_S)) {
> + int pmp_prot;
> + int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, pte_addr,
> + sizeof(target_ulong),
> + MMU_DATA_LOAD, PRV_S);
> + if (pmp_ret != TRANSLATE_SUCCESS) {
> return TRANSLATE_PMP_FAIL;
> }
>
> @@ -682,13 +727,14 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> #ifndef CONFIG_USER_ONLY
> vaddr im_address;
> hwaddr pa = 0;
> - int prot, prot2;
> + int prot, prot2, prot_pmp;
> bool pmp_violation = false;
> bool first_stage_error = true;
> bool two_stage_lookup = false;
> int ret = TRANSLATE_FAIL;
> int mode = mmu_idx;
> - target_ulong tlb_size = 0;
> + /* default TLB page size */
> + target_ulong tlb_size = TARGET_PAGE_SIZE;
>
> env->guest_phys_fault_addr = 0;
>
> @@ -745,10 +791,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>
> prot &= prot2;
>
> - if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> - (ret == TRANSLATE_SUCCESS) &&
> - !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
> - ret = TRANSLATE_PMP_FAIL;
> + if (ret == TRANSLATE_SUCCESS) {
> + ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
> + size, access_type, mode);
> + prot &= prot_pmp;
> }
>
> if (ret != TRANSLATE_SUCCESS) {
> @@ -771,25 +817,21 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> "%s address=%" VADDR_PRIx " ret %d physical "
> TARGET_FMT_plx " prot %d\n",
> __func__, address, ret, pa, prot);
> - }
>
> - if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> - (ret == TRANSLATE_SUCCESS) &&
> - !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
> - ret = TRANSLATE_PMP_FAIL;
> + if (ret == TRANSLATE_SUCCESS) {
> + ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
> + size, access_type, mode);
> + prot &= prot_pmp;
> + }
> }
> +
> if (ret == TRANSLATE_PMP_FAIL) {
> pmp_violation = true;
> }
>
> if (ret == TRANSLATE_SUCCESS) {
> - if (pmp_is_range_in_tlb(env, pa & TARGET_PAGE_MASK, &tlb_size)) {
> - tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
> - prot, mmu_idx, tlb_size);
> - } else {
> - tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> - prot, mmu_idx, TARGET_PAGE_SIZE);
> - }
> + tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
> + prot, mmu_idx, tlb_size);
> return true;
> } else if (probe) {
> return false;
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 80d0334e1b..ebd874cde3 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -217,6 +217,35 @@ static int pmp_is_in_range(CPURISCVState *env, int pmp_index, target_ulong addr)
> return result;
> }
>
> +/*
> + * Check if the address has required RWX privs when no PMP entry is matched.
> + */
> +static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr,
> + target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
> + target_ulong mode)
> +{
> + bool ret;
> +
> + if ((!riscv_feature(env, RISCV_FEATURE_PMP)) || (mode == PRV_M)) {
> + /*
> + * Privileged spec v1.10 states if HW doesn't implement any PMP entry
> + * or no PMP entry matches an M-Mode access, the access succeeds.
> + */
> + ret = true;
> + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
> + } else {
> + /*
> + * Other modes are not allowed to succeed if they don't * match a rule,
> + * but there are rules. We've checked for no rule earlier in this
> + * function.
> + */
> + ret = false;
> + *allowed_privs = 0;
> + }
> +
> + return ret;
> +}
> +
>
> /*
> * Public Interface
> @@ -226,18 +255,19 @@ static int pmp_is_in_range(CPURISCVState *env, int pmp_index, target_ulong addr)
> * Check if the address has required RWX privs to complete desired operation
> */
> bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> - target_ulong size, pmp_priv_t privs, target_ulong mode)
> + target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
> + target_ulong mode)
> {
> int i = 0;
> int ret = -1;
> int pmp_size = 0;
> target_ulong s = 0;
> target_ulong e = 0;
> - pmp_priv_t allowed_privs = 0;
>
> /* Short cut if no rules */
> if (0 == pmp_get_num_rules(env)) {
> - return (env->priv == PRV_M) ? true : false;
> + return pmp_hart_has_privs_default(env, addr, size, privs,
> + allowed_privs, mode);
> }
>
> if (size == 0) {
> @@ -277,37 +307,25 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> * check
> */
> if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
> - allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
> + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
> if ((mode != PRV_M) || pmp_is_locked(env, i)) {
> - allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
> + *allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
> }
>
> - if ((privs & allowed_privs) == privs) {
> - ret = 1;
> - break;
> - } else {
> - ret = 0;
> - break;
> - }
> + ret = ((privs & *allowed_privs) == privs);
> + break;
> }
> }
>
> /* No rule matched */
> if (ret == -1) {
> - if (mode == PRV_M) {
> - ret = 1; /* Privileged spec v1.10 states if no PMP entry matches an
> - * M-Mode access, the access succeeds */
> - } else {
> - ret = 0; /* Other modes are not allowed to succeed if they don't
> - * match a rule, but there are rules. We've checked for
> - * no rule earlier in this function. */
> - }
> + return pmp_hart_has_privs_default(env, addr, size, privs,
> + allowed_privs, mode);
> }
>
> return ret == 1 ? true : false;
> }
>
> -
> /*
> * Handle a write to a pmpcfg CSP
> */
> @@ -442,3 +460,23 @@ bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa,
>
> return false;
> }
> +
> +/*
> + * Convert PMP privilege to TLB page privilege.
> + */
> +int pmp_priv_to_page_prot(pmp_priv_t pmp_priv)
> +{
> + int prot = 0;
> +
> + if (pmp_priv & PMP_READ) {
> + prot |= PAGE_READ;
> + }
> + if (pmp_priv & PMP_WRITE) {
> + prot |= PAGE_WRITE;
> + }
> + if (pmp_priv & PMP_EXEC) {
> + prot |= PAGE_EXEC;
> + }
> +
> + return prot;
> +}
> diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
> index c8d5ef4a69..b82a30f0d5 100644
> --- a/target/riscv/pmp.h
> +++ b/target/riscv/pmp.h
> @@ -59,11 +59,13 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
> target_ulong val);
> target_ulong pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index);
> bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> - target_ulong size, pmp_priv_t priv, target_ulong mode);
> + target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
> + target_ulong mode);
> bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa,
> target_ulong *tlb_size);
> 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);
> +int pmp_priv_to_page_prot(pmp_priv_t pmp_priv);
>
> #endif
> --
> 2.30.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed
2021-02-21 14:01 ` [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed Jim Shu
@ 2021-03-16 20:02 ` Alistair Francis
0 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2021-03-16 20:02 UTC (permalink / raw)
To: Jim Shu
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt
On Sun, Feb 21, 2021 at 10:31 AM Jim Shu <cwshu@andestech.com> wrote:
>
> If PMP permission of any address has been changed by updating PMP entry,
> flush all TLB pages to prevent from getting old permission.
>
> Signed-off-by: Jim Shu <cwshu@andestech.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/pmp.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index ebd874cde3..cff020122a 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -28,6 +28,7 @@
> #include "qapi/error.h"
> #include "cpu.h"
> #include "trace.h"
> +#include "exec/exec-all.h"
>
> static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> uint8_t val);
> @@ -347,6 +348,9 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
> cfg_val = (val >> 8 * i) & 0xff;
> 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));
> }
>
>
> --
> 2.30.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits
2021-02-21 14:01 [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits Jim Shu
` (2 preceding siblings ...)
2021-02-21 14:01 ` [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed Jim Shu
@ 2021-03-17 17:47 ` Alistair Francis
3 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2021-03-17 17:47 UTC (permalink / raw)
To: Jim Shu
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
listair.Francis, qemu-devel@nongnu.org Developers,
Palmer Dabbelt
On Sun, Feb 21, 2021 at 10:33 AM Jim Shu <cwshu@andestech.com> wrote:
>
> Sorry for sending this patch set again.
> The cover letter of my previous mail doesn't add cc list.
> ---
>
> Current implementation of PMP permission checking only has effect when
> softmmu's TLB miss. PMP checking is bypassed when TLB hits because TLB page
> permission isn't affected by PMP permission.
>
> To fix this issue, this patch set addes the feature to propagate PMP
> permission to the TLB page and flush TLB pages if PMP permission has
> been changed.
>
> The patch set is tested on Zephyr RTOS userspace testsuite on QEMU riscv32
> virt machine.
>
> Jim Shu (3):
> target/riscv: propagate PMP permission to TLB page
> target/riscv: add log of PMP permission checking
> target/riscv: flush TLB pages if PMP permission has been changed
Thanks!
Applied to riscv-to-apply.next
Alistair
>
> target/riscv/cpu_helper.c | 96 ++++++++++++++++++++++++++++++---------
> target/riscv/pmp.c | 84 +++++++++++++++++++++++++---------
> target/riscv/pmp.h | 4 +-
> 3 files changed, 141 insertions(+), 43 deletions(-)
>
> --
> 2.30.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] target/riscv: propagate PMP permission to TLB page
[not found] <1613914370-17285-1-git-send-email-cwshu@andestech.com>
@ 2021-02-21 13:32 ` Jim Shu
0 siblings, 0 replies; 8+ messages in thread
From: Jim Shu @ 2021-02-21 13:32 UTC (permalink / raw)
Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
open list:All patches CC here, Alistair Francis, Palmer Dabbelt,
Jim Shu
Currently, PMP permission checking of TLB page is bypassed if TLB hits
Fix it by propagating PMP permission to TLB page permission.
PMP permission checking also use MMU-style API to change TLB permission
and size.
Signed-off-by: Jim Shu <cwshu@andestech.com>
---
target/riscv/cpu_helper.c | 84 +++++++++++++++++++++++++++++----------
target/riscv/pmp.c | 80 +++++++++++++++++++++++++++----------
target/riscv/pmp.h | 4 +-
3 files changed, 125 insertions(+), 43 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 2f43939fb6..f6ac63bf0e 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -280,6 +280,49 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
env->load_res = -1;
}
+/*
+ * get_physical_address_pmp - check PMP permission for this physical address
+ *
+ * Match the PMP region and check permission for this physical address and it's
+ * TLB page. Returns 0 if the permission checking was successful
+ *
+ * @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,
+ int size, MMUAccessType access_type,
+ int mode)
+{
+ pmp_priv_t pmp_priv;
+ target_ulong tlb_size_pmp = 0;
+
+ if (!riscv_feature(env, RISCV_FEATURE_PMP)) {
+ *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+ return TRANSLATE_SUCCESS;
+ }
+
+ if (!pmp_hart_has_privs(env, addr, size, 1 << access_type, &pmp_priv,
+ mode)) {
+ *prot = 0;
+ return TRANSLATE_PMP_FAIL;
+ }
+
+ *prot = pmp_priv_to_page_prot(pmp_priv);
+ if (tlb_size != NULL) {
+ if (pmp_is_range_in_tlb(env, addr & ~(*tlb_size - 1), &tlb_size_pmp)) {
+ *tlb_size = tlb_size_pmp;
+ }
+ }
+
+ return TRANSLATE_SUCCESS;
+}
+
/* get_physical_address - get the physical address for this virtual address
*
* Do a page table walk to obtain the physical address corresponding to a
@@ -442,9 +485,11 @@ restart:
pte_addr = base + idx * ptesize;
}
- if (riscv_feature(env, RISCV_FEATURE_PMP) &&
- !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
- 1 << MMU_DATA_LOAD, PRV_S)) {
+ int pmp_prot;
+ int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, pte_addr,
+ sizeof(target_ulong),
+ MMU_DATA_LOAD, PRV_S);
+ if (pmp_ret != TRANSLATE_SUCCESS) {
return TRANSLATE_PMP_FAIL;
}
@@ -682,13 +727,14 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
#ifndef CONFIG_USER_ONLY
vaddr im_address;
hwaddr pa = 0;
- int prot, prot2;
+ int prot, prot2, prot_pmp;
bool pmp_violation = false;
bool first_stage_error = true;
bool two_stage_lookup = false;
int ret = TRANSLATE_FAIL;
int mode = mmu_idx;
- target_ulong tlb_size = 0;
+ /* default TLB page size */
+ target_ulong tlb_size = TARGET_PAGE_SIZE;
env->guest_phys_fault_addr = 0;
@@ -745,10 +791,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
prot &= prot2;
- if (riscv_feature(env, RISCV_FEATURE_PMP) &&
- (ret == TRANSLATE_SUCCESS) &&
- !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
- ret = TRANSLATE_PMP_FAIL;
+ if (ret == TRANSLATE_SUCCESS) {
+ ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
+ size, access_type, mode);
+ prot &= prot_pmp;
}
if (ret != TRANSLATE_SUCCESS) {
@@ -771,25 +817,21 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
"%s address=%" VADDR_PRIx " ret %d physical "
TARGET_FMT_plx " prot %d\n",
__func__, address, ret, pa, prot);
- }
- if (riscv_feature(env, RISCV_FEATURE_PMP) &&
- (ret == TRANSLATE_SUCCESS) &&
- !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
- ret = TRANSLATE_PMP_FAIL;
+ if (ret == TRANSLATE_SUCCESS) {
+ ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
+ size, access_type, mode);
+ prot &= prot_pmp;
+ }
}
+
if (ret == TRANSLATE_PMP_FAIL) {
pmp_violation = true;
}
if (ret == TRANSLATE_SUCCESS) {
- if (pmp_is_range_in_tlb(env, pa & TARGET_PAGE_MASK, &tlb_size)) {
- tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
- prot, mmu_idx, tlb_size);
- } else {
- tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
- prot, mmu_idx, TARGET_PAGE_SIZE);
- }
+ tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
+ prot, mmu_idx, tlb_size);
return true;
} else if (probe) {
return false;
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 80d0334e1b..ebd874cde3 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -217,6 +217,35 @@ static int pmp_is_in_range(CPURISCVState *env, int pmp_index, target_ulong addr)
return result;
}
+/*
+ * Check if the address has required RWX privs when no PMP entry is matched.
+ */
+static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr,
+ target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
+ target_ulong mode)
+{
+ bool ret;
+
+ if ((!riscv_feature(env, RISCV_FEATURE_PMP)) || (mode == PRV_M)) {
+ /*
+ * Privileged spec v1.10 states if HW doesn't implement any PMP entry
+ * or no PMP entry matches an M-Mode access, the access succeeds.
+ */
+ ret = true;
+ *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
+ } else {
+ /*
+ * Other modes are not allowed to succeed if they don't * match a rule,
+ * but there are rules. We've checked for no rule earlier in this
+ * function.
+ */
+ ret = false;
+ *allowed_privs = 0;
+ }
+
+ return ret;
+}
+
/*
* Public Interface
@@ -226,18 +255,19 @@ static int pmp_is_in_range(CPURISCVState *env, int pmp_index, target_ulong addr)
* Check if the address has required RWX privs to complete desired operation
*/
bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
- target_ulong size, pmp_priv_t privs, target_ulong mode)
+ target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
+ target_ulong mode)
{
int i = 0;
int ret = -1;
int pmp_size = 0;
target_ulong s = 0;
target_ulong e = 0;
- pmp_priv_t allowed_privs = 0;
/* Short cut if no rules */
if (0 == pmp_get_num_rules(env)) {
- return (env->priv == PRV_M) ? true : false;
+ return pmp_hart_has_privs_default(env, addr, size, privs,
+ allowed_privs, mode);
}
if (size == 0) {
@@ -277,37 +307,25 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
* check
*/
if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
- allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
+ *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
if ((mode != PRV_M) || pmp_is_locked(env, i)) {
- allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
+ *allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
}
- if ((privs & allowed_privs) == privs) {
- ret = 1;
- break;
- } else {
- ret = 0;
- break;
- }
+ ret = ((privs & *allowed_privs) == privs);
+ break;
}
}
/* No rule matched */
if (ret == -1) {
- if (mode == PRV_M) {
- ret = 1; /* Privileged spec v1.10 states if no PMP entry matches an
- * M-Mode access, the access succeeds */
- } else {
- ret = 0; /* Other modes are not allowed to succeed if they don't
- * match a rule, but there are rules. We've checked for
- * no rule earlier in this function. */
- }
+ return pmp_hart_has_privs_default(env, addr, size, privs,
+ allowed_privs, mode);
}
return ret == 1 ? true : false;
}
-
/*
* Handle a write to a pmpcfg CSP
*/
@@ -442,3 +460,23 @@ bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa,
return false;
}
+
+/*
+ * Convert PMP privilege to TLB page privilege.
+ */
+int pmp_priv_to_page_prot(pmp_priv_t pmp_priv)
+{
+ int prot = 0;
+
+ if (pmp_priv & PMP_READ) {
+ prot |= PAGE_READ;
+ }
+ if (pmp_priv & PMP_WRITE) {
+ prot |= PAGE_WRITE;
+ }
+ if (pmp_priv & PMP_EXEC) {
+ prot |= PAGE_EXEC;
+ }
+
+ return prot;
+}
diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h
index c8d5ef4a69..b82a30f0d5 100644
--- a/target/riscv/pmp.h
+++ b/target/riscv/pmp.h
@@ -59,11 +59,13 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
target_ulong val);
target_ulong pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index);
bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
- target_ulong size, pmp_priv_t priv, target_ulong mode);
+ target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs,
+ target_ulong mode);
bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa,
target_ulong *tlb_size);
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);
+int pmp_priv_to_page_prot(pmp_priv_t pmp_priv);
#endif
--
2.30.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-17 17:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-21 14:01 [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits Jim Shu
2021-02-21 14:01 ` [PATCH 1/3] target/riscv: propagate PMP permission to TLB page Jim Shu
2021-03-16 20:01 ` Alistair Francis
2021-02-21 14:01 ` [PATCH 2/3] target/riscv: add log of PMP permission checking Jim Shu
2021-02-21 14:01 ` [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed Jim Shu
2021-03-16 20:02 ` Alistair Francis
2021-03-17 17:47 ` [PATCH 0/3] target/riscv: fix PMP permission checking when softmmu's TLB hits Alistair Francis
[not found] <1613914370-17285-1-git-send-email-cwshu@andestech.com>
2021-02-21 13:32 ` [PATCH 1/3] target/riscv: propagate PMP permission to TLB page Jim Shu
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).