[Qemu-devel] [PATCHv3 1/5] RISC-V: Only Check PMP if MMU translation succeeds

[Qemu-devel] [PATCHv3 1/5] RISC-V: Only Check PMP if MMU translation succeeds

[Hesham Almatary]

The current implementation unnecessarily checks for PMP even if MMU translation
failed. This may trigger a wrong PMP access exception instead of
a page exception.

For example, the very first instruction fetched after the first satp write in
S-Mode will trigger a PMP access fault instead of an instruction fetch page
fault.

This patch prioritises MMU exceptions over PMP exceptions and only checks for
PMP if MMU translation succeeds.

Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/cpu_helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 41d6db41c3..40fb47e794 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -401,6 +401,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                   " prot %d\n", __func__, address, ret, pa, prot);

     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
+        (ret == TRANSLATE_SUCCESS) &&
         !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
         ret = TRANSLATE_FAIL;
     }
--
2.17.1

[Qemu-riscv] [PATCHv3 1/5] RISC-V: Only Check PMP if MMU translation succeeds

[Hesham Almatary]

The current implementation unnecessarily checks for PMP even if MMU translation
failed. This may trigger a wrong PMP access exception instead of
a page exception.

For example, the very first instruction fetched after the first satp write in
S-Mode will trigger a PMP access fault instead of an instruction fetch page
fault.

This patch prioritises MMU exceptions over PMP exceptions and only checks for
PMP if MMU translation succeeds.

Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/cpu_helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 41d6db41c3..40fb47e794 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -401,6 +401,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                   " prot %d\n", __func__, address, ret, pa, prot);

     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
+        (ret == TRANSLATE_SUCCESS) &&
         !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
         ret = TRANSLATE_FAIL;
     }
--
2.17.1

[Qemu-devel] [PATCHv3 2/5] RISC-V: Raise access fault exceptions on PMP violations

[Hesham Almatary]

Section 3.6 in RISC-V v1.10 privilege specification states that PMP violations
report "access exceptions." The current PMP implementation has
a bug which wrongly reports "page exceptions" on PMP violations.

This patch fixes this bug by reporting the correct PMP access exceptions
trap values.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/cpu_helper.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 40fb47e794..7c7282c680 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -318,12 +318,13 @@ restart:
 }

 static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
-                                MMUAccessType access_type)
+                                MMUAccessType access_type, bool pmp_violation)
 {
     CPUState *cs = CPU(riscv_env_get_cpu(env));
     int page_fault_exceptions =
         (env->priv_ver >= PRIV_VERSION_1_10_0) &&
-        get_field(env->satp, SATP_MODE) != VM_1_10_MBARE;
+        get_field(env->satp, SATP_MODE) != VM_1_10_MBARE &&
+        !pmp_violation;
     switch (access_type) {
     case MMU_INST_FETCH:
         cs->exception_index = page_fault_exceptions ?
@@ -389,6 +390,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     CPURISCVState *env = &cpu->env;
     hwaddr pa = 0;
     int prot;
+    bool pmp_violation = false;
     int ret = TRANSLATE_FAIL;

     qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
@@ -403,6 +405,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
         (ret == TRANSLATE_SUCCESS) &&
         !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
+        pmp_violation = true;
         ret = TRANSLATE_FAIL;
     }
     if (ret == TRANSLATE_SUCCESS) {
@@ -412,7 +415,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     } else if (probe) {
         return false;
     } else {
-        raise_mmu_exception(env, address, access_type);
+        raise_mmu_exception(env, address, access_type, pmp_violation);
         riscv_raise_exception(env, cs->exception_index, retaddr);
     }
 #else
--
2.17.1

[Qemu-riscv] [PATCHv3 2/5] RISC-V: Raise access fault exceptions on PMP violations

[Hesham Almatary]

Section 3.6 in RISC-V v1.10 privilege specification states that PMP violations
report "access exceptions." The current PMP implementation has
a bug which wrongly reports "page exceptions" on PMP violations.

This patch fixes this bug by reporting the correct PMP access exceptions
trap values.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/cpu_helper.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 40fb47e794..7c7282c680 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -318,12 +318,13 @@ restart:
 }

 static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
-                                MMUAccessType access_type)
+                                MMUAccessType access_type, bool pmp_violation)
 {
     CPUState *cs = CPU(riscv_env_get_cpu(env));
     int page_fault_exceptions =
         (env->priv_ver >= PRIV_VERSION_1_10_0) &&
-        get_field(env->satp, SATP_MODE) != VM_1_10_MBARE;
+        get_field(env->satp, SATP_MODE) != VM_1_10_MBARE &&
+        !pmp_violation;
     switch (access_type) {
     case MMU_INST_FETCH:
         cs->exception_index = page_fault_exceptions ?
@@ -389,6 +390,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     CPURISCVState *env = &cpu->env;
     hwaddr pa = 0;
     int prot;
+    bool pmp_violation = false;
     int ret = TRANSLATE_FAIL;

     qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
@@ -403,6 +405,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
         (ret == TRANSLATE_SUCCESS) &&
         !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
+        pmp_violation = true;
         ret = TRANSLATE_FAIL;
     }
     if (ret == TRANSLATE_SUCCESS) {
@@ -412,7 +415,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     } else if (probe) {
         return false;
     } else {
-        raise_mmu_exception(env, address, access_type);
+        raise_mmu_exception(env, address, access_type, pmp_violation);
         riscv_raise_exception(env, cs->exception_index, retaddr);
     }
 #else
--
2.17.1

[Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

The PMP should be checked when doing a page table walk, and report access
fault exception if the to-be-read PTE failed the PMP check.

Suggested-by: Jonathan Behrens <fintelia@gmail.com>
Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/cpu.h        |  1 +
 target/riscv/cpu_helper.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index c17184f4e4..ab3ba3f15a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -94,6 +94,7 @@ enum {
 #define PRIV_VERSION_1_09_1 0x00010901
 #define PRIV_VERSION_1_10_0 0x00011000

+#define TRANSLATE_PMP_FAIL 2
 #define TRANSLATE_FAIL 1
 #define TRANSLATE_SUCCESS 0
 #define NB_MMU_MODES 4
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 7c7282c680..d0b0f9cf88 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -211,6 +211,12 @@ restart:

         /* check that physical address of PTE is legal */
         target_ulong 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)) {
+            return TRANSLATE_PMP_FAIL;
+        }
 #if defined(TARGET_RISCV32)
         target_ulong pte = ldl_phys(cs->as, pte_addr);
 #elif defined(TARGET_RISCV64)
@@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
         (ret == TRANSLATE_SUCCESS) &&
         !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
+        ret = TRANSLATE_PMP_FAIL;
+    }
+    if (ret == TRANSLATE_PMP_FAIL) {
         pmp_violation = true;
-        ret = TRANSLATE_FAIL;
     }
     if (ret == TRANSLATE_SUCCESS) {
         tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
--
2.17.1

[Qemu-riscv] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

The PMP should be checked when doing a page table walk, and report access
fault exception if the to-be-read PTE failed the PMP check.

Suggested-by: Jonathan Behrens <fintelia@gmail.com>
Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/cpu.h        |  1 +
 target/riscv/cpu_helper.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index c17184f4e4..ab3ba3f15a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -94,6 +94,7 @@ enum {
 #define PRIV_VERSION_1_09_1 0x00010901
 #define PRIV_VERSION_1_10_0 0x00011000

+#define TRANSLATE_PMP_FAIL 2
 #define TRANSLATE_FAIL 1
 #define TRANSLATE_SUCCESS 0
 #define NB_MMU_MODES 4
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 7c7282c680..d0b0f9cf88 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -211,6 +211,12 @@ restart:

         /* check that physical address of PTE is legal */
         target_ulong 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)) {
+            return TRANSLATE_PMP_FAIL;
+        }
 #if defined(TARGET_RISCV32)
         target_ulong pte = ldl_phys(cs->as, pte_addr);
 #elif defined(TARGET_RISCV64)
@@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
         (ret == TRANSLATE_SUCCESS) &&
         !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
+        ret = TRANSLATE_PMP_FAIL;
+    }
+    if (ret == TRANSLATE_PMP_FAIL) {
         pmp_violation = true;
-        ret = TRANSLATE_FAIL;
     }
     if (ret == TRANSLATE_SUCCESS) {
         tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
--
2.17.1

Re: [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Alistair Francis]

On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The PMP should be checked when doing a page table walk, and report access
> fault exception if the to-be-read PTE failed the PMP check.
>
> Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> ---
>  target/riscv/cpu.h        |  1 +
>  target/riscv/cpu_helper.c | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index c17184f4e4..ab3ba3f15a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -94,6 +94,7 @@ enum {
>  #define PRIV_VERSION_1_09_1 0x00010901
>  #define PRIV_VERSION_1_10_0 0x00011000
>
> +#define TRANSLATE_PMP_FAIL 2
>  #define TRANSLATE_FAIL 1
>  #define TRANSLATE_SUCCESS 0
>  #define NB_MMU_MODES 4
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 7c7282c680..d0b0f9cf88 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -211,6 +211,12 @@ restart:
>
>          /* check that physical address of PTE is legal */
>          target_ulong 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)) {

I see a problem here.

pmp_hart_has_privs() checks permissions based on the current value of
env->priv. This might not always be the correct permissions to check,
we should instead use the current mode to check permissions.

The best way to do this to me is to probably provide a privileged mode
override to the function, can you add that?

Alistair

> +            return TRANSLATE_PMP_FAIL;
> +        }
>  #if defined(TARGET_RISCV32)
>          target_ulong pte = ldl_phys(cs->as, pte_addr);
>  #elif defined(TARGET_RISCV64)
> @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>          (ret == TRANSLATE_SUCCESS) &&
>          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> +        ret = TRANSLATE_PMP_FAIL;
> +    }
> +    if (ret == TRANSLATE_PMP_FAIL) {
>          pmp_violation = true;
> -        ret = TRANSLATE_FAIL;
>      }
>      if (ret == TRANSLATE_SUCCESS) {
>          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> --
> 2.17.1
>
>

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Alistair Francis]

On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The PMP should be checked when doing a page table walk, and report access
> fault exception if the to-be-read PTE failed the PMP check.
>
> Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> ---
>  target/riscv/cpu.h        |  1 +
>  target/riscv/cpu_helper.c | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index c17184f4e4..ab3ba3f15a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -94,6 +94,7 @@ enum {
>  #define PRIV_VERSION_1_09_1 0x00010901
>  #define PRIV_VERSION_1_10_0 0x00011000
>
> +#define TRANSLATE_PMP_FAIL 2
>  #define TRANSLATE_FAIL 1
>  #define TRANSLATE_SUCCESS 0
>  #define NB_MMU_MODES 4
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 7c7282c680..d0b0f9cf88 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -211,6 +211,12 @@ restart:
>
>          /* check that physical address of PTE is legal */
>          target_ulong 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)) {

I see a problem here.

pmp_hart_has_privs() checks permissions based on the current value of
env->priv. This might not always be the correct permissions to check,
we should instead use the current mode to check permissions.

The best way to do this to me is to probably provide a privileged mode
override to the function, can you add that?

Alistair

> +            return TRANSLATE_PMP_FAIL;
> +        }
>  #if defined(TARGET_RISCV32)
>          target_ulong pte = ldl_phys(cs->as, pte_addr);
>  #elif defined(TARGET_RISCV64)
> @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>          (ret == TRANSLATE_SUCCESS) &&
>          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> +        ret = TRANSLATE_PMP_FAIL;
> +    }
> +    if (ret == TRANSLATE_PMP_FAIL) {
>          pmp_violation = true;
> -        ret = TRANSLATE_FAIL;
>      }
>      if (ret == TRANSLATE_SUCCESS) {
>          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> --
> 2.17.1
>
>

Re: [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> <Hesham.Almatary@cl.cam.ac.uk> wrote:
> >
> > The PMP should be checked when doing a page table walk, and report access
> > fault exception if the to-be-read PTE failed the PMP check.
> >
> > Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> > ---
> >  target/riscv/cpu.h        |  1 +
> >  target/riscv/cpu_helper.c | 10 +++++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index c17184f4e4..ab3ba3f15a 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -94,6 +94,7 @@ enum {
> >  #define PRIV_VERSION_1_09_1 0x00010901
> >  #define PRIV_VERSION_1_10_0 0x00011000
> >
> > +#define TRANSLATE_PMP_FAIL 2
> >  #define TRANSLATE_FAIL 1
> >  #define TRANSLATE_SUCCESS 0
> >  #define NB_MMU_MODES 4
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 7c7282c680..d0b0f9cf88 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -211,6 +211,12 @@ restart:
> >
> >          /* check that physical address of PTE is legal */
> >          target_ulong 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)) {
>
> I see a problem here.
>
> pmp_hart_has_privs() checks permissions based on the current value of
> env->priv. This might not always be the correct permissions to check,
> we should instead use the current mode to check permissions.
>
That is not clear to me. Isn't env->priv the current privildge mode?
Could you please elaborate on what other cases this might not be the case?

> The best way to do this to me is to probably provide a privileged mode
> override to the function, can you add that?
>
> Alistair
>
> > +            return TRANSLATE_PMP_FAIL;
> > +        }
> >  #if defined(TARGET_RISCV32)
> >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> >  #elif defined(TARGET_RISCV64)
> > @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> >          (ret == TRANSLATE_SUCCESS) &&
> >          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> > +        ret = TRANSLATE_PMP_FAIL;
> > +    }
> > +    if (ret == TRANSLATE_PMP_FAIL) {
> >          pmp_violation = true;
> > -        ret = TRANSLATE_FAIL;
> >      }
> >      if (ret == TRANSLATE_SUCCESS) {
> >          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> > --
> > 2.17.1
> >
> >

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> <Hesham.Almatary@cl.cam.ac.uk> wrote:
> >
> > The PMP should be checked when doing a page table walk, and report access
> > fault exception if the to-be-read PTE failed the PMP check.
> >
> > Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> > ---
> >  target/riscv/cpu.h        |  1 +
> >  target/riscv/cpu_helper.c | 10 +++++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index c17184f4e4..ab3ba3f15a 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -94,6 +94,7 @@ enum {
> >  #define PRIV_VERSION_1_09_1 0x00010901
> >  #define PRIV_VERSION_1_10_0 0x00011000
> >
> > +#define TRANSLATE_PMP_FAIL 2
> >  #define TRANSLATE_FAIL 1
> >  #define TRANSLATE_SUCCESS 0
> >  #define NB_MMU_MODES 4
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 7c7282c680..d0b0f9cf88 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -211,6 +211,12 @@ restart:
> >
> >          /* check that physical address of PTE is legal */
> >          target_ulong 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)) {
>
> I see a problem here.
>
> pmp_hart_has_privs() checks permissions based on the current value of
> env->priv. This might not always be the correct permissions to check,
> we should instead use the current mode to check permissions.
>
That is not clear to me. Isn't env->priv the current privildge mode?
Could you please elaborate on what other cases this might not be the case?

> The best way to do this to me is to probably provide a privileged mode
> override to the function, can you add that?
>
> Alistair
>
> > +            return TRANSLATE_PMP_FAIL;
> > +        }
> >  #if defined(TARGET_RISCV32)
> >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> >  #elif defined(TARGET_RISCV64)
> > @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> >          (ret == TRANSLATE_SUCCESS) &&
> >          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> > +        ret = TRANSLATE_PMP_FAIL;
> > +    }
> > +    if (ret == TRANSLATE_PMP_FAIL) {
> >          pmp_violation = true;
> > -        ret = TRANSLATE_FAIL;
> >      }
> >      if (ret == TRANSLATE_SUCCESS) {
> >          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> > --
> > 2.17.1
> >
> >

Re: [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

ping

On Wed, 22 May 2019 at 11:26, Hesham Almatary
<hesham.almatary@cl.cam.ac.uk> wrote:
>
> On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> > <Hesham.Almatary@cl.cam.ac.uk> wrote:
> > >
> > > The PMP should be checked when doing a page table walk, and report access
> > > fault exception if the to-be-read PTE failed the PMP check.
> > >
> > > Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> > > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> > > ---
> > >  target/riscv/cpu.h        |  1 +
> > >  target/riscv/cpu_helper.c | 10 +++++++++-
> > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index c17184f4e4..ab3ba3f15a 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -94,6 +94,7 @@ enum {
> > >  #define PRIV_VERSION_1_09_1 0x00010901
> > >  #define PRIV_VERSION_1_10_0 0x00011000
> > >
> > > +#define TRANSLATE_PMP_FAIL 2
> > >  #define TRANSLATE_FAIL 1
> > >  #define TRANSLATE_SUCCESS 0
> > >  #define NB_MMU_MODES 4
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 7c7282c680..d0b0f9cf88 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -211,6 +211,12 @@ restart:
> > >
> > >          /* check that physical address of PTE is legal */
> > >          target_ulong 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)) {
> >
> > I see a problem here.
> >
> > pmp_hart_has_privs() checks permissions based on the current value of
> > env->priv. This might not always be the correct permissions to check,
> > we should instead use the current mode to check permissions.
> >
> That is not clear to me. Isn't env->priv the current privildge mode?
> Could you please elaborate on what other cases this might not be the case?
>
> > The best way to do this to me is to probably provide a privileged mode
> > override to the function, can you add that?
> >
> > Alistair
> >
> > > +            return TRANSLATE_PMP_FAIL;
> > > +        }
> > >  #if defined(TARGET_RISCV32)
> > >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> > >  #elif defined(TARGET_RISCV64)
> > > @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> > >          (ret == TRANSLATE_SUCCESS) &&
> > >          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> > > +        ret = TRANSLATE_PMP_FAIL;
> > > +    }
> > > +    if (ret == TRANSLATE_PMP_FAIL) {
> > >          pmp_violation = true;
> > > -        ret = TRANSLATE_FAIL;
> > >      }
> > >      if (ret == TRANSLATE_SUCCESS) {
> > >          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> > > --
> > > 2.17.1
> > >
> > >

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

ping

On Wed, 22 May 2019 at 11:26, Hesham Almatary
<hesham.almatary@cl.cam.ac.uk> wrote:
>
> On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> > <Hesham.Almatary@cl.cam.ac.uk> wrote:
> > >
> > > The PMP should be checked when doing a page table walk, and report access
> > > fault exception if the to-be-read PTE failed the PMP check.
> > >
> > > Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> > > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> > > ---
> > >  target/riscv/cpu.h        |  1 +
> > >  target/riscv/cpu_helper.c | 10 +++++++++-
> > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index c17184f4e4..ab3ba3f15a 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -94,6 +94,7 @@ enum {
> > >  #define PRIV_VERSION_1_09_1 0x00010901
> > >  #define PRIV_VERSION_1_10_0 0x00011000
> > >
> > > +#define TRANSLATE_PMP_FAIL 2
> > >  #define TRANSLATE_FAIL 1
> > >  #define TRANSLATE_SUCCESS 0
> > >  #define NB_MMU_MODES 4
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 7c7282c680..d0b0f9cf88 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -211,6 +211,12 @@ restart:
> > >
> > >          /* check that physical address of PTE is legal */
> > >          target_ulong 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)) {
> >
> > I see a problem here.
> >
> > pmp_hart_has_privs() checks permissions based on the current value of
> > env->priv. This might not always be the correct permissions to check,
> > we should instead use the current mode to check permissions.
> >
> That is not clear to me. Isn't env->priv the current privildge mode?
> Could you please elaborate on what other cases this might not be the case?
>
> > The best way to do this to me is to probably provide a privileged mode
> > override to the function, can you add that?
> >
> > Alistair
> >
> > > +            return TRANSLATE_PMP_FAIL;
> > > +        }
> > >  #if defined(TARGET_RISCV32)
> > >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> > >  #elif defined(TARGET_RISCV64)
> > > @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> > >          (ret == TRANSLATE_SUCCESS) &&
> > >          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> > > +        ret = TRANSLATE_PMP_FAIL;
> > > +    }
> > > +    if (ret == TRANSLATE_PMP_FAIL) {
> > >          pmp_violation = true;
> > > -        ret = TRANSLATE_FAIL;
> > >      }
> > >      if (ret == TRANSLATE_SUCCESS) {
> > >          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> > > --
> > > 2.17.1
> > >
> > >

Re: [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Alistair Francis]

On Wed, May 22, 2019 at 2:27 AM Hesham Almatary
<hesham.almatary@cl.cam.ac.uk> wrote:
>
> On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> > <Hesham.Almatary@cl.cam.ac.uk> wrote:
> > >
> > > The PMP should be checked when doing a page table walk, and report access
> > > fault exception if the to-be-read PTE failed the PMP check.
> > >
> > > Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> > > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> > > ---
> > >  target/riscv/cpu.h        |  1 +
> > >  target/riscv/cpu_helper.c | 10 +++++++++-
> > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index c17184f4e4..ab3ba3f15a 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -94,6 +94,7 @@ enum {
> > >  #define PRIV_VERSION_1_09_1 0x00010901
> > >  #define PRIV_VERSION_1_10_0 0x00011000
> > >
> > > +#define TRANSLATE_PMP_FAIL 2
> > >  #define TRANSLATE_FAIL 1
> > >  #define TRANSLATE_SUCCESS 0
> > >  #define NB_MMU_MODES 4
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 7c7282c680..d0b0f9cf88 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -211,6 +211,12 @@ restart:
> > >
> > >          /* check that physical address of PTE is legal */
> > >          target_ulong 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)) {
> >
> > I see a problem here.
> >
> > pmp_hart_has_privs() checks permissions based on the current value of
> > env->priv. This might not always be the correct permissions to check,
> > we should instead use the current mode to check permissions.
> >
> That is not clear to me. Isn't env->priv the current privildge mode?
> Could you please elaborate on what other cases this might not be the case?

Sorry for the delay. The RISC-V Hypervisor Extension allows load/store
operations to be carried out as a previous privilege. The mstatus.MPRV
and hstatus.SPRV allow this.

Alistair

>
> > The best way to do this to me is to probably provide a privileged mode
> > override to the function, can you add that?
> >
> > Alistair
> >
> > > +            return TRANSLATE_PMP_FAIL;
> > > +        }
> > >  #if defined(TARGET_RISCV32)
> > >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> > >  #elif defined(TARGET_RISCV64)
> > > @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> > >          (ret == TRANSLATE_SUCCESS) &&
> > >          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> > > +        ret = TRANSLATE_PMP_FAIL;
> > > +    }
> > > +    if (ret == TRANSLATE_PMP_FAIL) {
> > >          pmp_violation = true;
> > > -        ret = TRANSLATE_FAIL;
> > >      }
> > >      if (ret == TRANSLATE_SUCCESS) {
> > >          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> > > --
> > > 2.17.1
> > >
> > >

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Alistair Francis]

On Wed, May 22, 2019 at 2:27 AM Hesham Almatary
<hesham.almatary@cl.cam.ac.uk> wrote:
>
> On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> > <Hesham.Almatary@cl.cam.ac.uk> wrote:
> > >
> > > The PMP should be checked when doing a page table walk, and report access
> > > fault exception if the to-be-read PTE failed the PMP check.
> > >
> > > Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> > > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> > > ---
> > >  target/riscv/cpu.h        |  1 +
> > >  target/riscv/cpu_helper.c | 10 +++++++++-
> > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index c17184f4e4..ab3ba3f15a 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -94,6 +94,7 @@ enum {
> > >  #define PRIV_VERSION_1_09_1 0x00010901
> > >  #define PRIV_VERSION_1_10_0 0x00011000
> > >
> > > +#define TRANSLATE_PMP_FAIL 2
> > >  #define TRANSLATE_FAIL 1
> > >  #define TRANSLATE_SUCCESS 0
> > >  #define NB_MMU_MODES 4
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 7c7282c680..d0b0f9cf88 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -211,6 +211,12 @@ restart:
> > >
> > >          /* check that physical address of PTE is legal */
> > >          target_ulong 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)) {
> >
> > I see a problem here.
> >
> > pmp_hart_has_privs() checks permissions based on the current value of
> > env->priv. This might not always be the correct permissions to check,
> > we should instead use the current mode to check permissions.
> >
> That is not clear to me. Isn't env->priv the current privildge mode?
> Could you please elaborate on what other cases this might not be the case?

Sorry for the delay. The RISC-V Hypervisor Extension allows load/store
operations to be carried out as a previous privilege. The mstatus.MPRV
and hstatus.SPRV allow this.

Alistair

>
> > The best way to do this to me is to probably provide a privileged mode
> > override to the function, can you add that?
> >
> > Alistair
> >
> > > +            return TRANSLATE_PMP_FAIL;
> > > +        }
> > >  #if defined(TARGET_RISCV32)
> > >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> > >  #elif defined(TARGET_RISCV64)
> > > @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> > >          (ret == TRANSLATE_SUCCESS) &&
> > >          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> > > +        ret = TRANSLATE_PMP_FAIL;
> > > +    }
> > > +    if (ret == TRANSLATE_PMP_FAIL) {
> > >          pmp_violation = true;
> > > -        ret = TRANSLATE_FAIL;
> > >      }
> > >      if (ret == TRANSLATE_SUCCESS) {
> > >          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> > > --
> > > 2.17.1
> > >
> > >

Re: [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

On Thu, 30 May 2019 at 05:07, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, May 22, 2019 at 2:27 AM Hesham Almatary
> <hesham.almatary@cl.cam.ac.uk> wrote:
> >
> > On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> > > <Hesham.Almatary@cl.cam.ac.uk> wrote:
> > > >
> > > > The PMP should be checked when doing a page table walk, and report access
> > > > fault exception if the to-be-read PTE failed the PMP check.
> > > >
> > > > Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> > > > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> > > > ---
> > > >  target/riscv/cpu.h        |  1 +
> > > >  target/riscv/cpu_helper.c | 10 +++++++++-
> > > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > index c17184f4e4..ab3ba3f15a 100644
> > > > --- a/target/riscv/cpu.h
> > > > +++ b/target/riscv/cpu.h
> > > > @@ -94,6 +94,7 @@ enum {
> > > >  #define PRIV_VERSION_1_09_1 0x00010901
> > > >  #define PRIV_VERSION_1_10_0 0x00011000
> > > >
> > > > +#define TRANSLATE_PMP_FAIL 2
> > > >  #define TRANSLATE_FAIL 1
> > > >  #define TRANSLATE_SUCCESS 0
> > > >  #define NB_MMU_MODES 4
> > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > > index 7c7282c680..d0b0f9cf88 100644
> > > > --- a/target/riscv/cpu_helper.c
> > > > +++ b/target/riscv/cpu_helper.c
> > > > @@ -211,6 +211,12 @@ restart:
> > > >
> > > >          /* check that physical address of PTE is legal */
> > > >          target_ulong 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)) {
> > >
> > > I see a problem here.
> > >
> > > pmp_hart_has_privs() checks permissions based on the current value of
> > > env->priv. This might not always be the correct permissions to check,
> > > we should instead use the current mode to check permissions.
> > >
> > That is not clear to me. Isn't env->priv the current privildge mode?
> > Could you please elaborate on what other cases this might not be the case?
>
> Sorry for the delay. The RISC-V Hypervisor Extension allows load/store
> operations to be carried out as a previous privilege. The mstatus.MPRV
> and hstatus.SPRV allow this.
>
No problem, thanks for the clarification.
You are right, I haven't considered MPRV. I fixed that in a separate
commit in a v4 series of patches.
> Alistair
>
> >
> > > The best way to do this to me is to probably provide a privileged mode
> > > override to the function, can you add that?
> > >
> > > Alistair
> > >
> > > > +            return TRANSLATE_PMP_FAIL;
> > > > +        }
> > > >  #if defined(TARGET_RISCV32)
> > > >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> > > >  #elif defined(TARGET_RISCV64)
> > > > @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > > >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> > > >          (ret == TRANSLATE_SUCCESS) &&
> > > >          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> > > > +        ret = TRANSLATE_PMP_FAIL;
> > > > +    }
> > > > +    if (ret == TRANSLATE_PMP_FAIL) {
> > > >          pmp_violation = true;
> > > > -        ret = TRANSLATE_FAIL;
> > > >      }
> > > >      if (ret == TRANSLATE_SUCCESS) {
> > > >          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> > > > --
> > > > 2.17.1
> > > >
> > > >

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 3/5] RISC-V: Check PMP during Page Table Walks

[Hesham Almatary]

On Thu, 30 May 2019 at 05:07, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, May 22, 2019 at 2:27 AM Hesham Almatary
> <hesham.almatary@cl.cam.ac.uk> wrote:
> >
> > On Tue, 21 May 2019 at 23:40, Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
> > > <Hesham.Almatary@cl.cam.ac.uk> wrote:
> > > >
> > > > The PMP should be checked when doing a page table walk, and report access
> > > > fault exception if the to-be-read PTE failed the PMP check.
> > > >
> > > > Suggested-by: Jonathan Behrens <fintelia@gmail.com>
> > > > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> > > > ---
> > > >  target/riscv/cpu.h        |  1 +
> > > >  target/riscv/cpu_helper.c | 10 +++++++++-
> > > >  2 files changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > index c17184f4e4..ab3ba3f15a 100644
> > > > --- a/target/riscv/cpu.h
> > > > +++ b/target/riscv/cpu.h
> > > > @@ -94,6 +94,7 @@ enum {
> > > >  #define PRIV_VERSION_1_09_1 0x00010901
> > > >  #define PRIV_VERSION_1_10_0 0x00011000
> > > >
> > > > +#define TRANSLATE_PMP_FAIL 2
> > > >  #define TRANSLATE_FAIL 1
> > > >  #define TRANSLATE_SUCCESS 0
> > > >  #define NB_MMU_MODES 4
> > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > > index 7c7282c680..d0b0f9cf88 100644
> > > > --- a/target/riscv/cpu_helper.c
> > > > +++ b/target/riscv/cpu_helper.c
> > > > @@ -211,6 +211,12 @@ restart:
> > > >
> > > >          /* check that physical address of PTE is legal */
> > > >          target_ulong 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)) {
> > >
> > > I see a problem here.
> > >
> > > pmp_hart_has_privs() checks permissions based on the current value of
> > > env->priv. This might not always be the correct permissions to check,
> > > we should instead use the current mode to check permissions.
> > >
> > That is not clear to me. Isn't env->priv the current privildge mode?
> > Could you please elaborate on what other cases this might not be the case?
>
> Sorry for the delay. The RISC-V Hypervisor Extension allows load/store
> operations to be carried out as a previous privilege. The mstatus.MPRV
> and hstatus.SPRV allow this.
>
No problem, thanks for the clarification.
You are right, I haven't considered MPRV. I fixed that in a separate
commit in a v4 series of patches.
> Alistair
>
> >
> > > The best way to do this to me is to probably provide a privileged mode
> > > override to the function, can you add that?
> > >
> > > Alistair
> > >
> > > > +            return TRANSLATE_PMP_FAIL;
> > > > +        }
> > > >  #if defined(TARGET_RISCV32)
> > > >          target_ulong pte = ldl_phys(cs->as, pte_addr);
> > > >  #elif defined(TARGET_RISCV64)
> > > > @@ -405,8 +411,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > > >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> > > >          (ret == TRANSLATE_SUCCESS) &&
> > > >          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> > > > +        ret = TRANSLATE_PMP_FAIL;
> > > > +    }
> > > > +    if (ret == TRANSLATE_PMP_FAIL) {
> > > >          pmp_violation = true;
> > > > -        ret = TRANSLATE_FAIL;
> > > >      }
> > > >      if (ret == TRANSLATE_SUCCESS) {
> > > >          tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & TARGET_PAGE_MASK,
> > > > --
> > > > 2.17.1
> > > >
> > > >

[Qemu-devel] [PATCHv3 4/5] RISC-V: Fix a PMP bug where it succeeds even if PMP entry is off

[Hesham Almatary]

The current implementation returns 1 (PMP check success) if the address is in
range even if the PMP entry is off. This is a bug.

For example, if there is a PMP check in S-Mode which is in range, but its PMP
entry is off, this will succeed, which it should not.

The patch fixes this bug by only checking the PMP permissions if the address is
in range and its corresponding PMP entry it not off. Otherwise, it will keep
the ret = -1 which will be checked and handled correctly at the end of the
function.

Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/pmp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index b11c4ae22f..8668f0dd7c 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -259,11 +259,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
         /* fully inside */
         const uint8_t a_field =
             pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
-        if ((s + e) == 2) {
-            if (PMP_AMATCH_OFF == a_field) {
-                return 1;
-            }

+        /*
+         * If the PMP entry is not off and the address is in range, do the priv
+         * check
+         */
+        if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
             allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
             if ((env->priv != PRV_M) || pmp_is_locked(env, i)) {
                 allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
--
2.17.1

[Qemu-riscv] [PATCHv3 4/5] RISC-V: Fix a PMP bug where it succeeds even if PMP entry is off

[Hesham Almatary]

The current implementation returns 1 (PMP check success) if the address is in
range even if the PMP entry is off. This is a bug.

For example, if there is a PMP check in S-Mode which is in range, but its PMP
entry is off, this will succeed, which it should not.

The patch fixes this bug by only checking the PMP permissions if the address is
in range and its corresponding PMP entry it not off. Otherwise, it will keep
the ret = -1 which will be checked and handled correctly at the end of the
function.

Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/pmp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index b11c4ae22f..8668f0dd7c 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -259,11 +259,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
         /* fully inside */
         const uint8_t a_field =
             pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
-        if ((s + e) == 2) {
-            if (PMP_AMATCH_OFF == a_field) {
-                return 1;
-            }

+        /*
+         * If the PMP entry is not off and the address is in range, do the priv
+         * check
+         */
+        if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
             allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
             if ((env->priv != PRV_M) || pmp_is_locked(env, i)) {
                 allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
--
2.17.1

Re: [Qemu-devel] [PATCHv3 4/5] RISC-V: Fix a PMP bug where it succeeds even if PMP entry is off

[Alistair Francis]

On Tue, May 21, 2019 at 3:47 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The current implementation returns 1 (PMP check success) if the address is in
> range even if the PMP entry is off. This is a bug.
>
> For example, if there is a PMP check in S-Mode which is in range, but its PMP
> entry is off, this will succeed, which it should not.
>
> The patch fixes this bug by only checking the PMP permissions if the address is
> in range and its corresponding PMP entry it not off. Otherwise, it will keep
> the ret = -1 which will be checked and handled correctly at the end of the
> function.
>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>

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

Alistair

> ---
>  target/riscv/pmp.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index b11c4ae22f..8668f0dd7c 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -259,11 +259,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>          /* fully inside */
>          const uint8_t a_field =
>              pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
> -        if ((s + e) == 2) {
> -            if (PMP_AMATCH_OFF == a_field) {
> -                return 1;
> -            }
>
> +        /*
> +         * If the PMP entry is not off and the address is in range, do the priv
> +         * check
> +         */
> +        if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
>              allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
>              if ((env->priv != PRV_M) || pmp_is_locked(env, i)) {
>                  allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
> --
> 2.17.1
>
>

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 4/5] RISC-V: Fix a PMP bug where it succeeds even if PMP entry is off

[Alistair Francis]

On Tue, May 21, 2019 at 3:47 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The current implementation returns 1 (PMP check success) if the address is in
> range even if the PMP entry is off. This is a bug.
>
> For example, if there is a PMP check in S-Mode which is in range, but its PMP
> entry is off, this will succeed, which it should not.
>
> The patch fixes this bug by only checking the PMP permissions if the address is
> in range and its corresponding PMP entry it not off. Otherwise, it will keep
> the ret = -1 which will be checked and handled correctly at the end of the
> function.
>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>

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

Alistair

> ---
>  target/riscv/pmp.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index b11c4ae22f..8668f0dd7c 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -259,11 +259,12 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>          /* fully inside */
>          const uint8_t a_field =
>              pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg);
> -        if ((s + e) == 2) {
> -            if (PMP_AMATCH_OFF == a_field) {
> -                return 1;
> -            }
>
> +        /*
> +         * If the PMP entry is not off and the address is in range, do the priv
> +         * check
> +         */
> +        if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) {
>              allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC;
>              if ((env->priv != PRV_M) || pmp_is_locked(env, i)) {
>                  allowed_privs &= env->pmp_state.pmp[i].cfg_reg;
> --
> 2.17.1
>
>

[Qemu-devel] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

[Hesham Almatary]

The PMP check should be of the memory access size rather
than TARGET_PAGE_SIZE.

Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/cpu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index d0b0f9cf88..ce1f47e4e3 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,

     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
         (ret == TRANSLATE_SUCCESS) &&
-        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
+        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
         ret = TRANSLATE_PMP_FAIL;
     }
     if (ret == TRANSLATE_PMP_FAIL) {
--
2.17.1

[Qemu-riscv] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

[Hesham Almatary]

The PMP check should be of the memory access size rather
than TARGET_PAGE_SIZE.

Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
---
 target/riscv/cpu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index d0b0f9cf88..ce1f47e4e3 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,

     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
         (ret == TRANSLATE_SUCCESS) &&
-        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
+        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
         ret = TRANSLATE_PMP_FAIL;
     }
     if (ret == TRANSLATE_PMP_FAIL) {
--
2.17.1

Re: [Qemu-devel] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

[Alistair Francis]

On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The PMP check should be of the memory access size rather
> than TARGET_PAGE_SIZE.
>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>

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

Alistair

> ---
>  target/riscv/cpu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d0b0f9cf88..ce1f47e4e3 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>          (ret == TRANSLATE_SUCCESS) &&
> -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> +        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
>          ret = TRANSLATE_PMP_FAIL;
>      }
>      if (ret == TRANSLATE_PMP_FAIL) {
> --
> 2.17.1
>
>

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

[Alistair Francis]

On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The PMP check should be of the memory access size rather
> than TARGET_PAGE_SIZE.
>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>

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

Alistair

> ---
>  target/riscv/cpu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d0b0f9cf88..ce1f47e4e3 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>          (ret == TRANSLATE_SUCCESS) &&
> -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
> +        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
>          ret = TRANSLATE_PMP_FAIL;
>      }
>      if (ret == TRANSLATE_PMP_FAIL) {
> --
> 2.17.1
>
>

Re: [Qemu-devel] [Qemu-riscv] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

[Jonathan Behrens]

Hesham,

I don't think this is quite right. If I understand correctly, PMP
permissions are only validated on TLB fills, not on all accesses. (Is
anyone able to confirm this?) If so, this function can't just validate the
range of a single access and then place the entire page into the TLB.
However, the current code is also wrong because an access should
succeed/fail based on the permissions only for the range it actually
touches even regardless of the permissions on the rest of the page. Now
that I think about it, I'd also expect that somewhere in the PMP logic
would flush the TLB every time any of the related control registers change
though I can't find anywhere that this is happening...

Sorry to keep raising complaints about this patch set, the interaction
between physical memory protection and paging is very subtle. Even some
real hardware has had errata related to it!

Jonathan

On Tue, May 21, 2019 at 6:33 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
> <Hesham.Almatary@cl.cam.ac.uk> wrote:
> >
> > The PMP check should be of the memory access size rather
> > than TARGET_PAGE_SIZE.
> >
> > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
> > ---
> >  target/riscv/cpu_helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index d0b0f9cf88..ce1f47e4e3 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
> >
> >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> >          (ret == TRANSLATE_SUCCESS) &&
> > -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 <<
> access_type)) {
> > +        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
> >          ret = TRANSLATE_PMP_FAIL;
> >      }
> >      if (ret == TRANSLATE_PMP_FAIL) {
> > --
> > 2.17.1
> >
> >
>
>

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

[Jonathan Behrens]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

Hesham,

I don't think this is quite right. If I understand correctly, PMP
permissions are only validated on TLB fills, not on all accesses. (Is
anyone able to confirm this?) If so, this function can't just validate the
range of a single access and then place the entire page into the TLB.
However, the current code is also wrong because an access should
succeed/fail based on the permissions only for the range it actually
touches even regardless of the permissions on the rest of the page. Now
that I think about it, I'd also expect that somewhere in the PMP logic
would flush the TLB every time any of the related control registers change
though I can't find anywhere that this is happening...

Sorry to keep raising complaints about this patch set, the interaction
between physical memory protection and paging is very subtle. Even some
real hardware has had errata related to it!

Jonathan

On Tue, May 21, 2019 at 6:33 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
> <Hesham.Almatary@cl.cam.ac.uk> wrote:
> >
> > The PMP check should be of the memory access size rather
> > than TARGET_PAGE_SIZE.
> >
> > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
> > ---
> >  target/riscv/cpu_helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index d0b0f9cf88..ce1f47e4e3 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
> >
> >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> >          (ret == TRANSLATE_SUCCESS) &&
> > -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 <<
> access_type)) {
> > +        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
> >          ret = TRANSLATE_PMP_FAIL;
> >      }
> >      if (ret == TRANSLATE_PMP_FAIL) {
> > --
> > 2.17.1
> >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 2908 bytes --]

Re: [Qemu-devel] [Qemu-riscv] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

[Hesham Almatary]

On Wed, 22 May 2019 at 03:25, Jonathan Behrens <fintelia@gmail.com> wrote:
>
> Hesham,
>
> I don't think this is quite right. If I understand correctly, PMP permissions are only validated on TLB fills, not on all accesses. (Is anyone able to confirm this?) If so, this function can't just validate the range of a single access and then place the entire page into the TLB. However, the current code is also wrong because an access should succeed/fail based on the permissions only for the range it actually touches even regardless of the permissions on the rest of the page. Now that I think about it, I'd also expect that somewhere in the PMP logic would flush the TLB every time any of the related control registers change though I can't find anywhere that this is happening...
>
I believe the TLB fill function is called on all accesses, but I might
be wrong. I will wait for someone to confirm otherwise.

It's mentioned in the spec that sfence.vma has to be executed after
changing PMP configs, so it's a SW concern (i.e., not QEMU's).

> Sorry to keep raising complaints about this patch set, the interaction between physical memory protection and paging is very subtle. Even some real hardware has had errata related to it!
>
> Jonathan
>
> On Tue, May 21, 2019 at 6:33 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
>> <Hesham.Almatary@cl.cam.ac.uk> wrote:
>> >
>> > The PMP check should be of the memory access size rather
>> > than TARGET_PAGE_SIZE.
>> >
>> > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>
>> Alistair
>>
>> > ---
>> >  target/riscv/cpu_helper.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> > index d0b0f9cf88..ce1f47e4e3 100644
>> > --- a/target/riscv/cpu_helper.c
>> > +++ b/target/riscv/cpu_helper.c
>> > @@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> >
>> >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>> >          (ret == TRANSLATE_SUCCESS) &&
>> > -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
>> > +        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
>> >          ret = TRANSLATE_PMP_FAIL;
>> >      }
>> >      if (ret == TRANSLATE_PMP_FAIL) {
>> > --
>> > 2.17.1
>> >
>> >
>>

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

[Hesham Almatary]

On Wed, 22 May 2019 at 03:25, Jonathan Behrens <fintelia@gmail.com> wrote:
>
> Hesham,
>
> I don't think this is quite right. If I understand correctly, PMP permissions are only validated on TLB fills, not on all accesses. (Is anyone able to confirm this?) If so, this function can't just validate the range of a single access and then place the entire page into the TLB. However, the current code is also wrong because an access should succeed/fail based on the permissions only for the range it actually touches even regardless of the permissions on the rest of the page. Now that I think about it, I'd also expect that somewhere in the PMP logic would flush the TLB every time any of the related control registers change though I can't find anywhere that this is happening...
>
I believe the TLB fill function is called on all accesses, but I might
be wrong. I will wait for someone to confirm otherwise.

It's mentioned in the spec that sfence.vma has to be executed after
changing PMP configs, so it's a SW concern (i.e., not QEMU's).

> Sorry to keep raising complaints about this patch set, the interaction between physical memory protection and paging is very subtle. Even some real hardware has had errata related to it!
>
> Jonathan
>
> On Tue, May 21, 2019 at 6:33 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
>> <Hesham.Almatary@cl.cam.ac.uk> wrote:
>> >
>> > The PMP check should be of the memory access size rather
>> > than TARGET_PAGE_SIZE.
>> >
>> > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>
>> Alistair
>>
>> > ---
>> >  target/riscv/cpu_helper.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> > index d0b0f9cf88..ce1f47e4e3 100644
>> > --- a/target/riscv/cpu_helper.c
>> > +++ b/target/riscv/cpu_helper.c
>> > @@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> >
>> >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>> >          (ret == TRANSLATE_SUCCESS) &&
>> > -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
>> > +        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
>> >          ret = TRANSLATE_PMP_FAIL;
>> >      }
>> >      if (ret == TRANSLATE_PMP_FAIL) {
>> > --
>> > 2.17.1
>> >
>> >
>>

Re: [Qemu-devel] [PATCHv3 1/5] RISC-V: Only Check PMP if MMU translation succeeds

[Alistair Francis]

On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The current implementation unnecessarily checks for PMP even if MMU translation
> failed. This may trigger a wrong PMP access exception instead of
> a page exception.
>
> For example, the very first instruction fetched after the first satp write in
> S-Mode will trigger a PMP access fault instead of an instruction fetch page
> fault.
>
> This patch prioritises MMU exceptions over PMP exceptions and only checks for
> PMP if MMU translation succeeds.

It's probably worth noting in the commit message that this commit is
only required for the future commits. Otherwise it is a little
confusing.

Alistair

>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> ---
>  target/riscv/cpu_helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 41d6db41c3..40fb47e794 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -401,6 +401,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                    " prot %d\n", __func__, address, ret, pa, prot);
>
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> +        (ret == TRANSLATE_SUCCESS) &&
>          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
>          ret = TRANSLATE_FAIL;
>      }
> --
> 2.17.1
>
>

Re: [Qemu-riscv] [Qemu-devel] [PATCHv3 1/5] RISC-V: Only Check PMP if MMU translation succeeds

[Alistair Francis]

On Tue, May 21, 2019 at 3:44 AM Hesham Almatary
<Hesham.Almatary@cl.cam.ac.uk> wrote:
>
> The current implementation unnecessarily checks for PMP even if MMU translation
> failed. This may trigger a wrong PMP access exception instead of
> a page exception.
>
> For example, the very first instruction fetched after the first satp write in
> S-Mode will trigger a PMP access fault instead of an instruction fetch page
> fault.
>
> This patch prioritises MMU exceptions over PMP exceptions and only checks for
> PMP if MMU translation succeeds.

It's probably worth noting in the commit message that this commit is
only required for the future commits. Otherwise it is a little
confusing.

Alistair

>
> Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
> ---
>  target/riscv/cpu_helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 41d6db41c3..40fb47e794 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -401,6 +401,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                    " prot %d\n", __func__, address, ret, pa, prot);
>
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> +        (ret == TRANSLATE_SUCCESS) &&
>          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
>          ret = TRANSLATE_FAIL;
>      }
> --
> 2.17.1
>
>