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

[Daniel Henrique Barboza]

On 4/12/24 11:15, Aleksei Filippov wrote:
> 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...*

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