* [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup
2020-10-08 16:21 [PATCH 0/3] target/arm: MTE fixes Richard Henderson
@ 2020-10-08 16:21 ` Richard Henderson
2020-10-08 16:32 ` Philippe Mathieu-Daudé
2020-10-09 11:12 ` Vincenzo Frascino
2020-10-08 16:21 ` [PATCH 2/3] target/arm: Fix reported EL for mte_check_fail Richard Henderson
` (3 subsequent siblings)
4 siblings, 2 replies; 10+ messages in thread
From: Richard Henderson @ 2020-10-08 16:21 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm, vincenzo.frascino
We already have the full ARMMMUIdx as computed from the
function parameter.
For the purpose of regime_has_2_ranges, we can ignore any
difference between AccType_Normal and AccType_Unpriv, which
would be the only difference between the passed mmu_idx
and arm_mmu_idx_el.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/mte_helper.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
index 5615c6706c..734cc5ca67 100644
--- a/target/arm/mte_helper.c
+++ b/target/arm/mte_helper.c
@@ -563,8 +563,7 @@ static void mte_check_fail(CPUARMState *env, uint32_t desc,
case 2:
/* Tag check fail causes asynchronous flag set. */
- mmu_idx = arm_mmu_idx_el(env, el);
- if (regime_has_2_ranges(mmu_idx)) {
+ if (regime_has_2_ranges(arm_mmu_idx)) {
select = extract64(dirty_ptr, 55, 1);
} else {
select = 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup
2020-10-08 16:21 ` [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup Richard Henderson
@ 2020-10-08 16:32 ` Philippe Mathieu-Daudé
2020-10-09 11:12 ` Vincenzo Frascino
1 sibling, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-08 16:32 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: peter.maydell, qemu-arm, vincenzo.frascino
On 10/8/20 6:21 PM, Richard Henderson wrote:
> We already have the full ARMMMUIdx as computed from the
> function parameter.
>
> For the purpose of regime_has_2_ranges, we can ignore any
> difference between AccType_Normal and AccType_Unpriv, which
> would be the only difference between the passed mmu_idx
> and arm_mmu_idx_el.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/mte_helper.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> index 5615c6706c..734cc5ca67 100644
> --- a/target/arm/mte_helper.c
> +++ b/target/arm/mte_helper.c
> @@ -563,8 +563,7 @@ static void mte_check_fail(CPUARMState *env, uint32_t desc,
>
> case 2:
> /* Tag check fail causes asynchronous flag set. */
> - mmu_idx = arm_mmu_idx_el(env, el);
> - if (regime_has_2_ranges(mmu_idx)) {
> + if (regime_has_2_ranges(arm_mmu_idx)) {
> select = extract64(dirty_ptr, 55, 1);
> } else {
> select = 0;
>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup
2020-10-08 16:21 ` [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup Richard Henderson
2020-10-08 16:32 ` Philippe Mathieu-Daudé
@ 2020-10-09 11:12 ` Vincenzo Frascino
1 sibling, 0 replies; 10+ messages in thread
From: Vincenzo Frascino @ 2020-10-09 11:12 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: peter.maydell, qemu-arm
On 10/8/20 5:21 PM, Richard Henderson wrote:
> We already have the full ARMMMUIdx as computed from the
> function parameter.
>
> For the purpose of regime_has_2_ranges, we can ignore any
> difference between AccType_Normal and AccType_Unpriv, which
> would be the only difference between the passed mmu_idx
> and arm_mmu_idx_el.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Tested-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
> target/arm/mte_helper.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> index 5615c6706c..734cc5ca67 100644
> --- a/target/arm/mte_helper.c
> +++ b/target/arm/mte_helper.c
> @@ -563,8 +563,7 @@ static void mte_check_fail(CPUARMState *env, uint32_t desc,
>
> case 2:
> /* Tag check fail causes asynchronous flag set. */
> - mmu_idx = arm_mmu_idx_el(env, el);
> - if (regime_has_2_ranges(mmu_idx)) {
> + if (regime_has_2_ranges(arm_mmu_idx)) {
> select = extract64(dirty_ptr, 55, 1);
> } else {
> select = 0;
>
--
Regards,
Vincenzo
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] target/arm: Fix reported EL for mte_check_fail
2020-10-08 16:21 [PATCH 0/3] target/arm: MTE fixes Richard Henderson
2020-10-08 16:21 ` [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup Richard Henderson
@ 2020-10-08 16:21 ` Richard Henderson
2020-10-09 11:10 ` Vincenzo Frascino
2020-10-08 16:21 ` [PATCH 3/3] target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11 Richard Henderson
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Richard Henderson @ 2020-10-08 16:21 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm, vincenzo.frascino
The reporting in AArch64.TagCheckFail only depends on PSTATE.EL,
and not the AccType of the operation. There are two guest
visible problems that affect LDTR and STTR because of this:
(1) Selecting TCF0 vs TCF1 to decide on reporting,
(2) Report "data abort same el" not "data abort lower el".
Reported-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/mte_helper.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
index 734cc5ca67..153bd1e9df 100644
--- a/target/arm/mte_helper.c
+++ b/target/arm/mte_helper.c
@@ -525,14 +525,10 @@ static void mte_check_fail(CPUARMState *env, uint32_t desc,
reg_el = regime_el(env, arm_mmu_idx);
sctlr = env->cp15.sctlr_el[reg_el];
- switch (arm_mmu_idx) {
- case ARMMMUIdx_E10_0:
- case ARMMMUIdx_E20_0:
- el = 0;
+ el = arm_current_el(env);
+ if (el == 0) {
tcf = extract64(sctlr, 38, 2);
- break;
- default:
- el = reg_el;
+ } else {
tcf = extract64(sctlr, 40, 2);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] target/arm: Fix reported EL for mte_check_fail
2020-10-08 16:21 ` [PATCH 2/3] target/arm: Fix reported EL for mte_check_fail Richard Henderson
@ 2020-10-09 11:10 ` Vincenzo Frascino
0 siblings, 0 replies; 10+ messages in thread
From: Vincenzo Frascino @ 2020-10-09 11:10 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: peter.maydell, qemu-arm
On 10/8/20 5:21 PM, Richard Henderson wrote:
> The reporting in AArch64.TagCheckFail only depends on PSTATE.EL,
> and not the AccType of the operation. There are two guest
> visible problems that affect LDTR and STTR because of this:
>
> (1) Selecting TCF0 vs TCF1 to decide on reporting,
> (2) Report "data abort same el" not "data abort lower el".
>
> Reported-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Tested-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
> target/arm/mte_helper.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> index 734cc5ca67..153bd1e9df 100644
> --- a/target/arm/mte_helper.c
> +++ b/target/arm/mte_helper.c
> @@ -525,14 +525,10 @@ static void mte_check_fail(CPUARMState *env, uint32_t desc,
> reg_el = regime_el(env, arm_mmu_idx);
> sctlr = env->cp15.sctlr_el[reg_el];
>
> - switch (arm_mmu_idx) {
> - case ARMMMUIdx_E10_0:
> - case ARMMMUIdx_E20_0:
> - el = 0;
> + el = arm_current_el(env);
> + if (el == 0) {
> tcf = extract64(sctlr, 38, 2);
> - break;
> - default:
> - el = reg_el;
> + } else {
> tcf = extract64(sctlr, 40, 2);
> }
>
>
--
Regards,
Vincenzo
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11
2020-10-08 16:21 [PATCH 0/3] target/arm: MTE fixes Richard Henderson
2020-10-08 16:21 ` [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup Richard Henderson
2020-10-08 16:21 ` [PATCH 2/3] target/arm: Fix reported EL for mte_check_fail Richard Henderson
@ 2020-10-08 16:21 ` Richard Henderson
2020-10-09 11:10 ` Vincenzo Frascino
2020-10-09 11:11 ` [PATCH 0/3] target/arm: MTE fixes Vincenzo Frascino
2020-10-20 14:35 ` Peter Maydell
4 siblings, 1 reply; 10+ messages in thread
From: Richard Henderson @ 2020-10-08 16:21 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, qemu-arm, vincenzo.frascino
Unlike many other bits in HCR_EL2, the description for this
bit does not contain the phrase "if ... this field behaves
as 0 for all purposes other than", so do not squash the bit
in arm_hcr_el2_eff.
Instead, replicate the E2H+TGE test in the two places that
require it.
Reported-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/internals.h | 9 +++++----
target/arm/helper.c | 9 +++++----
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index ae99725d2b..5460678756 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1252,10 +1252,11 @@ static inline bool allocation_tag_access_enabled(CPUARMState *env, int el,
&& !(env->cp15.scr_el3 & SCR_ATA)) {
return false;
}
- if (el < 2
- && arm_feature(env, ARM_FEATURE_EL2)
- && !(arm_hcr_el2_eff(env) & HCR_ATA)) {
- return false;
+ if (el < 2 && arm_feature(env, ARM_FEATURE_EL2)) {
+ uint64_t hcr = arm_hcr_el2_eff(env);
+ if (!(hcr & HCR_ATA) && (!(hcr & HCR_E2H) || !(hcr & HCR_TGE))) {
+ return false;
+ }
}
sctlr &= (el == 0 ? SCTLR_ATA0 : SCTLR_ATA);
return sctlr != 0;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index cd0779ff5f..0620572e44 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6874,10 +6874,11 @@ static CPAccessResult access_mte(CPUARMState *env, const ARMCPRegInfo *ri,
{
int el = arm_current_el(env);
- if (el < 2 &&
- arm_feature(env, ARM_FEATURE_EL2) &&
- !(arm_hcr_el2_eff(env) & HCR_ATA)) {
- return CP_ACCESS_TRAP_EL2;
+ if (el < 2 && arm_feature(env, ARM_FEATURE_EL2)) {
+ uint64_t hcr = arm_hcr_el2_eff(env);
+ if (!(hcr & HCR_ATA) && (!(hcr & HCR_E2H) || !(hcr & HCR_TGE))) {
+ return CP_ACCESS_TRAP_EL2;
+ }
}
if (el < 3 &&
arm_feature(env, ARM_FEATURE_EL3) &&
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11
2020-10-08 16:21 ` [PATCH 3/3] target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11 Richard Henderson
@ 2020-10-09 11:10 ` Vincenzo Frascino
0 siblings, 0 replies; 10+ messages in thread
From: Vincenzo Frascino @ 2020-10-09 11:10 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: peter.maydell, qemu-arm
On 10/8/20 5:21 PM, Richard Henderson wrote:
> Unlike many other bits in HCR_EL2, the description for this
> bit does not contain the phrase "if ... this field behaves
> as 0 for all purposes other than", so do not squash the bit
> in arm_hcr_el2_eff.
>
> Instead, replicate the E2H+TGE test in the two places that
> require it.
>
> Reported-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Tested-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
> target/arm/internals.h | 9 +++++----
> target/arm/helper.c | 9 +++++----
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index ae99725d2b..5460678756 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1252,10 +1252,11 @@ static inline bool allocation_tag_access_enabled(CPUARMState *env, int el,
> && !(env->cp15.scr_el3 & SCR_ATA)) {
> return false;
> }
> - if (el < 2
> - && arm_feature(env, ARM_FEATURE_EL2)
> - && !(arm_hcr_el2_eff(env) & HCR_ATA)) {
> - return false;
> + if (el < 2 && arm_feature(env, ARM_FEATURE_EL2)) {
> + uint64_t hcr = arm_hcr_el2_eff(env);
> + if (!(hcr & HCR_ATA) && (!(hcr & HCR_E2H) || !(hcr & HCR_TGE))) {
> + return false;
> + }
> }
> sctlr &= (el == 0 ? SCTLR_ATA0 : SCTLR_ATA);
> return sctlr != 0;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index cd0779ff5f..0620572e44 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6874,10 +6874,11 @@ static CPAccessResult access_mte(CPUARMState *env, const ARMCPRegInfo *ri,
> {
> int el = arm_current_el(env);
>
> - if (el < 2 &&
> - arm_feature(env, ARM_FEATURE_EL2) &&
> - !(arm_hcr_el2_eff(env) & HCR_ATA)) {
> - return CP_ACCESS_TRAP_EL2;
> + if (el < 2 && arm_feature(env, ARM_FEATURE_EL2)) {
> + uint64_t hcr = arm_hcr_el2_eff(env);
> + if (!(hcr & HCR_ATA) && (!(hcr & HCR_E2H) || !(hcr & HCR_TGE))) {
> + return CP_ACCESS_TRAP_EL2;
> + }
> }
> if (el < 3 &&
> arm_feature(env, ARM_FEATURE_EL3) &&
>
--
Regards,
Vincenzo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] target/arm: MTE fixes
2020-10-08 16:21 [PATCH 0/3] target/arm: MTE fixes Richard Henderson
` (2 preceding siblings ...)
2020-10-08 16:21 ` [PATCH 3/3] target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11 Richard Henderson
@ 2020-10-09 11:11 ` Vincenzo Frascino
2020-10-20 14:35 ` Peter Maydell
4 siblings, 0 replies; 10+ messages in thread
From: Vincenzo Frascino @ 2020-10-09 11:11 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: peter.maydell, qemu-arm
Hi Richard,
On 10/8/20 5:21 PM, Richard Henderson wrote:
> One code cleanup and two bug fixes for MTE.
>
> Vincenzo, thanks for the clear report. Can you please run
> this through your test case?
>
>
No problem, thank you for addressing the issues quickly. I did run my tests and
added my tags to the relevant patches.
> r~
>
>
> Richard Henderson (3):
> target/arm: Remove redundant mmu_idx lookup
> target/arm: Fix reported EL for mte_check_fail
> target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11
>
> target/arm/internals.h | 9 +++++----
> target/arm/helper.c | 9 +++++----
> target/arm/mte_helper.c | 13 ++++---------
> 3 files changed, 14 insertions(+), 17 deletions(-)
>
--
Regards,
Vincenzo
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/3] target/arm: MTE fixes
2020-10-08 16:21 [PATCH 0/3] target/arm: MTE fixes Richard Henderson
` (3 preceding siblings ...)
2020-10-09 11:11 ` [PATCH 0/3] target/arm: MTE fixes Vincenzo Frascino
@ 2020-10-20 14:35 ` Peter Maydell
4 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2020-10-20 14:35 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-arm, vincenzo.frascino, QEMU Developers
On Thu, 8 Oct 2020 at 17:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> One code cleanup and two bug fixes for MTE.
>
> Vincenzo, thanks for the clear report. Can you please run
> this through your test case?
>
>
> r~
>
>
> Richard Henderson (3):
> target/arm: Remove redundant mmu_idx lookup
> target/arm: Fix reported EL for mte_check_fail
> target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11
Applied to target-arm.next, thanks.
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread