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

[Aleksei Filippov]

On 09.04.2024 20:52, Daniel Henrique Barboza wrote:
> raise_mmu_exception(), as is today, is prioritizing guest page faults by
> checking first if virt_enabled && !first_stage, and then considering the
> regular inst/load/store faults.
>
> There's no mention in the spec about guest page fault being a higher
> priority that PMP faults. In fact, privileged spec section 3.7.1 says:
>
> "Attempting to fetch an instruction from a PMP region that does not have
> execute permissions raises an instruction access-fault exception.
> Attempting to execute a load or load-reserved instruction which accesses
> a physical address within a PMP region without read permissions raises a
> load access-fault exception. Attempting to execute a store,
> store-conditional, or AMO instruction which accesses a physical address
> within a PMP region without write permissions raises a store
> access-fault exception."
>
> So, in fact, we're doing it wrong - PMP faults should always be thrown,
> regardless of also being a first or second stage fault.
>
> The way riscv_cpu_tlb_fill() and get_physical_address() work is
> adequate: a TRANSLATE_PMP_FAIL error is immediately reported and
> reflected in the 'pmp_violation' flag. What we need is to change
> raise_mmu_exception() to prioritize it.
>
> Reported-by: Joseph Chan <jchan@ventanamicro.com>
> Fixes: 82d53adfbb ("target/riscv/cpu_helper.c: Invalid exception on MMU translation 
> stage")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu_helper.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index fc090d729a..e3a7797d00 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1176,28 +1176,30 @@ static void raise_mmu_exception(CPURISCVState *env, 
> target_ulong address,
>
>       switch (access_type) {
>       case MMU_INST_FETCH:
> -        if (env->virt_enabled && !first_stage) {
> +        if (pmp_violation) {
> +            cs->exception_index = RISCV_EXCP_INST_ACCESS_FAULT;
> +        } else if (env->virt_enabled && !first_stage) {
>               cs->exception_index = RISCV_EXCP_INST_GUEST_PAGE_FAULT;
>           } else {
> -            cs->exception_index = pmp_violation ?
> -                RISCV_EXCP_INST_ACCESS_FAULT : RISCV_EXCP_INST_PAGE_FAULT;
> +            cs->exception_index = RISCV_EXCP_INST_PAGE_FAULT;
>           }
>           break;
>       case MMU_DATA_LOAD:
> -        if (two_stage && !first_stage) {
> +        if (pmp_violation) {
> +            cs->exception_index = RISCV_EXCP_LOAD_ACCESS_FAULT;
> +        } else if (two_stage && !first_stage) {
>               cs->exception_index = RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT;
>           } else {
> -            cs->exception_index = pmp_violation ?
> -                RISCV_EXCP_LOAD_ACCESS_FAULT : RISCV_EXCP_LOAD_PAGE_FAULT;
> +            cs->exception_index = RISCV_EXCP_LOAD_PAGE_FAULT;
>           }
>           break;
>       case MMU_DATA_STORE:
> -        if (two_stage && !first_stage) {
> +        if (pmp_violation) {
> +            cs->exception_index = RISCV_EXCP_STORE_AMO_ACCESS_FAULT;
> +        } else if (two_stage && !first_stage) {
>               cs->exception_index = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT;
>           } else {
> -            cs->exception_index = pmp_violation ?
> -                RISCV_EXCP_STORE_AMO_ACCESS_FAULT :
> -                RISCV_EXCP_STORE_PAGE_FAULT;
> +            cs->exception_index = RISCV_EXCP_STORE_PAGE_FAULT;
>           }
>           break;
>       default:


Just tested your patch and found out that we still need to fix `if else` in
riscv_cpu_tlb_fill() after pmp check in 2 stage translation part, as I suggested
before, cz the problem with mtval2 will happened in case of successes 2 stage
translation but failed pmp check. In this case we gonna set
mtval2(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...*
--
Sincerely,
Aleksei Filippov