Re: [PATCH v3 6/7] target/riscv: Make the short cut really work in pmp

qemu-devel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 6/7] target/riscv: Make the short cut really work in pmp_h

From:	Weiwei Li
Subject:	Re: [PATCH v3 6/7] target/riscv: Make the short cut really work in pmp_hart_has_privs
Date:	Thu, 20 Apr 2023 21:53:52 +0800
User-agent:	Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0


On 2023/4/20 21:33, LIU Zhiwei wrote:

On 2023/4/19 11:27, Weiwei Li wrote:
We needn't check the PMP entries if there is no PMP rules.
This commit doesn't give much information. If you are fixing a bug,you should point it out why the original implementation is wrong.


This patch is not to fix bug , but to improve the original implementation.

I'll add more description in the commit message.


Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
  target/riscv/pmp.c | 251 ++++++++++++++++++++++-----------------------

Have we changed all the logic of this function? It's really a lot ofcode change. I am not sure if there is a git option to reduce the codemove in the patch.


Zhiwei

  1 file changed, 123 insertions(+), 128 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 7feaddd7eb..755ed2b963 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c

@@ -314,149 +314,144 @@ int pmp_hart_has_privs(CPURISCVState *env,target_ulong addr,

      target_ulong e = 0;
        /* Short cut if no rules */
-    if (0 == pmp_get_num_rules(env)) {
-        if (pmp_hart_has_privs_default(env, addr, size, privs,
-                                       allowed_privs, mode)) {
-            ret = MAX_RISCV_PMPS;
-        }
-    }
-

In original code, short cut if no rules didn't really work. all thefollowing check also should be done when pmp_get_num_rules() == 0, andthis is unneccessary.

So I move the following check into the condition pmp_get_num_rules(env)!= 0, and reuse the pmp_hart_has_privs_default() check at the end ofthis function for pmp_get_num_rules() == 0.


Regards,

Weiwei Li

-    if (size == 0) {
-        if (riscv_cpu_cfg(env)->mmu) {
-            /*
-             * If size is unknown (0), assume that all bytes
-             * from addr to the end of the page will be accessed.
-             */
-            pmp_size = -(addr | TARGET_PAGE_MASK);
+    if (pmp_get_num_rules(env) != 0) {
+        if (size == 0) {
+            if (riscv_cpu_cfg(env)->mmu) {
+                /*
+                 * If size is unknown (0), assume that all bytes
+                 * from addr to the end of the page will be accessed.
+                 */
+                pmp_size = -(addr | TARGET_PAGE_MASK);
+            } else {
+                pmp_size = sizeof(target_ulong);
+            }
          } else {
-            pmp_size = sizeof(target_ulong);
-        }
-    } else {
-        pmp_size = size;
-    }
-
-    /*
-     * 1.10 draft priv spec states there is an implicit order
-     * from low to high
-     */
-    for (i = 0; i < MAX_RISCV_PMPS; i++) {
-        s = pmp_is_in_range(env, i, addr);
-        e = pmp_is_in_range(env, i, addr + pmp_size - 1);
-
-        /* partially inside */
-        if ((s + e) == 1) {
-            qemu_log_mask(LOG_GUEST_ERROR,

- "pmp violation - access is partiallyinside\n");

-            ret = -1;
-            break;
+            pmp_size = size;
          }
  -        /* fully inside */
-        const uint8_t a_field =
-            pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
-
          /*
-         * Convert the PMP permissions to match the truth table in the
-         * ePMP spec.
+         * 1.10 draft priv spec states there is an implicit order
+         * from low to high
           */
-        const uint8_t epmp_operation =
-            ((env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) >> 4) |
-            ((env->pmp_state.pmp[i].cfg_reg & PMP_READ) << 2) |
-            (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) |
-            ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2);
+        for (i = 0; i < MAX_RISCV_PMPS; i++) {
+            s = pmp_is_in_range(env, i, addr);
+            e = pmp_is_in_range(env, i, addr + pmp_size - 1);
+
+            /* partially inside */
+            if ((s + e) == 1) {
+                qemu_log_mask(LOG_GUEST_ERROR,

+ "pmp violation - access is partiallyinside\n");

+                ret = -1;
+                break;
+            }
+
+            /* fully inside */
+            const uint8_t a_field =
+ pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
  -        if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
              /*
-             * If the PMP entry is not off and the address is in range,
-             * do the priv check

+ * Convert the PMP permissions to match the truth tablein the

+             * ePMP spec.
               */
-            if (!MSECCFG_MML_ISSET(env)) {
-                /*
-                 * If mseccfg.MML Bit is not set, do pmp priv check
-                 * This will always apply to regular PMP.
-                 */
-                *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;
-                }
-            } else {
+            const uint8_t epmp_operation =
+                ((env->pmp_state.pmp[i].cfg_reg & PMP_LOCK) >> 4) |
+                ((env->pmp_state.pmp[i].cfg_reg & PMP_READ) << 2) |
+                (env->pmp_state.pmp[i].cfg_reg & PMP_WRITE) |
+                ((env->pmp_state.pmp[i].cfg_reg & PMP_EXEC) >> 2);
+
+            if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
                  /*

- * If mseccfg.MML Bit set, do the enhanced pmp privcheck+ * If the PMP entry is not off and the address is inrange,

+                 * do the priv check
                   */
-                if (mode == PRV_M) {
-                    switch (epmp_operation) {
-                    case 0:
-                    case 1:
-                    case 4:
-                    case 5:
-                    case 6:
-                    case 7:
-                    case 8:
-                        *allowed_privs = 0;
-                        break;
-                    case 2:
-                    case 3:
-                    case 14:
-                        *allowed_privs = PMP_READ | PMP_WRITE;
-                        break;
-                    case 9:
-                    case 10:
-                        *allowed_privs = PMP_EXEC;
-                        break;
-                    case 11:
-                    case 13:
-                        *allowed_privs = PMP_READ | PMP_EXEC;
-                        break;
-                    case 12:
-                    case 15:
-                        *allowed_privs = PMP_READ;
-                        break;
-                    default:
-                        g_assert_not_reached();
+                if (!MSECCFG_MML_ISSET(env)) {
+                    /*
+                     * If mseccfg.MML Bit is not set, do pmp priv check
+                     * This will always apply to regular PMP.
+                     */
+                    *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;

                      }
                  } else {
-                    switch (epmp_operation) {
-                    case 0:
-                    case 8:
-                    case 9:
-                    case 12:
-                    case 13:
-                    case 14:
-                        *allowed_privs = 0;
-                        break;
-                    case 1:
-                    case 10:
-                    case 11:
-                        *allowed_privs = PMP_EXEC;
-                        break;
-                    case 2:
-                    case 4:
-                    case 15:
-                        *allowed_privs = PMP_READ;
-                        break;
-                    case 3:
-                    case 6:
-                        *allowed_privs = PMP_READ | PMP_WRITE;
-                        break;
-                    case 5:
-                        *allowed_privs = PMP_READ | PMP_EXEC;
-                        break;
-                    case 7:

- *allowed_privs = PMP_READ | PMP_WRITE |PMP_EXEC;

-                        break;
-                    default:
-                        g_assert_not_reached();
+                    /*

+ * If mseccfg.MML Bit set, do the enhanced pmppriv check

+                     */
+                    if (mode == PRV_M) {
+                        switch (epmp_operation) {
+                        case 0:
+                        case 1:
+                        case 4:
+                        case 5:
+                        case 6:
+                        case 7:
+                        case 8:
+                            *allowed_privs = 0;
+                            break;
+                        case 2:
+                        case 3:
+                        case 14:
+                            *allowed_privs = PMP_READ | PMP_WRITE;
+                            break;
+                        case 9:
+                        case 10:
+                            *allowed_privs = PMP_EXEC;
+                            break;
+                        case 11:
+                        case 13:
+                            *allowed_privs = PMP_READ | PMP_EXEC;
+                            break;
+                        case 12:
+                        case 15:
+                            *allowed_privs = PMP_READ;
+                            break;
+                        default:
+                            g_assert_not_reached();
+                        }
+                    } else {
+                        switch (epmp_operation) {
+                        case 0:
+                        case 8:
+                        case 9:
+                        case 12:
+                        case 13:
+                        case 14:
+                            *allowed_privs = 0;
+                            break;
+                        case 1:
+                        case 10:
+                        case 11:
+                            *allowed_privs = PMP_EXEC;
+                            break;
+                        case 2:
+                        case 4:
+                        case 15:
+                            *allowed_privs = PMP_READ;
+                            break;
+                        case 3:
+                        case 6:
+                            *allowed_privs = PMP_READ | PMP_WRITE;
+                            break;
+                        case 5:
+                            *allowed_privs = PMP_READ | PMP_EXEC;
+                            break;
+                        case 7:

+ *allowed_privs = PMP_READ | PMP_WRITE |PMP_EXEC;

+                            break;
+                        default:
+                            g_assert_not_reached();
+                        }
                      }
                  }
-            }
  -            /*
-             * If matching address range was found, the protection bits
-             * defined with PMP must be used. We shouldn't fallback on
-             * finding default privileges.
-             */
-            ret = i;
-            break;
+                /*

+ * If matching address range was found, theprotection bits+ * defined with PMP must be used. We shouldn'tfallback on

+                 * finding default privileges.
+                 */
+                ret = i;
+                break;
+            }
          }
      }

[Prev in Thread]

Current Thread

[Next in Thread]

Re: [PATCH v3 1/7] target/riscv: Update pmp_get_tlb_size(), (continued)
- [PATCH v3 3/7] target/riscv: Flush TLB when pmpaddr is updated, Weiwei Li, 2023/04/18
  - Re: [PATCH v3 3/7] target/riscv: Flush TLB when pmpaddr is updated, LIU Zhiwei, 2023/04/20
- [PATCH v3 2/7] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp, Weiwei Li, 2023/04/18
  - Re: [PATCH v3 2/7] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp, LIU Zhiwei, 2023/04/20
    - Re: [PATCH v3 2/7] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp, Weiwei Li, 2023/04/20
- [PATCH v3 4/7] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes, Weiwei Li, 2023/04/18
  - Re: [PATCH v3 4/7] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes, LIU Zhiwei, 2023/04/20
- [PATCH v3 6/7] target/riscv: Make the short cut really work in pmp_hart_has_privs, Weiwei Li, 2023/04/18
  - Re: [PATCH v3 6/7] target/riscv: Make the short cut really work in pmp_hart_has_privs, LIU Zhiwei, 2023/04/20
    - Re: [PATCH v3 6/7] target/riscv: Make the short cut really work in pmp_hart_has_privs, Weiwei Li <=
- [PATCH v3 5/7] accel/tcg: Uncache the host address for instruction fetch when tlb size < 1, Weiwei Li, 2023/04/18
  - Re: [PATCH v3 5/7] accel/tcg: Uncache the host address for instruction fetch when tlb size < 1, Richard Henderson, 2023/04/19

Prev by Date: Re: [PATCH] docs/about/deprecated.rst: Add "since 7.1" tag to dtb-kaslr-seed deprecation
Next by Date: Re: [PATCH] hw/rdma: Compile target-independent parts of the rdma code only once
Previous by thread: Re: [PATCH v3 6/7] target/riscv: Make the short cut really work in pmp_hart_has_privs
Next by thread: [PATCH v3 5/7] accel/tcg: Uncache the host address for instruction fetch when tlb size < 1
Index(es):
- Date
- Thread