| Message ID | 20240613094538.3263536-1-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/mm: Drop ESR_ELx_FSC_TYPE | expand |
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: > /*
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 > >
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
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 >
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.
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 */
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.
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
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.
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: /*
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(-)