[PATCH 0/3] target/arm: MTE fixes

[PATCH 0/3] target/arm: MTE fixes

[Richard Henderson]

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

 target/arm/internals.h  |  9 +++++----
 target/arm/helper.c     |  9 +++++----
 target/arm/mte_helper.c | 13 ++++---------
 3 files changed, 14 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH 1/3] target/arm: Remove redundant mmu_idx lookup

[Richard Henderson]

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

[PATCH 2/3] target/arm: Fix reported EL for mte_check_fail

[Richard Henderson]

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

[PATCH 3/3] target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11

[Richard Henderson]

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

Re: [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 2/3] target/arm: Fix reported EL for mte_check_fail

[Vincenzo Frascino]

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

Re: [PATCH 3/3] target/arm: Ignore HCR_EL2.ATA when {E2H,TGE} != 11

[Vincenzo Frascino]

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

Re: [PATCH 0/3] target/arm: MTE fixes

[Vincenzo Frascino]

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

Re: [PATCH 1/3] target/arm: Remove redundant mmu_idx lookup

[Vincenzo Frascino]

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

Re: [PATCH 0/3] target/arm: MTE fixes

[Peter Maydell]

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