Re: [PATCH for-9.0] target/riscv: prioritize pmp errors in raise_mmu_exception()

[Daniel Henrique Barboza]

On 4/12/24 13:52, Aleksei Filippov wrote:

On 12.04.2024 19:00, Daniel Henrique Barboza wrote:

Thanks for giving it a go. You're right, this patch alone is not enough and 
we'll
need your patch too.

But note that, with what you've said in mind, your patch will also end up 
setting
mtval2 and env->guest_phys_fault_addr in case a PMP fault occurs during the
get_physical_address() right at the start of second stage:

         if (ret == TRANSLATE_SUCCESS) {
             /* Second stage lookup */
             im_address = pa;

             ret = get_physical_address(env, &pa, &prot2, im_address, NULL,
                                        access_type, MMUIdx_U, false, true,
                                        false);


I think your patch needs to also prevent env->guest_phys_fault_addr to be set 
when
ret == TRANSLATE_PMP_FAIL.

With these changes in your patch, and this patch, we're free to set 
"first_stage_error = false;"
at the start of second stage lookup, keeping consistency, because 
raise_mmu_exception is now
able to deal with it. I can amend this change in this patch. This patch would 
prioritize
PMP errors, your patch will fix the problem with mtval2.

Let me know what do you think. If you agree I can re-send both patches together.


Thanks,


Daniel

Oh, I actually missed that, thx, you are right. I could prepare patch to fix
this, do you want it in this thread or in previous with only my patch in?

If you don't mind, please re-send it together in a new thread with this patch 
too.

Include this one as is, then include yours as a complement that fixes the 
problem
with mtval2 that my patch missed. You can use the explanation you gave me as the
new commit msg for your patch. E.g:

"target/riscv: do not set mtval2 for non guest-page faults

Previous patch fixed the PMP priority in raise_mmu_exception() but we're still
setting mtval2 incorrectly. In riscv_cpu_tlb_fill(), after pmp check in 2 stage
translation part, mtval2 will be set in case of successes 2 stage translation 
but
failed pmp check.

In this case we gonna set mtval2 via env->guest_phys_fault_addr in context of
riscv_cpu_tlb_fill(), as this was a guest-page-fault, but it didn't and mtval2
should be zero, according to RISCV privileged spec sect. 9.4.4: When a guest
page-fault is taken into M-mode, mtval2 is written with either zero or guest
physical address that faulted, shifted by 2 bits. *For other traps, mtval2
is set to zero...* "


Thanks,

Daniel