[PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Anshuman Khandual]

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@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
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:
 			/*
-- 
2.30.2

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@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

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@arm.com>

> ---
> 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:
>  			/*

Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Mark Rutland]

On Thu, Jun 13, 2024 at 03:15:38PM +0530, 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@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
> 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:
>  			/*
> -- 
> 2.30.2
> 
> 

Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Marc Zyngier]

On Thu, 13 Jun 2024 10:45:38 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> 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.

I'd rather we do not drop the existing #defines, for a very
self-serving reason:

NV requires an implementation to synthesise fault syndromes, and these
definition are extensively used to compose the syndrome information
(see the NV MMU series at [1]). This is also heavily use to emulate
the AT instructions (fault reporting in PAR_EL1.FST).

Having additional helpers is fine. Dropping the base definitions
isn't, and I'd like to avoid reintroducing them.

Thanks,

	M.

[1] http://lore.kernel.org/r/20240529145628.3272630-1-maz@kernel.org

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Anshuman Khandual]

On 6/13/24 16:53, Marc Zyngier wrote:
> On Thu, 13 Jun 2024 10:45:38 +0100,
> Anshuman Khandual <anshuman.khandual@arm.com> 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.
> 
> I'd rather we do not drop the existing #defines, for a very
> self-serving reason:
> 
> NV requires an implementation to synthesise fault syndromes, and these
> definition are extensively used to compose the syndrome information
> (see the NV MMU series at [1]). This is also heavily use to emulate
> the AT instructions (fault reporting in PAR_EL1.FST).
> 
> Having additional helpers is fine. Dropping the base definitions
> isn't, and I'd like to avoid reintroducing them.

You would like to just leave behind all the existing level 0 syndrome macro
definitions in place ?

#define ESR_ELx_FSC_ACCESS	(0x08)
#define ESR_ELx_FSC_FAULT	(0x04)
#define ESR_ELx_FSC_PERM	(0x0C)

Or which are rather

#define ESR_ELx_FSC_ACCESS	ESR_ELx_FSC_ACCESS_L0
#define ESR_ELx_FSC_FAULT	ESR_ELx_FSC_FAULT_L0
#define ESR_ELx_FSC_PERM	ESR_ELx_FSC_PERM_L0

But just wondering why cannot ESR_ELx_FSC_[ACCESS|FAULT|PERM]_L0 definitions
be used directly in new use cases ?

> 
> Thanks,
> 
> 	M.
> 
> [1] http://lore.kernel.org/r/20240529145628.3272630-1-maz@kernel.org
> 

Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Marc Zyngier]

On Fri, 14 Jun 2024 03:24:53 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> On 6/13/24 16:53, Marc Zyngier wrote:
> > On Thu, 13 Jun 2024 10:45:38 +0100,
> > Anshuman Khandual <anshuman.khandual@arm.com> 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.
> > 
> > I'd rather we do not drop the existing #defines, for a very
> > self-serving reason:
> > 
> > NV requires an implementation to synthesise fault syndromes, and these
> > definition are extensively used to compose the syndrome information
> > (see the NV MMU series at [1]). This is also heavily use to emulate
> > the AT instructions (fault reporting in PAR_EL1.FST).
> > 
> > Having additional helpers is fine. Dropping the base definitions
> > isn't, and I'd like to avoid reintroducing them.
> 
> You would like to just leave behind all the existing level 0 syndrome macro
> definitions in place ?

They are not level 0. They are values for the type of the fault. They
are *abused* as level 0, but that's not what they are here for.

> 
> #define ESR_ELx_FSC_ACCESS	(0x08)
> #define ESR_ELx_FSC_FAULT	(0x04)
> #define ESR_ELx_FSC_PERM	(0x0C)

+ ESR_ELx_FSC_{TYPE,LEVEL}, because they are convenient macros to
extract the type/level of a fault. NV further adds ESR_ELx_FSC_ADDRSZ
which has been missing.

> 
> Or which are rather
> 
> #define ESR_ELx_FSC_ACCESS	ESR_ELx_FSC_ACCESS_L0
> #define ESR_ELx_FSC_FAULT	ESR_ELx_FSC_FAULT_L0
> #define ESR_ELx_FSC_PERM	ESR_ELx_FSC_PERM_L0

I definitely prefer the former.

> But just wondering why cannot ESR_ELx_FSC_[ACCESS|FAULT|PERM]_L0 definitions
> be used directly in new use cases ?

Because that is semantically wrong to add/or a level on something that
*already* describes a level. Specially for the level -1 case.

On top of that, what I dislike the most about this patch is that it
defines discrete values for something that could be parametric at zero
cost, just like ESR_ELx_FSC_SEA_TTW(). Yes, there is some additional
complexity, but nothing that the compiler can't elide.

For example, something like this:

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 7abf09df7033..c320aeb1bb9a 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -121,6 +121,10 @@
 #define ESR_ELx_FSC_SECC	(0x18)
 #define ESR_ELx_FSC_SECC_TTW(n)	(0x1c + (n))
 
+#define ESR_ELx_FSC_FAULT_nL	(0x2C)
+#define ESR_ELx_FSC_FAULT_L(n)	(((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \
+				  	    ESR_ELx_FSC_FAULT) + (n))
+
 /* ISS field definitions for Data Aborts */
 #define ESR_ELx_ISV_SHIFT	(24)
 #define ESR_ELx_ISV		(UL(1) << ESR_ELx_ISV_SHIFT)

Importantly, it avoids the ESR_ELx_FSC_FAULT_LN1 horror, and allows
ESR_ELx_FSC_FAULT_L(-1) to be written.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Anshuman Khandual]

On 6/14/24 16:07, Marc Zyngier wrote:
> On Fri, 14 Jun 2024 03:24:53 +0100,
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>> On 6/13/24 16:53, Marc Zyngier wrote:
>>> On Thu, 13 Jun 2024 10:45:38 +0100,
>>> Anshuman Khandual <anshuman.khandual@arm.com> 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.
>>>
>>> I'd rather we do not drop the existing #defines, for a very
>>> self-serving reason:
>>>
>>> NV requires an implementation to synthesise fault syndromes, and these
>>> definition are extensively used to compose the syndrome information
>>> (see the NV MMU series at [1]). This is also heavily use to emulate
>>> the AT instructions (fault reporting in PAR_EL1.FST).
>>>
>>> Having additional helpers is fine. Dropping the base definitions
>>> isn't, and I'd like to avoid reintroducing them.
>>
>> You would like to just leave behind all the existing level 0 syndrome macro
>> definitions in place ?
> 
> They are not level 0. They are values for the type of the fault. They
> are *abused* as level 0, but that's not what they are here for.

After thinking through the above statement multiple times, just realized
that individual page table level fault type value is *derived* from given
fault type arithmetically, which is also reflected in the macros you have
proposed later.

> 
>>
>> #define ESR_ELx_FSC_ACCESS	(0x08)
>> #define ESR_ELx_FSC_FAULT	(0x04)
>> #define ESR_ELx_FSC_PERM	(0x0C)
> 
> + ESR_ELx_FSC_{TYPE,LEVEL}, because they are convenient macros to
> extract the type/level of a fault. NV further adds ESR_ELx_FSC_ADDRSZ
> which has been missing.

Understood, this extracts both the fault type and its level as well.

> 
>>
>> Or which are rather
>>
>> #define ESR_ELx_FSC_ACCESS	ESR_ELx_FSC_ACCESS_L0
>> #define ESR_ELx_FSC_FAULT	ESR_ELx_FSC_FAULT_L0
>> #define ESR_ELx_FSC_PERM	ESR_ELx_FSC_PERM_L0
> 
> I definitely prefer the former.

Okay.

> 
>> But just wondering why cannot ESR_ELx_FSC_[ACCESS|FAULT|PERM]_L0 definitions
>> be used directly in new use cases ?
> 
> Because that is semantically wrong to add/or a level on something that
> *already* describes a level. Specially for the level -1 case.

Fair enough, as the value already have both the parameters.

> 
> On top of that, what I dislike the most about this patch is that it
> defines discrete values for something that could be parametric at zero
> cost, just like ESR_ELx_FSC_SEA_TTW(). Yes, there is some additional
> complexity, but nothing that the compiler can't elide.

Understood.

> 
> For example, something like this:
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7abf09df7033..c320aeb1bb9a 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -121,6 +121,10 @@
>  #define ESR_ELx_FSC_SECC	(0x18)
>  #define ESR_ELx_FSC_SECC_TTW(n)	(0x1c + (n))
>  
> +#define ESR_ELx_FSC_FAULT_nL	(0x2C)
> +#define ESR_ELx_FSC_FAULT_L(n)	(((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \
> +				  	    ESR_ELx_FSC_FAULT) + (n))
> +
>  /* ISS field definitions for Data Aborts */
>  #define ESR_ELx_ISV_SHIFT	(24)
>  #define ESR_ELx_ISV		(UL(1) << ESR_ELx_ISV_SHIFT)
> 
> Importantly, it avoids the ESR_ELx_FSC_FAULT_LN1 horror, and allows
> ESR_ELx_FSC_FAULT_L(-1) to be written.
> 
> 	M.
> 

Does the following re-worked patch looks okay ? Earlier set_thread_esr() changes
can be dropped from arch/arm64/mm/fault.c and also the original commit message
still makes sense.

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 7abf09df7033..6cd13ac61005 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -121,6 +121,13 @@
 #define ESR_ELx_FSC_SECC       (0x18)
 #define ESR_ELx_FSC_SECC_TTW(n)        (0x1c + (n))
 
+/* Status codes for individual page table levels */
+#define ESR_ELx_FSC_ACCESS_L(n)        (ESR_ELx_FSC_ACCESS + n)
+#define ESR_ELx_FSC_FAULT_nL   (0x2C)
+#define ESR_ELx_FSC_FAULT_L(n) (((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \
+                                           ESR_ELx_FSC_FAULT) + (n))
+#define ESR_ELx_FSC_PERM_L(n)  (ESR_ELx_FSC_PERM + n)
+
 /* ISS field definitions for Data Aborts */
 #define ESR_ELx_ISV_SHIFT      (24)
 #define ESR_ELx_ISV            (UL(1) << ESR_ELx_ISV_SHIFT)
@@ -388,20 +395,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_L(3)) ||
+              (esr == ESR_ELx_FSC_FAULT_L(2)) ||
+              (esr == ESR_ELx_FSC_FAULT_L(1)) ||
+              (esr == ESR_ELx_FSC_FAULT_L(0)) ||
+              (esr == ESR_ELx_FSC_FAULT_L(-1));
 }
 
 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_L(3)) ||
+              (esr == ESR_ELx_FSC_PERM_L(2)) ||
+              (esr == ESR_ELx_FSC_PERM_L(1)) ||
+              (esr == ESR_ELx_FSC_PERM_L(0));
 }
 
 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_L(3)) ||
+              (esr == ESR_ELx_FSC_ACCESS_L(2)) ||
+              (esr == ESR_ELx_FSC_ACCESS_L(1)) ||
+              (esr == ESR_ELx_FSC_ACCESS_L(0));
 }
 
 /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
 

Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Marc Zyngier]

On Mon, 17 Jun 2024 04:15:40 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> Does the following re-worked patch looks okay ? Earlier set_thread_esr() changes
> can be dropped from arch/arm64/mm/fault.c and also the original commit message
> still makes sense.
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7abf09df7033..6cd13ac61005 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -121,6 +121,13 @@
>  #define ESR_ELx_FSC_SECC       (0x18)
>  #define ESR_ELx_FSC_SECC_TTW(n)        (0x1c + (n))
>  
> +/* Status codes for individual page table levels */
> +#define ESR_ELx_FSC_ACCESS_L(n)        (ESR_ELx_FSC_ACCESS + n)
> +#define ESR_ELx_FSC_FAULT_nL   (0x2C)
> +#define ESR_ELx_FSC_FAULT_L(n) (((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \
> +                                           ESR_ELx_FSC_FAULT) + (n))
> +#define ESR_ELx_FSC_PERM_L(n)  (ESR_ELx_FSC_PERM + n)
> +
>  /* ISS field definitions for Data Aborts */
>  #define ESR_ELx_ISV_SHIFT      (24)
>  #define ESR_ELx_ISV            (UL(1) << ESR_ELx_ISV_SHIFT)
> @@ -388,20 +395,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_L(3)) ||
> +              (esr == ESR_ELx_FSC_FAULT_L(2)) ||
> +              (esr == ESR_ELx_FSC_FAULT_L(1)) ||
> +              (esr == ESR_ELx_FSC_FAULT_L(0)) ||
> +              (esr == ESR_ELx_FSC_FAULT_L(-1));
>  }
>  
>  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_L(3)) ||
> +              (esr == ESR_ELx_FSC_PERM_L(2)) ||
> +              (esr == ESR_ELx_FSC_PERM_L(1)) ||
> +              (esr == ESR_ELx_FSC_PERM_L(0));
>  }
>  
>  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_L(3)) ||
> +              (esr == ESR_ELx_FSC_ACCESS_L(2)) ||
> +              (esr == ESR_ELx_FSC_ACCESS_L(1)) ||
> +              (esr == ESR_ELx_FSC_ACCESS_L(0));
>  }
>  
>  /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */

This looks better indeed.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Anshuman Khandual]

On 6/17/24 13:13, Marc Zyngier wrote:
> On Mon, 17 Jun 2024 04:15:40 +0100,
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>
>> Does the following re-worked patch looks okay ? Earlier set_thread_esr() changes
>> can be dropped from arch/arm64/mm/fault.c and also the original commit message
>> still makes sense.
>>
>> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
>> index 7abf09df7033..6cd13ac61005 100644
>> --- a/arch/arm64/include/asm/esr.h
>> +++ b/arch/arm64/include/asm/esr.h
>> @@ -121,6 +121,13 @@
>>  #define ESR_ELx_FSC_SECC       (0x18)
>>  #define ESR_ELx_FSC_SECC_TTW(n)        (0x1c + (n))
>>  
>> +/* Status codes for individual page table levels */
>> +#define ESR_ELx_FSC_ACCESS_L(n)        (ESR_ELx_FSC_ACCESS + n)
>> +#define ESR_ELx_FSC_FAULT_nL   (0x2C)
>> +#define ESR_ELx_FSC_FAULT_L(n) (((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \
>> +                                           ESR_ELx_FSC_FAULT) + (n))
>> +#define ESR_ELx_FSC_PERM_L(n)  (ESR_ELx_FSC_PERM + n)
>> +
>>  /* ISS field definitions for Data Aborts */
>>  #define ESR_ELx_ISV_SHIFT      (24)
>>  #define ESR_ELx_ISV            (UL(1) << ESR_ELx_ISV_SHIFT)
>> @@ -388,20 +395,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_L(3)) ||
>> +              (esr == ESR_ELx_FSC_FAULT_L(2)) ||
>> +              (esr == ESR_ELx_FSC_FAULT_L(1)) ||
>> +              (esr == ESR_ELx_FSC_FAULT_L(0)) ||
>> +              (esr == ESR_ELx_FSC_FAULT_L(-1));
>>  }
>>  
>>  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_L(3)) ||
>> +              (esr == ESR_ELx_FSC_PERM_L(2)) ||
>> +              (esr == ESR_ELx_FSC_PERM_L(1)) ||
>> +              (esr == ESR_ELx_FSC_PERM_L(0));
>>  }
>>  
>>  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_L(3)) ||
>> +              (esr == ESR_ELx_FSC_ACCESS_L(2)) ||
>> +              (esr == ESR_ELx_FSC_ACCESS_L(1)) ||
>> +              (esr == ESR_ELx_FSC_ACCESS_L(0));
>>  }
>>  
>>  /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
> 
> This looks better indeed.

Thanks Marc.

Hello Mark/Ryan,

Could I still keep your tags for the patch, or it's better to just
drop them as there are some new changes being folded in. Please do
advise. Thank you.

- Anshuman

Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Mark Rutland]

On Mon, Jun 17, 2024 at 02:09:27PM +0530, Anshuman Khandual wrote:
> On 6/17/24 13:13, Marc Zyngier wrote:
> > On Mon, 17 Jun 2024 04:15:40 +0100,
> > Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> >>
> >> Does the following re-worked patch looks okay ? Earlier set_thread_esr() changes
> >> can be dropped from arch/arm64/mm/fault.c and also the original commit message
> >> still makes sense.
> >>
> >> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> >> index 7abf09df7033..6cd13ac61005 100644
> >> --- a/arch/arm64/include/asm/esr.h
> >> +++ b/arch/arm64/include/asm/esr.h
> >> @@ -121,6 +121,13 @@
> >>  #define ESR_ELx_FSC_SECC       (0x18)
> >>  #define ESR_ELx_FSC_SECC_TTW(n)        (0x1c + (n))
> >>  
> >> +/* Status codes for individual page table levels */
> >> +#define ESR_ELx_FSC_ACCESS_L(n)        (ESR_ELx_FSC_ACCESS + n)
> >> +#define ESR_ELx_FSC_FAULT_nL   (0x2C)
> >> +#define ESR_ELx_FSC_FAULT_L(n) (((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \
> >> +                                           ESR_ELx_FSC_FAULT) + (n))
> >> +#define ESR_ELx_FSC_PERM_L(n)  (ESR_ELx_FSC_PERM + n)
> >> +
> >>  /* ISS field definitions for Data Aborts */
> >>  #define ESR_ELx_ISV_SHIFT      (24)
> >>  #define ESR_ELx_ISV            (UL(1) << ESR_ELx_ISV_SHIFT)
> >> @@ -388,20 +395,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_L(3)) ||
> >> +              (esr == ESR_ELx_FSC_FAULT_L(2)) ||
> >> +              (esr == ESR_ELx_FSC_FAULT_L(1)) ||
> >> +              (esr == ESR_ELx_FSC_FAULT_L(0)) ||
> >> +              (esr == ESR_ELx_FSC_FAULT_L(-1));
> >>  }
> >>  
> >>  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_L(3)) ||
> >> +              (esr == ESR_ELx_FSC_PERM_L(2)) ||
> >> +              (esr == ESR_ELx_FSC_PERM_L(1)) ||
> >> +              (esr == ESR_ELx_FSC_PERM_L(0));
> >>  }
> >>  
> >>  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_L(3)) ||
> >> +              (esr == ESR_ELx_FSC_ACCESS_L(2)) ||
> >> +              (esr == ESR_ELx_FSC_ACCESS_L(1)) ||
> >> +              (esr == ESR_ELx_FSC_ACCESS_L(0));
> >>  }
> >>  
> >>  /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
> > 
> > This looks better indeed.
> 
> Thanks Marc.
> 
> Hello Mark/Ryan,
> 
> Could I still keep your tags for the patch, or it's better to just
> drop them as there are some new changes being folded in. Please do
> advise. Thank you.

Just drop them for now -- we can easily reply with new tags (and I
probably will, as the above looks sane to me).

Mark.