diff mbox series

[V2] arm64/mm: Stop using ESR_ELx_FSC_TYPE during fault

Message ID 20240618034703.3622510-1-anshuman.khandual@arm.com (mailing list archive)
State New, archived
Headers show
Series [V2] arm64/mm: Stop using ESR_ELx_FSC_TYPE during fault | expand

Commit Message

Anshuman Khandual June 18, 2024, 3:47 a.m. UTC 
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, where in
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 stop using 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 v6.10-rc4 and still leaves behind ESR_ELx_FSC_TYPE for now.

Changes in V2:

- Defined ESR_ELx_FSC_[ACCESS|FAULT_PERM]_L() macros
- Changed fault helpers using the above macros instead
- Dropped each page table level fault status discrete values
- Dropped set_thread_esr() changes in arch/arm64/mm/fault.c
- Updated the commit message

Changes in V1:

https://lore.kernel.org/linux-arm-kernel/20240613094538.3263536-1-anshuman.khandual@arm.com/

 arch/arm64/include/asm/esr.h | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

Comments

Marc Zyngier June 18, 2024, 8:15 a.m. UTC | #1 
On Tue, 18 Jun 2024 04:47:03 +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, where in
> 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 stop using 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>

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.
Ryan Roberts June 20, 2024, 8:45 a.m. UTC | #2 
On 18/06/2024 04:47, 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, where in
> 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 stop using 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 v6.10-rc4 and still leaves behind ESR_ELx_FSC_TYPE for now.
> 
> Changes in V2:
> 
> - Defined ESR_ELx_FSC_[ACCESS|FAULT_PERM]_L() macros
> - Changed fault helpers using the above macros instead
> - Dropped each page table level fault status discrete values
> - Dropped set_thread_esr() changes in arch/arm64/mm/fault.c
> - Updated the commit message
> 
> Changes in V1:
> 
> https://lore.kernel.org/linux-arm-kernel/20240613094538.3263536-1-anshuman.khandual@arm.com/
> 
>  arch/arm64/include/asm/esr.h | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7abf09df7033..3f482500f71f 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -121,6 +121,14 @@
>  #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_PERM_L(n)	(ESR_ELx_FSC_PERM + 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))

I think the only real argument for parameterizing it like this is so we can
write "-1" as a parameter rather than "N1" as part of the macro name? Other than
that (marginal) benefit, personally I don't think this approach is very
extensible because we are devining a pattern from the encoding that doesn't
really exist. If we ever needed a level 4 or -3 the encoding would have to be
discontiguous and we would need to rework this again to accomodate. Perhaps the
chances of that ever happening are small enough that the problem can be ignored.

TBH, I didn't really follow Marc's argument for keeping the "type" macros either
since ESR_ELx_FSC_FAULT does not help to identify the type of a level -1 or -2
translation fault - the encoding is completely different. But I'll take it on
faith that Marc is correct and I just don't understand ;-)

Regardless, the implementation looks correct, so:

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

> +
>  /* ISS field definitions for Data Aborts */
>  #define ESR_ELx_ISV_SHIFT	(24)
>  #define ESR_ELx_ISV		(UL(1) << ESR_ELx_ISV_SHIFT)
> @@ -388,20 +396,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 */
Mark Rutland June 20, 2024, 9:58 a.m. UTC | #3 
On Thu, Jun 20, 2024 at 09:45:38AM +0100, Ryan Roberts wrote:
> On 18/06/2024 04:47, 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, where in
> > 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 stop using 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 v6.10-rc4 and still leaves behind ESR_ELx_FSC_TYPE for now.
> > 
> > Changes in V2:
> > 
> > - Defined ESR_ELx_FSC_[ACCESS|FAULT_PERM]_L() macros
> > - Changed fault helpers using the above macros instead
> > - Dropped each page table level fault status discrete values
> > - Dropped set_thread_esr() changes in arch/arm64/mm/fault.c
> > - Updated the commit message
> > 
> > Changes in V1:
> > 
> > https://lore.kernel.org/linux-arm-kernel/20240613094538.3263536-1-anshuman.khandual@arm.com/
> > 
> >  arch/arm64/include/asm/esr.h | 33 +++++++++++++++++++++++++++------
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> > index 7abf09df7033..3f482500f71f 100644
> > --- a/arch/arm64/include/asm/esr.h
> > +++ b/arch/arm64/include/asm/esr.h
> > @@ -121,6 +121,14 @@
> >  #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_PERM_L(n)	(ESR_ELx_FSC_PERM + 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))
> 
> I think the only real argument for parameterizing it like this is so we can
> write "-1" as a parameter rather than "N1" as part of the macro name? Other than
> that (marginal) benefit, personally I don't think this approach is very
> extensible because we are devining a pattern from the encoding that doesn't
> really exist. If we ever needed a level 4 or -3 the encoding would have to be
> discontiguous and we would need to rework this again to accomodate. Perhaps the
> chances of that ever happening are small enough that the problem can be ignored.

FWIW, I agree, I had preferred the spearate definitions because that
matched what was in the ARM ARM and didn't infer a pattern that could be
broken later.

> TBH, I didn't really follow Marc's argument for keeping the "type" macros either
> since ESR_ELx_FSC_FAULT does not help to identify the type of a level -1 or -2
> translation fault - the encoding is completely different. But I'll take it on
> faith that Marc is correct and I just don't understand ;-)
> 
> Regardless, the implementation looks correct, so:
> 
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>

Likewise, either way:

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

Mark.

> 
> > +
> >  /* ISS field definitions for Data Aborts */
> >  #define ESR_ELx_ISV_SHIFT	(24)
> >  #define ESR_ELx_ISV		(UL(1) << ESR_ELx_ISV_SHIFT)
> > @@ -388,20 +396,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 */
>
Catalin Marinas June 24, 2024, 6:12 p.m. UTC | #4 
On Tue, 18 Jun 2024 09:17:03 +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, where in
> the fault really occurred previously.
> 
> [...]

Applied to arm64 (for-next/misc), thanks!

[1/1] arm64/mm: Stop using ESR_ELx_FSC_TYPE during fault
      https://git.kernel.org/arm64/c/573611145fcb
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 7abf09df7033..3f482500f71f 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -121,6 +121,14 @@ 
 #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_PERM_L(n)	(ESR_ELx_FSC_PERM + 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)
@@ -388,20 +396,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 */