All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Fix some PMP implementation
@ 2020-07-21  6:03 Zong Li
  2020-07-21  6:03   ` Zong Li
  2020-07-21  6:03 ` [PATCH v2 2/2] target/riscv/pmp.c: Fix the index offset on RV64 Zong Li
  0 siblings, 2 replies; 6+ messages in thread
From: Zong Li @ 2020-07-21  6:03 UTC (permalink / raw)
  To: palmer, Alistair.Francis, bmeng.cn, sagark, kbastian, qemu-riscv,
	qemu-devel
  Cc: Zong Li

This patch set contains the fixes for wrong index of pmpcfg CSR on rv64,
and the pmp range in CSR function table.

Changed in v2:
 - Move out the shifting operation from loop. Suggested by Bin Meng.

Zong Li (2):
  target/riscv: Fix the range of pmpcfg of CSR funcion table
  target/riscv/pmp.c: Fix the index offset on RV64

 target/riscv/csr.c |  2 +-
 target/riscv/pmp.c | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.27.0



^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2 1/2] target/riscv: Fix the range of pmpcfg of CSR funcion table
  2020-07-21  6:03 [PATCH v2 0/2] Fix some PMP implementation Zong Li
@ 2020-07-21  6:03   ` Zong Li
  2020-07-21  6:03 ` [PATCH v2 2/2] target/riscv/pmp.c: Fix the index offset on RV64 Zong Li
  1 sibling, 0 replies; 6+ messages in thread
From: Zong Li @ 2020-07-21  6:03 UTC (permalink / raw)
  To: palmer, Alistair.Francis, bmeng.cn, sagark, kbastian, qemu-riscv,
	qemu-devel
  Cc: Bin Meng, Alistair Francis, Zong Li

The range of Physical Memory Protection should be from CSR_PMPCFG0
to CSR_PMPCFG3, not to CSR_PMPADDR9.

Signed-off-by: Zong Li <zong.li@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
---
 target/riscv/csr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ac01c835e1..6a96a01b1c 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1353,7 +1353,7 @@ static riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_MTINST] =              { hmode,   read_mtinst,      write_mtinst     },
 
     /* Physical Memory Protection */
-    [CSR_PMPCFG0  ... CSR_PMPADDR9] =  { pmp,   read_pmpcfg,  write_pmpcfg   },
+    [CSR_PMPCFG0  ... CSR_PMPCFG3]   = { pmp,   read_pmpcfg,  write_pmpcfg   },
     [CSR_PMPADDR0 ... CSR_PMPADDR15] = { pmp,   read_pmpaddr, write_pmpaddr  },
 
     /* Performance Counters */
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2 1/2] target/riscv: Fix the range of pmpcfg of CSR funcion table
@ 2020-07-21  6:03   ` Zong Li
  0 siblings, 0 replies; 6+ messages in thread
From: Zong Li @ 2020-07-21  6:03 UTC (permalink / raw)
  To: palmer, Alistair.Francis, bmeng.cn, sagark, kbastian, qemu-riscv,
	qemu-devel
  Cc: Zong Li, Alistair Francis, Bin Meng

The range of Physical Memory Protection should be from CSR_PMPCFG0
to CSR_PMPCFG3, not to CSR_PMPADDR9.

Signed-off-by: Zong Li <zong.li@sifive.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
---
 target/riscv/csr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ac01c835e1..6a96a01b1c 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1353,7 +1353,7 @@ static riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_MTINST] =              { hmode,   read_mtinst,      write_mtinst     },
 
     /* Physical Memory Protection */
-    [CSR_PMPCFG0  ... CSR_PMPADDR9] =  { pmp,   read_pmpcfg,  write_pmpcfg   },
+    [CSR_PMPCFG0  ... CSR_PMPCFG3]   = { pmp,   read_pmpcfg,  write_pmpcfg   },
     [CSR_PMPADDR0 ... CSR_PMPADDR15] = { pmp,   read_pmpaddr, write_pmpaddr  },
 
     /* Performance Counters */
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2 2/2] target/riscv/pmp.c: Fix the index offset on RV64
  2020-07-21  6:03 [PATCH v2 0/2] Fix some PMP implementation Zong Li
  2020-07-21  6:03   ` Zong Li
@ 2020-07-21  6:03 ` Zong Li
  2020-07-21  6:11     ` Bin Meng
  1 sibling, 1 reply; 6+ messages in thread
From: Zong Li @ 2020-07-21  6:03 UTC (permalink / raw)
  To: palmer, Alistair.Francis, bmeng.cn, sagark, kbastian, qemu-riscv,
	qemu-devel
  Cc: Zong Li

On RV64, the reg_index is 2 (pmpcfg2 CSR) after the seventh pmp
entry, it is not 1 (pmpcfg1 CSR) like RV32. In the original
implementation, the second parameter of pmp_write_cfg is
"reg_index * sizeof(target_ulong)", and we get the the result
which is started from 16 if reg_index is 2, but we expect that
it should be started from 8. Separate the implementation for
RV32 and RV64 respectively.

Changed in v2:
 - Move out the shifting operation from loop. Suggested by Bin Meng

Signed-off-by: Zong Li <zong.li@sifive.com>
---
 target/riscv/pmp.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 2a2b9f5363..3de6535fbd 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -309,6 +309,7 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
 {
     int i;
     uint8_t cfg_val;
+    uint32_t pmp_entry_base;
 
     trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
 
@@ -318,10 +319,15 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
         return;
     }
 
+#if defined(TARGET_RISCV32)
+    pmp_entry_base = (reg_index * sizeof(target_ulong));
+#elif defined(TARGET_RISCV64)
+    pmp_entry_base = (reg_index >> 1) * sizeof(target_ulong);
+#endif
+
     for (i = 0; i < sizeof(target_ulong); i++) {
         cfg_val = (val >> 8 * i)  & 0xff;
-        pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i,
-            cfg_val);
+        pmp_write_cfg(env, pmp_entry_base + i, cfg_val);
     }
 }
 
@@ -332,11 +338,18 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
 target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
 {
     int i;
+    uint32_t pmp_entry_base;
     target_ulong cfg_val = 0;
     target_ulong val = 0;
 
+#if defined(TARGET_RISCV32)
+    pmp_entry_base = (reg_index * sizeof(target_ulong));
+#elif defined(TARGET_RISCV64)
+    pmp_entry_base = (reg_index >> 1) * sizeof(target_ulong);
+#endif
+
     for (i = 0; i < sizeof(target_ulong); i++) {
-        val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
+        val = pmp_read_cfg(env, pmp_entry_base + i);
         cfg_val |= (val << (i * 8));
     }
     trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] target/riscv/pmp.c: Fix the index offset on RV64
  2020-07-21  6:03 ` [PATCH v2 2/2] target/riscv/pmp.c: Fix the index offset on RV64 Zong Li
@ 2020-07-21  6:11     ` Bin Meng
  0 siblings, 0 replies; 6+ messages in thread
From: Bin Meng @ 2020-07-21  6:11 UTC (permalink / raw)
  To: Zong Li
  Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
	qemu-devel@nongnu.org Developers, Alistair Francis,
	Palmer Dabbelt

Hi Zong,

On Tue, Jul 21, 2020 at 2:03 PM Zong Li <zong.li@sifive.com> wrote:
>
> On RV64, the reg_index is 2 (pmpcfg2 CSR) after the seventh pmp
> entry, it is not 1 (pmpcfg1 CSR) like RV32. In the original
> implementation, the second parameter of pmp_write_cfg is
> "reg_index * sizeof(target_ulong)", and we get the the result
> which is started from 16 if reg_index is 2, but we expect that
> it should be started from 8. Separate the implementation for
> RV32 and RV64 respectively.
>
> Changed in v2:
>  - Move out the shifting operation from loop. Suggested by Bin Meng

The changelog should go after --- below

>
> Signed-off-by: Zong Li <zong.li@sifive.com>
> ---
>  target/riscv/pmp.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 2a2b9f5363..3de6535fbd 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -309,6 +309,7 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>  {
>      int i;
>      uint8_t cfg_val;
> +    uint32_t pmp_entry_base;
>
>      trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
>
> @@ -318,10 +319,15 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>          return;
>      }
>
> +#if defined(TARGET_RISCV32)
> +    pmp_entry_base = (reg_index * sizeof(target_ulong));
> +#elif defined(TARGET_RISCV64)
> +    pmp_entry_base = (reg_index >> 1) * sizeof(target_ulong);
> +#endif

This is not necessary. You can simply do:

#if defined(TARGET_RISCV64)
    reg_index >>= 1;
#endif

> +
>      for (i = 0; i < sizeof(target_ulong); i++) {
>          cfg_val = (val >> 8 * i)  & 0xff;
> -        pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i,
> -            cfg_val);
> +        pmp_write_cfg(env, pmp_entry_base + i, cfg_val);
>      }
>  }
>
> @@ -332,11 +338,18 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>  target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
>  {
>      int i;
> +    uint32_t pmp_entry_base;
>      target_ulong cfg_val = 0;
>      target_ulong val = 0;
>
> +#if defined(TARGET_RISCV32)
> +    pmp_entry_base = (reg_index * sizeof(target_ulong));
> +#elif defined(TARGET_RISCV64)
> +    pmp_entry_base = (reg_index >> 1) * sizeof(target_ulong);
> +#endif
> +
>      for (i = 0; i < sizeof(target_ulong); i++) {
> -        val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> +        val = pmp_read_cfg(env, pmp_entry_base + i);
>          cfg_val |= (val << (i * 8));
>      }
>      trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);

Regards,
Bin


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] target/riscv/pmp.c: Fix the index offset on RV64
@ 2020-07-21  6:11     ` Bin Meng
  0 siblings, 0 replies; 6+ messages in thread
From: Bin Meng @ 2020-07-21  6:11 UTC (permalink / raw)
  To: Zong Li
  Cc: Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
	Bastian Koppelmann, open list:RISC-V,
	qemu-devel@nongnu.org Developers

Hi Zong,

On Tue, Jul 21, 2020 at 2:03 PM Zong Li <zong.li@sifive.com> wrote:
>
> On RV64, the reg_index is 2 (pmpcfg2 CSR) after the seventh pmp
> entry, it is not 1 (pmpcfg1 CSR) like RV32. In the original
> implementation, the second parameter of pmp_write_cfg is
> "reg_index * sizeof(target_ulong)", and we get the the result
> which is started from 16 if reg_index is 2, but we expect that
> it should be started from 8. Separate the implementation for
> RV32 and RV64 respectively.
>
> Changed in v2:
>  - Move out the shifting operation from loop. Suggested by Bin Meng

The changelog should go after --- below

>
> Signed-off-by: Zong Li <zong.li@sifive.com>
> ---
>  target/riscv/pmp.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 2a2b9f5363..3de6535fbd 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -309,6 +309,7 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>  {
>      int i;
>      uint8_t cfg_val;
> +    uint32_t pmp_entry_base;
>
>      trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
>
> @@ -318,10 +319,15 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>          return;
>      }
>
> +#if defined(TARGET_RISCV32)
> +    pmp_entry_base = (reg_index * sizeof(target_ulong));
> +#elif defined(TARGET_RISCV64)
> +    pmp_entry_base = (reg_index >> 1) * sizeof(target_ulong);
> +#endif

This is not necessary. You can simply do:

#if defined(TARGET_RISCV64)
    reg_index >>= 1;
#endif

> +
>      for (i = 0; i < sizeof(target_ulong); i++) {
>          cfg_val = (val >> 8 * i)  & 0xff;
> -        pmp_write_cfg(env, (reg_index * sizeof(target_ulong)) + i,
> -            cfg_val);
> +        pmp_write_cfg(env, pmp_entry_base + i, cfg_val);
>      }
>  }
>
> @@ -332,11 +338,18 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>  target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
>  {
>      int i;
> +    uint32_t pmp_entry_base;
>      target_ulong cfg_val = 0;
>      target_ulong val = 0;
>
> +#if defined(TARGET_RISCV32)
> +    pmp_entry_base = (reg_index * sizeof(target_ulong));
> +#elif defined(TARGET_RISCV64)
> +    pmp_entry_base = (reg_index >> 1) * sizeof(target_ulong);
> +#endif
> +
>      for (i = 0; i < sizeof(target_ulong); i++) {
> -        val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> +        val = pmp_read_cfg(env, pmp_entry_base + i);
>          cfg_val |= (val << (i * 8));
>      }
>      trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);

Regards,
Bin


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-21  6:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21  6:03 [PATCH v2 0/2] Fix some PMP implementation Zong Li
2020-07-21  6:03 ` [PATCH v2 1/2] target/riscv: Fix the range of pmpcfg of CSR funcion table Zong Li
2020-07-21  6:03   ` Zong Li
2020-07-21  6:03 ` [PATCH v2 2/2] target/riscv/pmp.c: Fix the index offset on RV64 Zong Li
2020-07-21  6:11   ` Bin Meng
2020-07-21  6:11     ` Bin Meng

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.