| Message ID | 20240409175241.1297072-1-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-9.0] target/riscv: prioritize pmp errors in raise_mmu_exception() | expand |
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...*
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
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?
On Tue, 9 Apr 2024 at 18:53, Daniel Henrique Barboza <dbarboza@ventanamicro.com> 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> I guess from the Fixes: git commit hash that this isn't a regression since 8.2 ? That would make it too late for 9.0 at this point in the release cycle. thanks -- PMM
On 4/12/24 14:12, Peter Maydell wrote: > On Tue, 9 Apr 2024 at 18:53, Daniel Henrique Barboza > <dbarboza@ventanamicro.com> 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> > > I guess from the Fixes: git commit hash that this isn't a regression > since 8.2 ? That would make it too late for 9.0 at this point in > the release cycle. I don't think it's critical enough to push it for 9.0 now. We'll settle for 9.1 and then Michael can pick it for 9.0-stable. Thanks, Daniel > > thanks > -- PMM
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 >
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:
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(-)