Re: [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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
>> ---
>>  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
>>
>>