* [PATCH v3 0/3] Fix some PMP implementation
@ 2020-07-21 12:40 Zong Li
2020-07-21 12:40 ` [PATCH v3 1/3] target/riscv: Fix the range of pmpcfg of CSR funcion table Zong Li
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Zong Li @ 2020-07-21 12:40 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. After 3rd version of this patch
series, we also fix the PMP wrong checking problem.
Changed in v3:
- Refine the implementation. Suggested by Bin Meng.
- Add fix for PMP wrong checking
Changed in v2:
- Move out the shifting operation from loop. Suggested by Bin Meng.
Zong Li (3):
target/riscv: Fix the range of pmpcfg of CSR funcion table
target/riscv/pmp.c: Fix the index offset on RV64
target/riscv: Fix the translation of physical address
target/riscv/cpu_helper.c | 3 ++-
target/riscv/csr.c | 2 +-
target/riscv/pmp.c | 8 ++++++++
3 files changed, 11 insertions(+), 2 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/3] target/riscv: Fix the range of pmpcfg of CSR funcion table
2020-07-21 12:40 [PATCH v3 0/3] Fix some PMP implementation Zong Li
@ 2020-07-21 12:40 ` Zong Li
2020-07-21 12:40 ` [PATCH v3 2/3] target/riscv/pmp.c: Fix the index offset on RV64 Zong Li
2020-07-21 12:40 ` [PATCH v3 3/3] target/riscv: Fix the translation of physical address Zong Li
2 siblings, 0 replies; 8+ messages in thread
From: Zong Li @ 2020-07-21 12:40 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] 8+ messages in thread
* [PATCH v3 2/3] target/riscv/pmp.c: Fix the index offset on RV64
2020-07-21 12:40 [PATCH v3 0/3] Fix some PMP implementation Zong Li
2020-07-21 12:40 ` [PATCH v3 1/3] target/riscv: Fix the range of pmpcfg of CSR funcion table Zong Li
@ 2020-07-21 12:40 ` Zong Li
2020-07-22 4:57 ` Bin Meng
2020-07-21 12:40 ` [PATCH v3 3/3] target/riscv: Fix the translation of physical address Zong Li
2 siblings, 1 reply; 8+ messages in thread
From: Zong Li @ 2020-07-21 12:40 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.
Signed-off-by: Zong Li <zong.li@sifive.com>
Changed in v3:
- Refine the implementation. Suggested by Bin Meng.
Changed in v2:
- Move out the shifting operation from loop. Suggested by Bin Meng.
---
target/riscv/pmp.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 2a2b9f5363..f2d50bace5 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -318,6 +318,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
return;
}
+#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,
@@ -335,6 +339,10 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
target_ulong cfg_val = 0;
target_ulong val = 0;
+#if defined(TARGET_RISCV64)
+ reg_index >>= 1;
+#endif
+
for (i = 0; i < sizeof(target_ulong); i++) {
val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
cfg_val |= (val << (i * 8));
--
2.27.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 3/3] target/riscv: Fix the translation of physical address
2020-07-21 12:40 [PATCH v3 0/3] Fix some PMP implementation Zong Li
2020-07-21 12:40 ` [PATCH v3 1/3] target/riscv: Fix the range of pmpcfg of CSR funcion table Zong Li
2020-07-21 12:40 ` [PATCH v3 2/3] target/riscv/pmp.c: Fix the index offset on RV64 Zong Li
@ 2020-07-21 12:40 ` Zong Li
2020-07-22 9:08 ` Alexander Richardson
2 siblings, 1 reply; 8+ messages in thread
From: Zong Li @ 2020-07-21 12:40 UTC (permalink / raw)
To: palmer, Alistair.Francis, bmeng.cn, sagark, kbastian, qemu-riscv,
qemu-devel
Cc: Zong Li
The real physical address should add the 12 bits page offset. It also
causes the PMP wrong checking due to the minimum granularity of PMP is
4 byte, but we always get the physical address which is 4KB alignment,
that means, we always use the start address of the page to check PMP for
all addresses which in the same page.
Signed-off-by: Zong Li <zong.li@sifive.com>
---
target/riscv/cpu_helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 75d2ae3434..08b069f0c9 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -543,7 +543,8 @@ restart:
/* for superpage mappings, make a fake leaf PTE for the TLB's
benefit. */
target_ulong vpn = addr >> PGSHIFT;
- *physical = (ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT;
+ *physical = ((ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT) |
+ (addr & ~TARGET_PAGE_MASK);
/* set permissions on the TLB entry */
if ((pte & PTE_R) || ((pte & PTE_X) && mxr)) {
--
2.27.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] target/riscv/pmp.c: Fix the index offset on RV64
2020-07-21 12:40 ` [PATCH v3 2/3] target/riscv/pmp.c: Fix the index offset on RV64 Zong Li
@ 2020-07-22 4:57 ` Bin Meng
2020-07-23 3:19 ` Zong Li
0 siblings, 1 reply; 8+ messages in thread
From: Bin Meng @ 2020-07-22 4:57 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 8:41 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.
>
> Signed-off-by: Zong Li <zong.li@sifive.com>
>
> Changed in v3:
> - Refine the implementation. Suggested by Bin Meng.
>
> Changed in v2:
> - Move out the shifting operation from loop. Suggested by Bin Meng.
As I mentioned previously, these changelog should go after --- below.
It should not appear in the commit message.
> ---
> target/riscv/pmp.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 2a2b9f5363..f2d50bace5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -318,6 +318,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
> return;
> }
>
> +#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,
> @@ -335,6 +339,10 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
> target_ulong cfg_val = 0;
> target_ulong val = 0;
>
> +#if defined(TARGET_RISCV64)
> + reg_index >>= 1;
> +#endif
We should also move the following:
trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);
before shifting reg_index. Otherwise it traces the wrong pmpcfg CSR read.
> +
> for (i = 0; i < sizeof(target_ulong); i++) {
> val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> cfg_val |= (val << (i * 8));
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 3/3] target/riscv: Fix the translation of physical address
2020-07-21 12:40 ` [PATCH v3 3/3] target/riscv: Fix the translation of physical address Zong Li
@ 2020-07-22 9:08 ` Alexander Richardson
2020-07-23 3:20 ` Zong Li
0 siblings, 1 reply; 8+ messages in thread
From: Alexander Richardson @ 2020-07-22 9:08 UTC (permalink / raw)
To: Zong Li
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt, Bin Meng
On Tue, 21 Jul 2020 at 13:43, Zong Li <zong.li@sifive.com> wrote:
>
> The real physical address should add the 12 bits page offset. It also
> causes the PMP wrong checking due to the minimum granularity of PMP is
> 4 byte, but we always get the physical address which is 4KB alignment,
> that means, we always use the start address of the page to check PMP for
> all addresses which in the same page.
>
> Signed-off-by: Zong Li <zong.li@sifive.com>
> ---
> target/riscv/cpu_helper.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 75d2ae3434..08b069f0c9 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -543,7 +543,8 @@ restart:
> /* for superpage mappings, make a fake leaf PTE for the TLB's
> benefit. */
> target_ulong vpn = addr >> PGSHIFT;
> - *physical = (ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT;
> + *physical = ((ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT) |
> + (addr & ~TARGET_PAGE_MASK);
>
> /* set permissions on the TLB entry */
> if ((pte & PTE_R) || ((pte & PTE_X) && mxr)) {
> --
> 2.27.0
I made the same change for our CHERI fork a few months ago but forgot
to send the patch upstream (despite marking the commit as a candidate
for upstreaming). Sorry about the duplicated debugging work!
(https://github.com/CTSRD-CHERI/qemu/commit/61c8e3f2c0fd4965ec3f316146d1751fae673c12)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/3] target/riscv/pmp.c: Fix the index offset on RV64
2020-07-22 4:57 ` Bin Meng
@ 2020-07-23 3:19 ` Zong Li
0 siblings, 0 replies; 8+ messages in thread
From: Zong Li @ 2020-07-23 3:19 UTC (permalink / raw)
To: Bin Meng
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt
On Wed, Jul 22, 2020 at 12:58 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Zong,
>
> On Tue, Jul 21, 2020 at 8:41 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.
> >
> > Signed-off-by: Zong Li <zong.li@sifive.com>
> >
> > Changed in v3:
> > - Refine the implementation. Suggested by Bin Meng.
> >
> > Changed in v2:
> > - Move out the shifting operation from loop. Suggested by Bin Meng.
>
> As I mentioned previously, these changelog should go after --- below.
> It should not appear in the commit message.
>
OK, remove it in the next version.
> > ---
> > target/riscv/pmp.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index 2a2b9f5363..f2d50bace5 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -318,6 +318,10 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
> > return;
> > }
> >
> > +#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,
> > @@ -335,6 +339,10 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t reg_index)
> > target_ulong cfg_val = 0;
> > target_ulong val = 0;
> >
> > +#if defined(TARGET_RISCV64)
> > + reg_index >>= 1;
> > +#endif
>
> We should also move the following:
>
> trace_pmpcfg_csr_read(env->mhartid, reg_index, cfg_val);
>
> before shifting reg_index. Otherwise it traces the wrong pmpcfg CSR read.
Yes, thanks for the reminding, Fix it in the next version.
>
> > +
> > for (i = 0; i < sizeof(target_ulong); i++) {
> > val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
> > cfg_val |= (val << (i * 8));
> > --
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 3/3] target/riscv: Fix the translation of physical address
2020-07-22 9:08 ` Alexander Richardson
@ 2020-07-23 3:20 ` Zong Li
0 siblings, 0 replies; 8+ messages in thread
From: Zong Li @ 2020-07-23 3:20 UTC (permalink / raw)
To: Alexander Richardson
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt, Bin Meng
On Wed, Jul 22, 2020 at 5:08 PM Alexander Richardson
<Alexander.Richardson@cl.cam.ac.uk> wrote:
>
> On Tue, 21 Jul 2020 at 13:43, Zong Li <zong.li@sifive.com> wrote:
> >
> > The real physical address should add the 12 bits page offset. It also
> > causes the PMP wrong checking due to the minimum granularity of PMP is
> > 4 byte, but we always get the physical address which is 4KB alignment,
> > that means, we always use the start address of the page to check PMP for
> > all addresses which in the same page.
> >
> > Signed-off-by: Zong Li <zong.li@sifive.com>
> > ---
> > target/riscv/cpu_helper.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 75d2ae3434..08b069f0c9 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -543,7 +543,8 @@ restart:
> > /* for superpage mappings, make a fake leaf PTE for the TLB's
> > benefit. */
> > target_ulong vpn = addr >> PGSHIFT;
> > - *physical = (ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT;
> > + *physical = ((ppn | (vpn & ((1L << ptshift) - 1))) << PGSHIFT) |
> > + (addr & ~TARGET_PAGE_MASK);
> >
> > /* set permissions on the TLB entry */
> > if ((pte & PTE_R) || ((pte & PTE_X) && mxr)) {
> > --
> > 2.27.0
>
> I made the same change for our CHERI fork a few months ago but forgot
> to send the patch upstream (despite marking the commit as a candidate
> for upstreaming). Sorry about the duplicated debugging work!
> (https://github.com/CTSRD-CHERI/qemu/commit/61c8e3f2c0fd4965ec3f316146d1751fae673c12)
No, problem.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-23 3:21 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 12:40 [PATCH v3 0/3] Fix some PMP implementation Zong Li
2020-07-21 12:40 ` [PATCH v3 1/3] target/riscv: Fix the range of pmpcfg of CSR funcion table Zong Li
2020-07-21 12:40 ` [PATCH v3 2/3] target/riscv/pmp.c: Fix the index offset on RV64 Zong Li
2020-07-22 4:57 ` Bin Meng
2020-07-23 3:19 ` Zong Li
2020-07-21 12:40 ` [PATCH v3 3/3] target/riscv: Fix the translation of physical address Zong Li
2020-07-22 9:08 ` Alexander Richardson
2020-07-23 3:20 ` Zong Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).