Hi Jonathan,
Thanks for your feedback.
On Sat, 18 May 2019 at 22:51, Jonathan Behrens <finte...@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.almat...@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.almat...@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
>>
>>
