Successed two stage translation, but failed pmp check can cause guest
page fault instead of regular page fault.

In case of execution ld instuction in VS mode we can
face situation when two stages of translation was passed successfully,
and if PMP check was failed first_stage_error variable going to be
setted to false and raise_mmu_exception will raise
RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT(scause == 21) instead of
RISCV_EXCP_LOAD_ACCESS_FAULT(scause == 5) and this is wrong, according
to RISCV privileged spec sect. 3.7: 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.

Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
---
 target/riscv/cpu_helper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index bc70ab5abc..eaef1c2c62 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1408,9 +1408,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                               __func__, pa, ret, prot_pmp, tlb_size);
 
                 prot &= prot_pmp;
-            }
-
-            if (ret != TRANSLATE_SUCCESS) {
+            } else {
                 /*
                  * Guest physical address translation failed, this is a HS
                  * level exception
-- 
2.34.1

On 3/29/24 10:45, Alexei Filippov wrote:
> Successed two stage translation, but failed pmp check can cause guest
> page fault instead of regular page fault.
> 
> In case of execution ld instuction in VS mode we can
> face situation when two stages of translation was passed successfully,
> and if PMP check was failed first_stage_error variable going to be
> setted to false and raise_mmu_exception will raise
> RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT(scause == 21) instead of
> RISCV_EXCP_LOAD_ACCESS_FAULT(scause == 5) and this is wrong, according
> to RISCV privileged spec sect. 3.7: 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.
> 
> Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
> ---
>   target/riscv/cpu_helper.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index bc70ab5abc..eaef1c2c62 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1408,9 +1408,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                                 __func__, pa, ret, prot_pmp, tlb_size);
>   
>                   prot &= prot_pmp;
> -            }
> -
> -            if (ret != TRANSLATE_SUCCESS) {
> +            } else {


This solves the issue but we're leaving the 'first_stage_error' flag in an inconsistent
state - the call for get_physical_address_pmp() inside "if (ret == TRANSLATE_SUCCESS)" is
both a PMP error and also a non-first stage error, but now we're leaving it to 'true'.
Raise raise_mmu_exception() will give the expected PMP fault for the wrong reasons, since
it thinks that it's a first-stage fault when it's not. This will work now, but any
future change to raise_mmu_exception can break this logic.

I have an internal (not yet sent to review) fix for the same problem. In my case I was
supposed to have an INST_ACCESS_FAULT (1) and was getting an INST_GUEST_PAGE_FAULT (20).
I'll see if I can send it ASAP so you can have a look and see if it's also good for your
problem.



Thanks,

Daniel






>                   /*
>                    * Guest physical address translation failed, this is a HS
>                    * level exception