All of lore.kernel.org
 help / color / mirror / Atom feed
* [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
  2021-02-21 13:32   ` Jim Shu
  2021-02-21 13:32   ` Jim Shu
  2 siblings, 0 replies; 11+ 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] 11+ messages in thread
* [PATCH 1/3] target/riscv: propagate PMP permission to TLB page
@ 2021-02-21 13:32   ` Jim Shu
  0 siblings, 0 replies; 11+ messages in thread
From: Jim Shu @ 2021-02-21 13:32 UTC (permalink / raw)
  Cc: Jim Shu, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
	Bastian Koppelmann, open list:RISC-V TCG CPUs,
	open list:All patches CC here

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] 11+ messages in thread
* [PATCH 2/3] target/riscv: add log of PMP permission checking
       [not found] <1613914370-17285-1-git-send-email-cwshu@andestech.com>
@ 2021-02-21 13:32   ` Jim Shu
  2021-02-21 13:32   ` Jim Shu
  2021-02-21 13:32   ` Jim Shu
  2 siblings, 0 replies; 11+ 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

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] 11+ messages in thread
* [PATCH 2/3] target/riscv: add log of PMP permission checking
@ 2021-02-21 13:32   ` Jim Shu
  0 siblings, 0 replies; 11+ messages in thread
From: Jim Shu @ 2021-02-21 13:32 UTC (permalink / raw)
  Cc: Jim Shu, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
	Bastian Koppelmann, open list:RISC-V TCG CPUs,
	open list:All patches CC here

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] 11+ messages in thread
* [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed
       [not found] <1613914370-17285-1-git-send-email-cwshu@andestech.com>
@ 2021-02-21 13:32   ` Jim Shu
  2021-02-21 13:32   ` Jim Shu
  2021-02-21 13:32   ` Jim Shu
  2 siblings, 0 replies; 11+ 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

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] 11+ messages in thread
* [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed
@ 2021-02-21 13:32   ` Jim Shu
  0 siblings, 0 replies; 11+ messages in thread
From: Jim Shu @ 2021-02-21 13:32 UTC (permalink / raw)
  Cc: Jim Shu, Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
	Bastian Koppelmann, open list:RISC-V TCG CPUs,
	open list:All patches CC here

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] 11+ messages in thread
* Re: [PATCH 2/3] target/riscv: add log of PMP permission checking
  2021-02-21 13:32   ` Jim Shu
  (?)
@ 2021-03-16 20:02   ` Alistair Francis
  -1 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2021-03-16 20:02 UTC (permalink / raw)
  To: Jim Shu
  Cc: open list:RISC-V TCG CPUs, Sagar Karandikar, Bastian Koppelmann,
	open list:All patches CC here, Alistair Francis, Palmer Dabbelt

On Sun, Feb 21, 2021 at 10:31 AM Jim Shu <cwshu@andestech.com> wrote:
>
> Like MMU translation, add qemu log of PMP permission checking for
> debugging.
>
> Signed-off-by: Jim Shu <cwshu@andestech.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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	[flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] target/riscv: propagate PMP permission to TLB page
  2021-02-21 14:01   ` Jim Shu
@ 2021-03-16 20:01     ` Alistair Francis
  -1 siblings, 0 replies; 11+ 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] 11+ messages in thread
* Re: [PATCH 1/3] target/riscv: propagate PMP permission to TLB page
@ 2021-03-16 20:01     ` Alistair Francis
  0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2021-03-16 20:01 UTC (permalink / raw)
  To: Jim Shu
  Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
	Alistair Francis, Palmer Dabbelt, Sagar Karandikar,
	Bastian Koppelmann

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] 11+ 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
  0 siblings, 0 replies; 11+ 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] 11+ messages in thread
* [PATCH 1/3] target/riscv: propagate PMP permission to TLB page
@ 2021-02-21 14:01   ` Jim Shu
  0 siblings, 0 replies; 11+ messages in thread
From: Jim Shu @ 2021-02-21 14:01 UTC (permalink / raw)
  To: qemu-riscv, qemu-devel
  Cc: Jim Shu, Palmer Dabbelt, Alistair Francis, 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] 11+ messages in thread
end of thread, other threads:[~2021-03-16 20:23 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [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
2021-02-21 13:32   ` Jim Shu
2021-02-21 13:32 ` [PATCH 2/3] target/riscv: add log of PMP permission checking Jim Shu
2021-02-21 13:32   ` Jim Shu
2021-03-16 20:02   ` Alistair Francis
2021-02-21 13:32 ` [PATCH 3/3] target/riscv: flush TLB pages if PMP permission has been changed Jim Shu
2021-02-21 13:32   ` Jim Shu
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-03-16 20:01   ` Alistair Francis
2021-03-16 20:01     ` Alistair Francis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.