| Message ID | 20240413105929.7030-1-alexei.filippov@syntacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() | expand |
FYI Priv-v1.12/riscv-privileged-20211203.pdf <https://github.com/riscv/riscv-isa-manual/releases/download/Priv-v1.12/riscv-privileged-20211203.pdf> defines exception priorities on Page 40, Table 3.7 Page 130, Table 8.7 There is a sentence under Table 3.7: "When a virtual address is translated into a physical address, the address translation algorithm determines what specific exception may be raised." The spec does not insist any implementation to report Exception Code 12 over 1; 13,15 over 5, 7. On the other hand, the phrases "During instruction address translation:" and "With physical address for instruction:" gives me the impression that when the implementation can distinguish between these situations, then reporting 12 , 13, 15 instead of 1, 5, 7 will provide a fine-grained reason for why things were broken. Regards, Joseph Chan On Sat, Apr 13, 2024 at 3:59 AM Alexei Filippov < alexei.filippov@syntacore.com> wrote: > From: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > 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 bc70ab5abc..196166f8dd 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -1203,28 +1203,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: > -- > 2.34.1 > >
Digging more in Priv-v1.12/riscv-privileged-20211203.pdf <https://github.com/riscv/riscv-isa-manual/releases/download/Priv-v1.12/riscv-privileged-20211203.pdf> : Page 82, Section 4.3.2 Virtual Address Translation Process. The spec actually mentions an address translation algorithm. Step 2 mentions the exception code should be " access-fault exception corresponding to the original access type". i.e. 1, 5, 7. All other steps should use " page-fault exception corresponding to the original access type". i.e. 12, 13, 15. Regards, Joseph Chan On Mon, Apr 15, 2024 at 11:50 AM Joseph Chan <jchan@ventanamicro.com> wrote: > FYI > > Priv-v1.12/riscv-privileged-20211203.pdf > <https://github.com/riscv/riscv-isa-manual/releases/download/Priv-v1.12/riscv-privileged-20211203.pdf> > defines exception priorities on > Page 40, Table 3.7 > Page 130, Table 8.7 > > There is a sentence under Table 3.7: > "When a virtual address is translated into a physical address, the address > translation algorithm > determines what specific exception may be raised." > > > The spec does not insist any implementation to report Exception Code 12 > over 1; 13,15 over 5, 7. On the other hand, the phrases "During instruction > address translation:" and "With physical address for instruction:" gives me > the impression that when the implementation can distinguish between these > situations, then reporting 12 , 13, 15 instead of 1, 5, 7 will provide a > fine-grained reason for why things were broken. > > Regards, > Joseph Chan > > > On Sat, Apr 13, 2024 at 3:59 AM Alexei Filippov < > alexei.filippov@syntacore.com> wrote: > >> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> >> 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 bc70ab5abc..196166f8dd 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -1203,28 +1203,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: >> -- >> 2.34.1 >> >>
On Sat, Apr 13, 2024 at 9:00 PM Alexei Filippov <alexei.filippov@syntacore.com> wrote: > > From: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > 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> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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 bc70ab5abc..196166f8dd 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -1203,28 +1203,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: > -- > 2.34.1 > >
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index bc70ab5abc..196166f8dd 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -1203,28 +1203,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: