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 <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
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
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 <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
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 <[email protected]>
> ---
> 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:
> /*
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 <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
Acked-by: Mark Rutland <[email protected]>
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
>
>
On Thu, 13 Jun 2024 10:45:38 +0100,
Anshuman Khandual <[email protected]> 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/[email protected]
--
Without deviation from the norm, progress is not possible.
On 6/13/24 16:53, Marc Zyngier wrote:
> On Thu, 13 Jun 2024 10:45:38 +0100,
> Anshuman Khandual <[email protected]> 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/[email protected]
>
On Fri, 14 Jun 2024 03:24:53 +0100,
Anshuman Khandual <[email protected]> wrote:
> On 6/13/24 16:53, Marc Zyngier wrote:
> > On Thu, 13 Jun 2024 10:45:38 +0100,
> > Anshuman Khandual <[email protected]> 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.