Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Ryan Roberts]

On 13/06/2024 10:45, Anshuman Khandual wrote:
> Fault status codes at page table level 0, 1, 2 and 3 for access, permission
> and translation faults are architecturally organized in a way, that masking
> out ESR_ELx_FSC_TYPE, fetches Level 0 status code for the respective fault.
> 
> Helpers like esr_fsc_is_[translation|permission|access_flag]_fault() mask
> out ESR_ELx_FSC_TYPE before comparing against corresponding Level 0 status
> code as the kernel does not yet care about the page table level, the fault
> really occurred previously.
> 
> This scheme is starting to crumble after FEAT_LPA2 when level -1 got added.
> Fault status code for translation fault at level -1 is 0x2B which does not
> follow ESR_ELx_FSC_TYPE, requiring esr_fsc_is_translation_fault() changes.
> 
> This changes above helpers to compare against individual fault status code
> values for each page table level and drop ESR_ELx_FSC_TYPE which is losing
> its value as a common mask.
> 
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>

This certainly looks like a nice clean up from a readability point of view, and
will also make it easier to support extra levels in future. It's probably
marginally slower, but given you are comparing the lowest level first, which I
guess is most likely to fault, you will short circuit most comparisons most of
the time. So:

Reviewed-by: Ryan Roberts <ryan.roberts@xxxxxxx>

> ---
> This applies on 6.10-rc3
> 
>  arch/arm64/include/asm/esr.h | 42 +++++++++++++++++++++++++++---------
>  arch/arm64/mm/fault.c        |  4 ++--
>  2 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7abf09df7033..8cc0311d3fba 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -109,14 +109,23 @@
>  
>  /* Shared ISS fault status code(IFSC/DFSC) for Data/Instruction aborts */
>  #define ESR_ELx_FSC		(0x3F)
> -#define ESR_ELx_FSC_TYPE	(0x3C)
>  #define ESR_ELx_FSC_LEVEL	(0x03)
>  #define ESR_ELx_FSC_EXTABT	(0x10)
>  #define ESR_ELx_FSC_MTE		(0x11)
>  #define ESR_ELx_FSC_SERROR	(0x11)
> -#define ESR_ELx_FSC_ACCESS	(0x08)
> -#define ESR_ELx_FSC_FAULT	(0x04)
> -#define ESR_ELx_FSC_PERM	(0x0C)
> +#define ESR_ELx_FSC_ACCESS_L0	(0x08)
> +#define ESR_ELx_FSC_ACCESS_L1	(0x09)
> +#define ESR_ELx_FSC_ACCESS_L2	(0x0A)
> +#define ESR_ELx_FSC_ACCESS_L3	(0x0B)
> +#define ESR_ELx_FSC_FAULT_LN1	(0x2B)
> +#define ESR_ELx_FSC_FAULT_L0	(0x04)
> +#define ESR_ELx_FSC_FAULT_L1	(0x05)
> +#define ESR_ELx_FSC_FAULT_L2	(0x06)
> +#define ESR_ELx_FSC_FAULT_L3	(0x07)
> +#define ESR_ELx_FSC_PERM_L0	(0x0C)
> +#define ESR_ELx_FSC_PERM_L1	(0x0D)
> +#define ESR_ELx_FSC_PERM_L2	(0x0E)
> +#define ESR_ELx_FSC_PERM_L3	(0x0F)
>  #define ESR_ELx_FSC_SEA_TTW(n)	(0x14 + (n))
>  #define ESR_ELx_FSC_SECC	(0x18)
>  #define ESR_ELx_FSC_SECC_TTW(n)	(0x1c + (n))
> @@ -388,20 +397,33 @@ static inline bool esr_is_data_abort(unsigned long esr)
>  
>  static inline bool esr_fsc_is_translation_fault(unsigned long esr)
>  {
> -	/* Translation fault, level -1 */
> -	if ((esr & ESR_ELx_FSC) == 0b101011)
> -		return true;
> -	return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
> +	esr = esr & ESR_ELx_FSC;
> +
> +	return (esr == ESR_ELx_FSC_FAULT_L3) ||
> +	       (esr == ESR_ELx_FSC_FAULT_L2) ||
> +	       (esr == ESR_ELx_FSC_FAULT_L1) ||
> +	       (esr == ESR_ELx_FSC_FAULT_L0) ||
> +	       (esr == ESR_ELx_FSC_FAULT_LN1);
>  }
>  
>  static inline bool esr_fsc_is_permission_fault(unsigned long esr)
>  {
> -	return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_PERM;
> +	esr = esr & ESR_ELx_FSC;
> +
> +	return (esr == ESR_ELx_FSC_PERM_L3) ||
> +	       (esr == ESR_ELx_FSC_PERM_L2) ||
> +	       (esr == ESR_ELx_FSC_PERM_L1) ||
> +	       (esr == ESR_ELx_FSC_PERM_L0);
>  }
>  
>  static inline bool esr_fsc_is_access_flag_fault(unsigned long esr)
>  {
> -	return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_ACCESS;
> +	esr = esr & ESR_ELx_FSC;
> +
> +	return (esr == ESR_ELx_FSC_ACCESS_L3) ||
> +	       (esr == ESR_ELx_FSC_ACCESS_L2) ||
> +	       (esr == ESR_ELx_FSC_ACCESS_L1) ||
> +	       (esr == ESR_ELx_FSC_ACCESS_L0);
>  }
>  
>  /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 451ba7cbd5ad..7199aaff2a29 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -440,7 +440,7 @@ static void set_thread_esr(unsigned long address, unsigned long esr)
>  			 */
>  			esr &= ESR_ELx_EC_MASK | ESR_ELx_IL |
>  				ESR_ELx_CM | ESR_ELx_WNR;
> -			esr |= ESR_ELx_FSC_FAULT;
> +			esr |= ESR_ELx_FSC_FAULT_L0;
>  			break;
>  		case ESR_ELx_EC_IABT_LOW:
>  			/*
> @@ -449,7 +449,7 @@ static void set_thread_esr(unsigned long address, unsigned long esr)
>  			 * reported with that DFSC value, so we clear them.
>  			 */
>  			esr &= ESR_ELx_EC_MASK | ESR_ELx_IL;
> -			esr |= ESR_ELx_FSC_FAULT;
> +			esr |= ESR_ELx_FSC_FAULT_L0;
>  			break;
>  		default:
>  			/*