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

[Qemu-devel] [PATCH 1/2] 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.

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 41d6db41c3..b48de36114 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",
@@ -402,6 +404,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,

     if (riscv_feature(env, RISCV_FEATURE_PMP) &&
         !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
+        pmp_violation = true;
         ret = TRANSLATE_FAIL;
     }
     if (ret == TRANSLATE_SUCCESS) {
@@ -411,7 +414,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] [PATCH 2/2] 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 b48de36114..7c7282c680 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -403,6 +403,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)) {
         pmp_violation = true;
         ret = TRANSLATE_FAIL;
--
2.17.1

Re: [Qemu-devel] [Qemu-riscv] [PATCH 1/2] RISC-V: Raise access fault exceptions on PMP violations

[Jonathan Behrens]

This patch assumes that translation failure should always raise a paging
fault, but it should be possible for it to raise an access fault as well
(since according to the spec "PMP  checks  are  also  applied  to
page-table  accesses  for  virtual-address translation, for which the
effective privilege mode is S."). I think the code to actually do the PMP
checking during page table walks is currently unimplemented though...

Jonathan

On Sat, May 18, 2019 at 3:14 PM Hesham Almatary <
Hesham.Almatary@cl.cam.ac.uk> wrote:

> 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.
>
> 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 41d6db41c3..b48de36114 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",
> @@ -402,6 +404,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
>
>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type))
> {
> +        pmp_violation = true;
>          ret = TRANSLATE_FAIL;
>      }
>      if (ret == TRANSLATE_SUCCESS) {
> @@ -411,7 +414,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
>
>
>

Re: [Qemu-devel] [Qemu-riscv] [PATCH 1/2] RISC-V: Raise access fault exceptions on PMP violations

[Hesham Almatary]

Hi Jonathan,

Thanks for your feedback.

On Sat, 18 May 2019 at 22:51, Jonathan Behrens <fintelia@gmail.com> wrote:
>
> This patch assumes that translation failure should always raise a paging fault, but it should be possible for it to raise an access fault as well (since according to the spec "PMP  checks  are  also  applied  to  page-table  accesses  for  virtual-address translation, for which the effective privilege mode is S."). I think the code to actually do the PMP checking during page table walks is currently unimplemented though...
>

The patch actually fixes (rather than assumes) one issue of the
current implementation which always raises a paging fault "when
translation succeeds and PMP fails". The second issue that you report
here which happens "when the PTW fails the PMP check" could be another
future separate fix.

I am happy to submit another patch to fix the second issue.

> Jonathan
>
> On Sat, May 18, 2019 at 3:14 PM Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk> wrote:
>>
>> 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.
>>
>> 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 41d6db41c3..b48de36114 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",
>> @@ -402,6 +404,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>
>>      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>>          !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
>> +        pmp_violation = true;
>>          ret = TRANSLATE_FAIL;
>>      }
>>      if (ret == TRANSLATE_SUCCESS) {
>> @@ -411,7 +414,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
>>
>>