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

[Joey Gouly]

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 <broonie@xxxxxxxxxx>
> ---
>  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