2023-04-18 14:13:23

by Mark Brown

[permalink] [raw]
Subject: [PATCH 0/2] arm64: Add decode of ISS2 to data abort reports

We provide fairly detailed decode of ESR for data aborts but do not
currently cover the information reported in ISS2 which has had quite a
bit of additional information added to it by recent architecture
extensions. Add decode for this information to aid in debugging, for
completeness including features we don't actually use yet.

Signed-off-by: Mark Brown <[email protected]>
---
Mark Brown (2):
arm64/esr: Use GENMASK() for the ISS mask
arm64/esr: Add decode of ISS2 to data abort reporting

arch/arm64/include/asm/esr.h | 19 ++++++++++++++++++-
arch/arm64/mm/fault.c | 14 ++++++++++++--
2 files changed, 30 insertions(+), 3 deletions(-)
---
base-commit: e8d018dd0257f744ca50a729e3d042cf2ec9da65
change-id: 20230417-arm64-iss2-dabt-decode-ec9b46c98a91

Best regards,
--
Mark Brown <[email protected]>


2023-04-18 14:13:25

by Mark Brown

[permalink] [raw]
Subject: [PATCH 1/2] arm64/esr: Use GENMASK() for the ISS mask

We express the mask for ESR_ELx.ISS in a non-standard manner, not using
the standard helpers. In preparation for adding decode for ISS2 convert to
use GENMASK(). No functional change.

Signed-off-by: Mark Brown <[email protected]>
---
arch/arm64/include/asm/esr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 8487aec9b658..0bd879007168 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -75,7 +75,7 @@

#define ESR_ELx_IL_SHIFT (25)
#define ESR_ELx_IL (UL(1) << ESR_ELx_IL_SHIFT)
-#define ESR_ELx_ISS_MASK (ESR_ELx_IL - 1)
+#define ESR_ELx_ISS_MASK (GENMASK(24, 0))
#define ESR_ELx_ISS(esr) ((esr) & ESR_ELx_ISS_MASK)

/* ISS field definitions shared by different classes */

--
2.30.2

2023-04-18 14:13:26

by Mark Brown

[permalink] [raw]
Subject: [PATCH 2/2] arm64/esr: Add decode of ISS2 to data abort reporting

The architecture has added more information about faults to ISS2 within
ESR. Add decode of this to our data abort fault decode to aid diagnostics.
Features that are not currently enabled are included here for completeness.

Since the architecture specifies the values of bits within ISS2 in terms
of ISS2 rather than in terms of the register as a whole we do so for our
definitions as well, this makes it easier to review bitfield definitions.

Signed-off-by: Mark Brown <[email protected]>
---
arch/arm64/include/asm/esr.h | 17 +++++++++++++++++
arch/arm64/mm/fault.c | 14 ++++++++++++--
2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 0bd879007168..17bc6536ffea 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -77,6 +77,9 @@
#define ESR_ELx_IL (UL(1) << ESR_ELx_IL_SHIFT)
#define ESR_ELx_ISS_MASK (GENMASK(24, 0))
#define ESR_ELx_ISS(esr) ((esr) & ESR_ELx_ISS_MASK)
+#define ESR_ELx_ISS2_SHIFT (32)
+#define ESR_ELx_ISS2_MASK (GENMASK_ULL(55, 32))
+#define ESR_ELx_ISS2(esr) ((esr) & ESR_ELx_ISS_MASK)

/* ISS field definitions shared by different classes */
#define ESR_ELx_WNR_SHIFT (6)
@@ -140,6 +143,20 @@
#define ESR_ELx_CM_SHIFT (8)
#define ESR_ELx_CM (UL(1) << ESR_ELx_CM_SHIFT)

+/* ISS2 field definitions for Data Aborts */
+#define ESR_ELx_TnD_SHIFT (10)
+#define ESR_ELx_TnD (UL(1) << ESR_ELx_TnD_SHIFT)
+#define ESR_ELx_TagAccess_SHIFT (9)
+#define ESR_ELx_TagAccess (UL(1) << ESR_ELx_TagAccess_SHIFT)
+#define ESR_ELx_GCS_SHIFT (8)
+#define ESR_ELx_GCS (UL(1) << ESR_ELx_GCS_SHIFT)
+#define ESR_ELx_Overlay_SHIFT (6)
+#define ESR_ELx_Overlay (UL(1) << ESR_ELx_Overlay_SHIFT)
+#define ESR_ELx_DirtyBit_SHIFT (5)
+#define ESR_ELx_DirtyBit (UL(1) << ESR_ELx_DirtyBit_SHIFT)
+#define ESR_ELx_Xs_SHIFT (0)
+#define ESR_ELx_Xs_MASK (GENMASK_ULL(4, 0))
+
/* ISS field definitions for exceptions taken in to Hyp */
#define ESR_ELx_CV (UL(1) << 24)
#define ESR_ELx_COND_SHIFT (20)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index f4cb0f85ccf4..2e76dc613c86 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -66,6 +66,8 @@ static inline const struct fault_info *esr_to_debug_fault_info(unsigned long esr

static void data_abort_decode(unsigned long esr)
{
+ u64 iss2 = ESR_ELx_ISS2(esr);
+
pr_alert("Data abort info:\n");

if (esr & ESR_ELx_ISV) {
@@ -81,9 +83,17 @@ static void data_abort_decode(unsigned long esr)
pr_alert(" ISV = 0, ISS = 0x%08lx\n", esr & ESR_ELx_ISS_MASK);
}

- pr_alert(" CM = %lu, WnR = %lu\n",
+ pr_alert(" CM = %lu, WnR = %lu, TnD = %llu, TagAccess = %lld\n",
(esr & ESR_ELx_CM) >> ESR_ELx_CM_SHIFT,
- (esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT);
+ (esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT,
+ (iss2 & ESR_ELx_TnD) >> ESR_ELx_TnD_SHIFT,
+ (iss2 & ESR_ELx_TagAccess) >> ESR_ELx_TagAccess_SHIFT);
+
+ pr_alert(" GCS = %lld, Overlay = %llu, DirtyBit = %llu, Xs = %llu\n",
+ (iss2 & ESR_ELx_GCS) >> ESR_ELx_GCS_SHIFT,
+ (iss2 & ESR_ELx_Overlay) >> ESR_ELx_Overlay_SHIFT,
+ (iss2 & ESR_ELx_DirtyBit) >> ESR_ELx_DirtyBit_SHIFT,
+ (iss2 & ESR_ELx_Xs_MASK) >> ESR_ELx_Xs_SHIFT);
}

static void mem_abort_decode(unsigned long esr)

--
2.30.2

2023-05-09 14:52:21

by Joey Gouly

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64/esr: Add decode of ISS2 to data abort reporting

Hi Mark,

On Tue, Apr 18, 2023 at 02:57:32PM +0100, Mark Brown wrote:
> The architecture has added more information about faults to ISS2 within
> ESR. Add decode of this to our data abort fault decode to aid diagnostics.
> Features that are not currently enabled are included here for completeness.
>
> Since the architecture specifies the values of bits within ISS2 in terms
> of ISS2 rather than in terms of the register as a whole we do so for our
> definitions as well, this makes it easier to review bitfield definitions.
>
> Signed-off-by: Mark Brown <[email protected]>
> ---
> arch/arm64/include/asm/esr.h | 17 +++++++++++++++++
> arch/arm64/mm/fault.c | 14 ++++++++++++--
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 0bd879007168..17bc6536ffea 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -77,6 +77,9 @@
> #define ESR_ELx_IL (UL(1) << ESR_ELx_IL_SHIFT)
> #define ESR_ELx_ISS_MASK (GENMASK(24, 0))
> #define ESR_ELx_ISS(esr) ((esr) & ESR_ELx_ISS_MASK)
> +#define ESR_ELx_ISS2_SHIFT (32)
> +#define ESR_ELx_ISS2_MASK (GENMASK_ULL(55, 32))
> +#define ESR_ELx_ISS2(esr) ((esr) & ESR_ELx_ISS_MASK)

Typo here, should be ESR_ELx_ISS2_MASK.

>
> /* ISS field definitions shared by different classes */
> #define ESR_ELx_WNR_SHIFT (6)
> @@ -140,6 +143,20 @@
> #define ESR_ELx_CM_SHIFT (8)
> #define ESR_ELx_CM (UL(1) << ESR_ELx_CM_SHIFT)
>
> +/* ISS2 field definitions for Data Aborts */
> +#define ESR_ELx_TnD_SHIFT (10)
> +#define ESR_ELx_TnD (UL(1) << ESR_ELx_TnD_SHIFT)
> +#define ESR_ELx_TagAccess_SHIFT (9)
> +#define ESR_ELx_TagAccess (UL(1) << ESR_ELx_TagAccess_SHIFT)
> +#define ESR_ELx_GCS_SHIFT (8)
> +#define ESR_ELx_GCS (UL(1) << ESR_ELx_GCS_SHIFT)
> +#define ESR_ELx_Overlay_SHIFT (6)
> +#define ESR_ELx_Overlay (UL(1) << ESR_ELx_Overlay_SHIFT)
> +#define ESR_ELx_DirtyBit_SHIFT (5)
> +#define ESR_ELx_DirtyBit (UL(1) << ESR_ELx_DirtyBit_SHIFT)
> +#define ESR_ELx_Xs_SHIFT (0)
> +#define ESR_ELx_Xs_MASK (GENMASK_ULL(4, 0))
> +
> /* ISS field definitions for exceptions taken in to Hyp */
> #define ESR_ELx_CV (UL(1) << 24)
> #define ESR_ELx_COND_SHIFT (20)
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index f4cb0f85ccf4..2e76dc613c86 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -66,6 +66,8 @@ static inline const struct fault_info *esr_to_debug_fault_info(unsigned long esr
>
> static void data_abort_decode(unsigned long esr)
> {
> + u64 iss2 = ESR_ELx_ISS2(esr);
> +
> pr_alert("Data abort info:\n");
>
> if (esr & ESR_ELx_ISV) {
> @@ -81,9 +83,17 @@ static void data_abort_decode(unsigned long esr)
> pr_alert(" ISV = 0, ISS = 0x%08lx\n", esr & ESR_ELx_ISS_MASK);
> }
>
> - pr_alert(" CM = %lu, WnR = %lu\n",
> + pr_alert(" CM = %lu, WnR = %lu, TnD = %llu, TagAccess = %lld\n",
> (esr & ESR_ELx_CM) >> ESR_ELx_CM_SHIFT,
> - (esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT);
> + (esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT,
> + (iss2 & ESR_ELx_TnD) >> ESR_ELx_TnD_SHIFT,
> + (iss2 & ESR_ELx_TagAccess) >> ESR_ELx_TagAccess_SHIFT);
> +
> + pr_alert(" GCS = %lld, Overlay = %llu, DirtyBit = %llu, Xs = %llu\n",
> + (iss2 & ESR_ELx_GCS) >> ESR_ELx_GCS_SHIFT,
> + (iss2 & ESR_ELx_Overlay) >> ESR_ELx_Overlay_SHIFT,
> + (iss2 & ESR_ELx_DirtyBit) >> ESR_ELx_DirtyBit_SHIFT,
> + (iss2 & ESR_ELx_Xs_MASK) >> ESR_ELx_Xs_SHIFT);
> }
>
> static void mem_abort_decode(unsigned long esr)

Thanks,
Joey