* [PATCH 2/2] target/riscv: do not set mtval2 for non guest-page faults
2024-04-13 10:59 [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() Alexei Filippov
@ 2024-04-13 10:59 ` Alexei Filippov
2024-04-29 9:43 ` Daniel Henrique Barboza
2024-04-15 18:50 ` [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() Joseph Chan
2024-05-14 5:48 ` Alistair Francis
2 siblings, 1 reply; 10+ messages in thread
From: Alexei Filippov @ 2024-04-13 10:59 UTC (permalink / raw)
To: palmer, alistair.francis, bin.meng, liwei1518, dbarboza,
zhiwei_liu
Cc: qemu-riscv, qemu-devel, Alexei Filippov
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...*
Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
---
target/riscv/cpu_helper.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 196166f8dd..89e659fe3a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1410,17 +1410,17 @@ 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
*/
first_stage_error = false;
- env->guest_phys_fault_addr = (im_address |
- (address &
- (TARGET_PAGE_SIZE - 1))) >> 2;
+ if (ret != TRANSLATE_PMP_FAIL) {
+ env->guest_phys_fault_addr = (im_address |
+ (address &
+ (TARGET_PAGE_SIZE - 1))) >> 2;
+ }
}
}
} else {
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] target/riscv: do not set mtval2 for non guest-page faults
2024-04-13 10:59 ` [PATCH 2/2] target/riscv: do not set mtval2 for non guest-page faults Alexei Filippov
@ 2024-04-29 9:43 ` Daniel Henrique Barboza
2024-05-03 10:30 ` [PATCH v2 " Alexei Filippov
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Henrique Barboza @ 2024-04-29 9:43 UTC (permalink / raw)
To: Alexei Filippov, palmer, alistair.francis, bin.meng, liwei1518,
zhiwei_liu
Cc: qemu-riscv, qemu-devel
On 4/13/24 07:59, Alexei Filippov wrote:
> 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...*
>
> Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/cpu_helper.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 196166f8dd..89e659fe3a 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1410,17 +1410,17 @@ 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
> */
> first_stage_error = false;
> - env->guest_phys_fault_addr = (im_address |
> - (address &
> - (TARGET_PAGE_SIZE - 1))) >> 2;
> + if (ret != TRANSLATE_PMP_FAIL) {
> + env->guest_phys_fault_addr = (im_address |
> + (address &
> + (TARGET_PAGE_SIZE - 1))) >> 2;
> + }
> }
> }
> } else {
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 2/2] target/riscv: do not set mtval2 for non guest-page faults
2024-04-29 9:43 ` Daniel Henrique Barboza
@ 2024-05-03 10:30 ` Alexei Filippov
2024-05-14 5:59 ` Alistair Francis
2024-05-14 6:16 ` Alistair Francis
0 siblings, 2 replies; 10+ messages in thread
From: Alexei Filippov @ 2024-05-03 10:30 UTC (permalink / raw)
To: dbarboza
Cc: alexei.filippov, alistair.francis, bin.meng, liwei1518, palmer,
qemu-devel, qemu-riscv, zhiwei_liu
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...*
Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
Changes since v1:
-Added Reviewed-by tag.
target/riscv/cpu_helper.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e3a7797d00..484edad900 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1375,17 +1375,17 @@ 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
*/
first_stage_error = false;
- env->guest_phys_fault_addr = (im_address |
- (address &
- (TARGET_PAGE_SIZE - 1))) >> 2;
+ if (ret != TRANSLATE_PMP_FAIL) {
+ env->guest_phys_fault_addr = (im_address |
+ (address &
+ (TARGET_PAGE_SIZE - 1))) >> 2;
+ }
}
}
} else {
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/2] target/riscv: do not set mtval2 for non guest-page faults
2024-05-03 10:30 ` [PATCH v2 " Alexei Filippov
@ 2024-05-14 5:59 ` Alistair Francis
2024-05-14 6:16 ` Alistair Francis
1 sibling, 0 replies; 10+ messages in thread
From: Alistair Francis @ 2024-05-14 5:59 UTC (permalink / raw)
To: Alexei Filippov
Cc: dbarboza, alistair.francis, bin.meng, liwei1518, palmer,
qemu-devel, qemu-riscv, zhiwei_liu
On Fri, May 3, 2024 at 8:32 PM Alexei Filippov
<alexei.filippov@syntacore.com> wrote:
>
> 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...*
>
> Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> Changes since v1:
> -Added Reviewed-by tag.
> target/riscv/cpu_helper.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e3a7797d00..484edad900 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1375,17 +1375,17 @@ 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
> */
> first_stage_error = false;
> - env->guest_phys_fault_addr = (im_address |
> - (address &
> - (TARGET_PAGE_SIZE - 1))) >> 2;
> + if (ret != TRANSLATE_PMP_FAIL) {
> + env->guest_phys_fault_addr = (im_address |
> + (address &
> + (TARGET_PAGE_SIZE - 1))) >> 2;
> + }
> }
> }
> } else {
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/2] target/riscv: do not set mtval2 for non guest-page faults
2024-05-03 10:30 ` [PATCH v2 " Alexei Filippov
2024-05-14 5:59 ` Alistair Francis
@ 2024-05-14 6:16 ` Alistair Francis
1 sibling, 0 replies; 10+ messages in thread
From: Alistair Francis @ 2024-05-14 6:16 UTC (permalink / raw)
To: Alexei Filippov
Cc: dbarboza, alistair.francis, bin.meng, liwei1518, palmer,
qemu-devel, qemu-riscv, zhiwei_liu
On Fri, May 3, 2024 at 8:32 PM Alexei Filippov
<alexei.filippov@syntacore.com> wrote:
>
> 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...*
>
> Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> Changes since v1:
> -Added Reviewed-by tag.
> target/riscv/cpu_helper.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e3a7797d00..484edad900 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1375,17 +1375,17 @@ 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
> */
> first_stage_error = false;
> - env->guest_phys_fault_addr = (im_address |
> - (address &
> - (TARGET_PAGE_SIZE - 1))) >> 2;
> + if (ret != TRANSLATE_PMP_FAIL) {
> + env->guest_phys_fault_addr = (im_address |
> + (address &
> + (TARGET_PAGE_SIZE - 1))) >> 2;
> + }
> }
> }
> } else {
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception()
2024-04-13 10:59 [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() Alexei Filippov
2024-04-13 10:59 ` [PATCH 2/2] target/riscv: do not set mtval2 for non guest-page faults Alexei Filippov
@ 2024-04-15 18:50 ` Joseph Chan
2024-04-16 3:14 ` Joseph Chan
2024-05-14 5:48 ` Alistair Francis
2 siblings, 1 reply; 10+ messages in thread
From: Joseph Chan @ 2024-04-15 18:50 UTC (permalink / raw)
To: Alexei Filippov
Cc: palmer, alistair.francis, bin.meng, liwei1518, dbarboza,
zhiwei_liu, qemu-riscv, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4700 bytes --]
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
>
>
[-- Attachment #2: Type: text/html, Size: 5812 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception()
2024-04-15 18:50 ` [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() Joseph Chan
@ 2024-04-16 3:14 ` Joseph Chan
0 siblings, 0 replies; 10+ messages in thread
From: Joseph Chan @ 2024-04-16 3:14 UTC (permalink / raw)
To: Alexei Filippov
Cc: palmer, alistair.francis, bin.meng, liwei1518, dbarboza,
zhiwei_liu, qemu-riscv, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5490 bytes --]
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
>>
>>
[-- Attachment #2: Type: text/html, Size: 6881 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception()
2024-04-13 10:59 [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() Alexei Filippov
2024-04-13 10:59 ` [PATCH 2/2] target/riscv: do not set mtval2 for non guest-page faults Alexei Filippov
2024-04-15 18:50 ` [PATCH 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception() Joseph Chan
@ 2024-05-14 5:48 ` Alistair Francis
2024-05-27 12:11 ` [PATCH v2 " Alexei Filippov
2 siblings, 1 reply; 10+ messages in thread
From: Alistair Francis @ 2024-05-14 5:48 UTC (permalink / raw)
To: Alexei Filippov
Cc: palmer, alistair.francis, bin.meng, liwei1518, dbarboza,
zhiwei_liu, qemu-riscv, qemu-devel, Joseph Chan
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 1/2] target/riscv: prioritize pmp errors in raise_mmu_exception()
2024-05-14 5:48 ` Alistair Francis
@ 2024-05-27 12:11 ` Alexei Filippov
0 siblings, 0 replies; 10+ messages in thread
From: Alexei Filippov @ 2024-05-27 12:11 UTC (permalink / raw)
To: alistair23
Cc: alexei.filippov, alistair.francis, bin.meng, dbarboza, jchan,
liwei1518, palmer, qemu-devel, qemu-riscv, zhiwei_liu
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")
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
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:
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread