Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE

[Mark Rutland <mark.rutland@arm.com>]

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.