* [RFC PATCH] ARM: Enable imprecise external aborts earlier @ 2014-02-07 16:19 Fabrice GASNIER 2014-02-07 16:19 ` [RFC PATCH] ARM: Add imprecise abort enable/disable macro Fabrice GASNIER 0 siblings, 1 reply; 25+ messages in thread From: Fabrice GASNIER @ 2014-02-07 16:19 UTC (permalink / raw) To: linux-arm-kernel Some HW / drivers like PCIe use imprecise aborts on ARM to detect ports that are behind a switch, by using: hook_fault_code(16+6, ...); At PCIe bus enumeration, imprecise aborts are not enabled. Imprecise aborts are enabled late during bootup process, when userland init starts. It seems unmasked later when starting userland init process (e.g. when CPSR.A bit on arm is cleared). Aborts are deferred until then. Enable imprecise external aborts earlier during bootup. Reference : http://marc.info/?l=linux-arm-kernel&m=139118404708291&w=2 Fabrice Gasnier (1): ARM: Add imprecise abort enable/disable macro arch/arm/include/asm/irqflags.h | 33 +++++++++++++++++++++++++++++++++ arch/arm/kernel/smp.c | 1 + arch/arm/kernel/traps.c | 4 ++++ 3 files changed, 38 insertions(+) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-07 16:19 [RFC PATCH] ARM: Enable imprecise external aborts earlier Fabrice GASNIER @ 2014-02-07 16:19 ` Fabrice GASNIER 2014-02-07 17:09 ` Will Deacon 2014-02-10 14:16 ` Dave Martin 0 siblings, 2 replies; 25+ messages in thread From: Fabrice GASNIER @ 2014-02-07 16:19 UTC (permalink / raw) To: linux-arm-kernel This patch adds imprecise abort enable/disable macros. It also enables imprecise aborts when starting kernel. Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> --- arch/arm/include/asm/irqflags.h | 33 +++++++++++++++++++++++++++++++++ arch/arm/kernel/smp.c | 1 + arch/arm/kernel/traps.c | 4 ++++ 3 files changed, 38 insertions(+) diff --git a/arch/arm/include/asm/irqflags.h b/arch/arm/include/asm/irqflags.h index 3b763d6..82e3834 100644 --- a/arch/arm/include/asm/irqflags.h +++ b/arch/arm/include/asm/irqflags.h @@ -51,6 +51,9 @@ static inline void arch_local_irq_disable(void) #define local_fiq_enable() __asm__("cpsie f @ __stf" : : : "memory", "cc") #define local_fiq_disable() __asm__("cpsid f @ __clf" : : : "memory", "cc") + +#define local_abt_enable() __asm__("cpsie a @ __sta" : : : "memory", "cc") +#define local_abt_disable() __asm__("cpsid a @ __cla" : : : "memory", "cc") #else /* @@ -130,6 +133,36 @@ static inline void arch_local_irq_disable(void) : "memory", "cc"); \ }) +/* + * Enable Aborts + */ +#define local_abt_enable() \ + ({ \ + unsigned long temp; \ + __asm__ __volatile__( \ + "mrs %0, cpsr @ sta\n" \ +" bic %0, %0, %1\n" \ +" msr cpsr_c, %0" \ + : "=r" (temp) \ + : "r" (PSR_A_BIT) \ + : "memory", "cc"); \ + }) + +/* + * Disable Aborts + */ +#define local_abt_disable() \ + ({ \ + unsigned long temp; \ + __asm__ __volatile__( \ + "mrs %0, cpsr @ cla\n" \ +" orr %0, %0, %1\n" \ +" msr cpsr_c, %0" \ + : "=r" (temp) \ + : "r" (PSR_A_BIT) \ + : "memory", "cc"); \ + }) + #endif /* diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index dc894ab..c2093cb 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -377,6 +377,7 @@ asmlinkage void secondary_start_kernel(void) local_irq_enable(); local_fiq_enable(); + local_abt_enable(); /* * OK, it's off to the idle thread for us diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 4636d56..ef15709 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) flush_icache_range(vectors, vectors + PAGE_SIZE * 2); modify_domain(DOMAIN_USER, DOMAIN_CLIENT); + + /* Enable imprecise aborts */ + local_abt_enable(); + #else /* ifndef CONFIG_CPU_V7M */ /* * on V7-M there is no need to copy the vector table to a dedicated -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-07 16:19 ` [RFC PATCH] ARM: Add imprecise abort enable/disable macro Fabrice GASNIER @ 2014-02-07 17:09 ` Will Deacon 2014-02-10 8:50 ` Fabrice Gasnier 2014-02-10 13:58 ` Russell King - ARM Linux 2014-02-10 14:16 ` Dave Martin 1 sibling, 2 replies; 25+ messages in thread From: Will Deacon @ 2014-02-07 17:09 UTC (permalink / raw) To: linux-arm-kernel On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: > This patch adds imprecise abort enable/disable macros. > It also enables imprecise aborts when starting kernel. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > --- > arch/arm/include/asm/irqflags.h | 33 +++++++++++++++++++++++++++++++++ > arch/arm/kernel/smp.c | 1 + > arch/arm/kernel/traps.c | 4 ++++ > 3 files changed, 38 insertions(+) > > diff --git a/arch/arm/include/asm/irqflags.h b/arch/arm/include/asm/irqflags.h > index 3b763d6..82e3834 100644 > --- a/arch/arm/include/asm/irqflags.h > +++ b/arch/arm/include/asm/irqflags.h > @@ -51,6 +51,9 @@ static inline void arch_local_irq_disable(void) > > #define local_fiq_enable() __asm__("cpsie f @ __stf" : : : "memory", "cc") > #define local_fiq_disable() __asm__("cpsid f @ __clf" : : : "memory", "cc") > + > +#define local_abt_enable() __asm__("cpsie a @ __sta" : : : "memory", "cc") > +#define local_abt_disable() __asm__("cpsid a @ __cla" : : : "memory", "cc") > #else > > /* > @@ -130,6 +133,36 @@ static inline void arch_local_irq_disable(void) > : "memory", "cc"); \ > }) > > +/* > + * Enable Aborts > + */ > +#define local_abt_enable() \ > + ({ \ > + unsigned long temp; \ > + __asm__ __volatile__( \ > + "mrs %0, cpsr @ sta\n" \ > +" bic %0, %0, %1\n" \ > +" msr cpsr_c, %0" \ > + : "=r" (temp) \ > + : "r" (PSR_A_BIT) \ Can you use "i" instead of a register for this constant? > + : "memory", "cc"); \ You don't need the "cc" clobber. > + }) > + > +/* > + * Disable Aborts > + */ > +#define local_abt_disable() \ > + ({ \ > + unsigned long temp; \ > + __asm__ __volatile__( \ > + "mrs %0, cpsr @ cla\n" \ > +" orr %0, %0, %1\n" \ > +" msr cpsr_c, %0" \ > + : "=r" (temp) \ > + : "r" (PSR_A_BIT) \ > + : "memory", "cc"); \ > + }) Same comments here. > #endif > > /* > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c > index dc894ab..c2093cb 100644 > --- a/arch/arm/kernel/smp.c > +++ b/arch/arm/kernel/smp.c > @@ -377,6 +377,7 @@ asmlinkage void secondary_start_kernel(void) > > local_irq_enable(); > local_fiq_enable(); > + local_abt_enable(); > > /* > * OK, it's off to the idle thread for us > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 4636d56..ef15709 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) > > flush_icache_range(vectors, vectors + PAGE_SIZE * 2); > modify_domain(DOMAIN_USER, DOMAIN_CLIENT); > + > + /* Enable imprecise aborts */ > + local_abt_enable(); Surely we want to enable this as early as possible? Now, putting this into head.S is ugly, as it duplicating it across all the proc*.S files, so why not setup_arch? Will ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-07 17:09 ` Will Deacon @ 2014-02-10 8:50 ` Fabrice Gasnier 2014-02-10 9:00 ` Ben Dooks 2014-02-10 11:17 ` Will Deacon 2014-02-10 13:58 ` Russell King - ARM Linux 1 sibling, 2 replies; 25+ messages in thread From: Fabrice Gasnier @ 2014-02-10 8:50 UTC (permalink / raw) To: linux-arm-kernel On 02/07/2014 06:09 PM, Will Deacon wrote: > On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: >> This patch adds imprecise abort enable/disable macros. >> It also enables imprecise aborts when starting kernel. >> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >> --- >> arch/arm/include/asm/irqflags.h | 33 +++++++++++++++++++++++++++++++++ >> arch/arm/kernel/smp.c | 1 + >> arch/arm/kernel/traps.c | 4 ++++ >> 3 files changed, 38 insertions(+) >> >> diff --git a/arch/arm/include/asm/irqflags.h b/arch/arm/include/asm/irqflags.h >> index 3b763d6..82e3834 100644 >> --- a/arch/arm/include/asm/irqflags.h >> +++ b/arch/arm/include/asm/irqflags.h >> @@ -51,6 +51,9 @@ static inline void arch_local_irq_disable(void) >> >> #define local_fiq_enable() __asm__("cpsie f @ __stf" : : : "memory", "cc") >> #define local_fiq_disable() __asm__("cpsid f @ __clf" : : : "memory", "cc") >> + >> +#define local_abt_enable() __asm__("cpsie a @ __sta" : : : "memory", "cc") >> +#define local_abt_disable() __asm__("cpsid a @ __cla" : : : "memory", "cc") >> #else >> >> /* >> @@ -130,6 +133,36 @@ static inline void arch_local_irq_disable(void) >> : "memory", "cc"); \ >> }) >> >> +/* >> + * Enable Aborts >> + */ >> +#define local_abt_enable() \ >> + ({ \ >> + unsigned long temp; \ >> + __asm__ __volatile__( \ >> + "mrs %0, cpsr @ sta\n" \ >> +" bic %0, %0, %1\n" \ >> +" msr cpsr_c, %0" \ >> + : "=r" (temp) \ >> + : "r" (PSR_A_BIT) \ > Can you use "i" instead of a register for this constant? Hi, Sure, I will change it in a future patch. > >> + : "memory", "cc"); \ > You don't need the "cc" clobber. That surprises me: I think "orr" and "bic" instruction might change N and Z bits, depending on the result. So shouldn't "cc" be placed here ? I also see that it is used in local_fiq_enable/disable macros just above, that are similar: #define local_fiq_enable() \ ({ \ unsigned long temp; \ __asm__ __volatile__( \ "mrs %0, cpsr @ stf\n" \ " bic %0, %0, #64\n" \ " msr cpsr_c, %0" \ : "=r" (temp) \ : \ : "memory", "cc"); \ }) >> #endif >> >> /* >> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c >> index dc894ab..c2093cb 100644 >> --- a/arch/arm/kernel/smp.c >> +++ b/arch/arm/kernel/smp.c >> @@ -377,6 +377,7 @@ asmlinkage void secondary_start_kernel(void) >> >> local_irq_enable(); >> local_fiq_enable(); >> + local_abt_enable(); >> >> /* >> * OK, it's off to the idle thread for us >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >> index 4636d56..ef15709 100644 >> --- a/arch/arm/kernel/traps.c >> +++ b/arch/arm/kernel/traps.c >> @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) >> >> flush_icache_range(vectors, vectors + PAGE_SIZE * 2); >> modify_domain(DOMAIN_USER, DOMAIN_CLIENT); >> + >> + /* Enable imprecise aborts */ >> + local_abt_enable(); > Surely we want to enable this as early as possible? Now, putting this into > head.S is ugly, as it duplicating it across all the proc*.S files, so why > not setup_arch? Sorry, I'm not sure to understand your last comment. At least, I need it enabled before probing drivers (PCIe bus) I've added imprecise abort enable code in traps.c, following Russel King's advice, please see: http://archive.arm.linux.org.uk/lurker/message/20140131.170827.d752a1cc.en.html As abort bit is local to a cpu, i've also added it in smp.c, but this may not be the right place ? Please elaborate, Thanks, Fabrice > > Will ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 8:50 ` Fabrice Gasnier @ 2014-02-10 9:00 ` Ben Dooks 2014-02-10 13:32 ` Fabrice Gasnier 2014-02-10 11:17 ` Will Deacon 1 sibling, 1 reply; 25+ messages in thread From: Ben Dooks @ 2014-02-10 9:00 UTC (permalink / raw) To: linux-arm-kernel On 10/02/14 08:50, Fabrice Gasnier wrote: > On 02/07/2014 06:09 PM, Will Deacon wrote: >> On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: >>> This patch adds imprecise abort enable/disable macros. >>> It also enables imprecise aborts when starting kernel. >>> >>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >>> --- >>> arch/arm/include/asm/irqflags.h | 33 >>> +++++++++++++++++++++++++++++++++ >>> arch/arm/kernel/smp.c | 1 + >>> arch/arm/kernel/traps.c | 4 ++++ >>> 3 files changed, 38 insertions(+) >>> >>> diff --git a/arch/arm/include/asm/irqflags.h >>> b/arch/arm/include/asm/irqflags.h >>> index 3b763d6..82e3834 100644 >>> --- a/arch/arm/include/asm/irqflags.h >>> +++ b/arch/arm/include/asm/irqflags.h >>> @@ -51,6 +51,9 @@ static inline void arch_local_irq_disable(void) >>> #define local_fiq_enable() __asm__("cpsie f @ __stf" : : : >>> "memory", "cc") >>> #define local_fiq_disable() __asm__("cpsid f @ __clf" : : : >>> "memory", "cc") >>> + >>> +#define local_abt_enable() __asm__("cpsie a @ __sta" : : : >>> "memory", "cc") >>> +#define local_abt_disable() __asm__("cpsid a @ __cla" : : : >>> "memory", "cc") >>> #else >>> /* >>> @@ -130,6 +133,36 @@ static inline void arch_local_irq_disable(void) >>> : "memory", "cc"); \ >>> }) >>> +/* >>> + * Enable Aborts >>> + */ >>> +#define local_abt_enable() \ >>> + ({ \ >>> + unsigned long temp; \ >>> + __asm__ __volatile__( \ >>> + "mrs %0, cpsr @ sta\n" \ >>> +" bic %0, %0, %1\n" \ >>> +" msr cpsr_c, %0" \ >>> + : "=r" (temp) \ >>> + : "r" (PSR_A_BIT) \ >> Can you use "i" instead of a register for this constant? > Hi, > > Sure, I will change it in a future patch. >> >>> + : "memory", "cc"); \ >> You don't need the "cc" clobber. > That surprises me: I think "orr" and "bic" instruction might change N > and Z bits, depending on the result. > So shouldn't "cc" be placed here ? > I also see that it is used in local_fiq_enable/disable macros just > above, that are similar: No, only if they have the S flag set on the instruction (ORRS,BICS) -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 9:00 ` Ben Dooks @ 2014-02-10 13:32 ` Fabrice Gasnier 0 siblings, 0 replies; 25+ messages in thread From: Fabrice Gasnier @ 2014-02-10 13:32 UTC (permalink / raw) To: linux-arm-kernel On 02/10/2014 10:00 AM, Ben Dooks wrote: > On 10/02/14 08:50, Fabrice Gasnier wrote: >> On 02/07/2014 06:09 PM, Will Deacon wrote: >>> On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: >>>> This patch adds imprecise abort enable/disable macros. >>>> It also enables imprecise aborts when starting kernel. >>>> >>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >>>> --- >>>> arch/arm/include/asm/irqflags.h | 33 >>>> +++++++++++++++++++++++++++++++++ >>>> arch/arm/kernel/smp.c | 1 + >>>> arch/arm/kernel/traps.c | 4 ++++ >>>> 3 files changed, 38 insertions(+) >>>> >>>> diff --git a/arch/arm/include/asm/irqflags.h >>>> b/arch/arm/include/asm/irqflags.h >>>> index 3b763d6..82e3834 100644 >>>> --- a/arch/arm/include/asm/irqflags.h >>>> +++ b/arch/arm/include/asm/irqflags.h >>>> @@ -51,6 +51,9 @@ static inline void arch_local_irq_disable(void) >>>> #define local_fiq_enable() __asm__("cpsie f @ __stf" : : : >>>> "memory", "cc") >>>> #define local_fiq_disable() __asm__("cpsid f @ __clf" : : : >>>> "memory", "cc") >>>> + >>>> +#define local_abt_enable() __asm__("cpsie a @ __sta" : : : >>>> "memory", "cc") >>>> +#define local_abt_disable() __asm__("cpsid a @ __cla" : : : >>>> "memory", "cc") >>>> #else >>>> /* >>>> @@ -130,6 +133,36 @@ static inline void arch_local_irq_disable(void) >>>> : "memory", "cc"); \ >>>> }) >>>> +/* >>>> + * Enable Aborts >>>> + */ >>>> +#define local_abt_enable() \ >>>> + ({ \ >>>> + unsigned long temp; \ >>>> + __asm__ __volatile__( \ >>>> + "mrs %0, cpsr @ sta\n" \ >>>> +" bic %0, %0, %1\n" \ >>>> +" msr cpsr_c, %0" \ >>>> + : "=r" (temp) \ >>>> + : "r" (PSR_A_BIT) \ >>> Can you use "i" instead of a register for this constant? >> Hi, >> >> Sure, I will change it in a future patch. >>> >>>> + : "memory", "cc"); \ >>> You don't need the "cc" clobber. >> That surprises me: I think "orr" and "bic" instruction might change N >> and Z bits, depending on the result. >> So shouldn't "cc" be placed here ? >> I also see that it is used in local_fiq_enable/disable macros just >> above, that are similar: > > No, only if they have the S flag set on the instruction (ORRS,BICS) > > Thank you for pointing that out! ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 8:50 ` Fabrice Gasnier 2014-02-10 9:00 ` Ben Dooks @ 2014-02-10 11:17 ` Will Deacon 2014-02-10 13:54 ` Fabrice Gasnier 2014-02-10 13:56 ` Russell King - ARM Linux 1 sibling, 2 replies; 25+ messages in thread From: Will Deacon @ 2014-02-10 11:17 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 08:50:16AM +0000, Fabrice Gasnier wrote: > On 02/07/2014 06:09 PM, Will Deacon wrote: > > On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > >> index 4636d56..ef15709 100644 > >> --- a/arch/arm/kernel/traps.c > >> +++ b/arch/arm/kernel/traps.c > >> @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) > >> > >> flush_icache_range(vectors, vectors + PAGE_SIZE * 2); > >> modify_domain(DOMAIN_USER, DOMAIN_CLIENT); > >> + > >> + /* Enable imprecise aborts */ > >> + local_abt_enable(); > > Surely we want to enable this as early as possible? Now, putting this into > > head.S is ugly, as it duplicating it across all the proc*.S files, so why > > not setup_arch? > Sorry, I'm not sure to understand your last comment. > At least, I need it enabled before probing drivers (PCIe bus) > I've added imprecise abort enable code in traps.c, following Russel > King's advice, please see: > http://archive.arm.linux.org.uk/lurker/message/20140131.170827.d752a1cc.en.html > As abort bit is local to a cpu, i've also added it in smp.c, but this > may not be the right place ? > > Please elaborate, I was just suggesting that we move your local_abt_enable() call to setup_arch, since that's called before early_trap_init on the primary CPU. Will ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 11:17 ` Will Deacon @ 2014-02-10 13:54 ` Fabrice Gasnier 2014-02-10 13:56 ` Russell King - ARM Linux 1 sibling, 0 replies; 25+ messages in thread From: Fabrice Gasnier @ 2014-02-10 13:54 UTC (permalink / raw) To: linux-arm-kernel On 02/10/2014 12:17 PM, Will Deacon wrote: > On Mon, Feb 10, 2014 at 08:50:16AM +0000, Fabrice Gasnier wrote: >> On 02/07/2014 06:09 PM, Will Deacon wrote: >>> On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: >>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >>>> index 4636d56..ef15709 100644 >>>> --- a/arch/arm/kernel/traps.c >>>> +++ b/arch/arm/kernel/traps.c >>>> @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) >>>> >>>> flush_icache_range(vectors, vectors + PAGE_SIZE * 2); >>>> modify_domain(DOMAIN_USER, DOMAIN_CLIENT); >>>> + >>>> + /* Enable imprecise aborts */ >>>> + local_abt_enable(); >>> Surely we want to enable this as early as possible? Now, putting this into >>> head.S is ugly, as it duplicating it across all the proc*.S files, so why >>> not setup_arch? >> Sorry, I'm not sure to understand your last comment. >> At least, I need it enabled before probing drivers (PCIe bus) >> I've added imprecise abort enable code in traps.c, following Russel >> King's advice, please see: >> http://archive.arm.linux.org.uk/lurker/message/20140131.170827.d752a1cc.en.html >> As abort bit is local to a cpu, i've also added it in smp.c, but this >> may not be the right place ? >> >> Please elaborate, > I was just suggesting that we move your local_abt_enable() call to > setup_arch, since that's called before early_trap_init on the primary CPU. Thanks for the clarification. Yes, maybe others would like to have it enabled as you suggest. My point is, it's good enough for such use case to do it in early_trap_init. I'd prefer to follow Russel's first advice. Fabrice > > Will ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 11:17 ` Will Deacon 2014-02-10 13:54 ` Fabrice Gasnier @ 2014-02-10 13:56 ` Russell King - ARM Linux 2014-02-10 14:12 ` Will Deacon 2014-02-10 14:42 ` Dave Martin 1 sibling, 2 replies; 25+ messages in thread From: Russell King - ARM Linux @ 2014-02-10 13:56 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 11:17:10AM +0000, Will Deacon wrote: > On Mon, Feb 10, 2014 at 08:50:16AM +0000, Fabrice Gasnier wrote: > > On 02/07/2014 06:09 PM, Will Deacon wrote: > > > On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > >> index 4636d56..ef15709 100644 > > >> --- a/arch/arm/kernel/traps.c > > >> +++ b/arch/arm/kernel/traps.c > > >> @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) > > >> > > >> flush_icache_range(vectors, vectors + PAGE_SIZE * 2); > > >> modify_domain(DOMAIN_USER, DOMAIN_CLIENT); > > >> + > > >> + /* Enable imprecise aborts */ > > >> + local_abt_enable(); > > > Surely we want to enable this as early as possible? Now, putting this into > > > head.S is ugly, as it duplicating it across all the proc*.S files, so why > > > not setup_arch? > > Sorry, I'm not sure to understand your last comment. > > At least, I need it enabled before probing drivers (PCIe bus) > > I've added imprecise abort enable code in traps.c, following Russel > > King's advice, please see: > > http://archive.arm.linux.org.uk/lurker/message/20140131.170827.d752a1cc.en.html > > As abort bit is local to a cpu, i've also added it in smp.c, but this > > may not be the right place ? > > > > Please elaborate, > > I was just suggesting that we move your local_abt_enable() call to > setup_arch, since that's called before early_trap_init on the primary CPU. Why would we want to enable aborts before we've setup the vectors page to handle an abort? That's akin to enabling interrupts and hoping there isn't one pending... -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit". ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 13:56 ` Russell King - ARM Linux @ 2014-02-10 14:12 ` Will Deacon 2014-02-10 14:42 ` Dave Martin 1 sibling, 0 replies; 25+ messages in thread From: Will Deacon @ 2014-02-10 14:12 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 01:56:59PM +0000, Russell King - ARM Linux wrote: > On Mon, Feb 10, 2014 at 11:17:10AM +0000, Will Deacon wrote: > > On Mon, Feb 10, 2014 at 08:50:16AM +0000, Fabrice Gasnier wrote: > > > On 02/07/2014 06:09 PM, Will Deacon wrote: > > > > On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: > > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > > >> index 4636d56..ef15709 100644 > > > >> --- a/arch/arm/kernel/traps.c > > > >> +++ b/arch/arm/kernel/traps.c > > > >> @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) > > > >> > > > >> flush_icache_range(vectors, vectors + PAGE_SIZE * 2); > > > >> modify_domain(DOMAIN_USER, DOMAIN_CLIENT); > > > >> + > > > >> + /* Enable imprecise aborts */ > > > >> + local_abt_enable(); > > > > Surely we want to enable this as early as possible? Now, putting this into > > > > head.S is ugly, as it duplicating it across all the proc*.S files, so why > > > > not setup_arch? > > > Sorry, I'm not sure to understand your last comment. > > > At least, I need it enabled before probing drivers (PCIe bus) > > > I've added imprecise abort enable code in traps.c, following Russel > > > King's advice, please see: > > > http://archive.arm.linux.org.uk/lurker/message/20140131.170827.d752a1cc.en.html > > > As abort bit is local to a cpu, i've also added it in smp.c, but this > > > may not be the right place ? > > > > > > Please elaborate, > > > > I was just suggesting that we move your local_abt_enable() call to > > setup_arch, since that's called before early_trap_init on the primary CPU. > > Why would we want to enable aborts before we've setup the vectors page > to handle an abort? That's akin to enabling interrupts and hoping there > isn't one pending... I figured we'd want to fall over as quickly as possible if the bootloader had left a pending exception for us, but you're right in that it's not very helpful if we can't print out a diagnostic. Fabrice, please ignore my suggestion and keep the unmasking where it is. Will ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 13:56 ` Russell King - ARM Linux 2014-02-10 14:12 ` Will Deacon @ 2014-02-10 14:42 ` Dave Martin 2014-02-10 15:19 ` Russell King - ARM Linux 1 sibling, 1 reply; 25+ messages in thread From: Dave Martin @ 2014-02-10 14:42 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 01:56:59PM +0000, Russell King - ARM Linux wrote: > On Mon, Feb 10, 2014 at 11:17:10AM +0000, Will Deacon wrote: > > On Mon, Feb 10, 2014 at 08:50:16AM +0000, Fabrice Gasnier wrote: > > > On 02/07/2014 06:09 PM, Will Deacon wrote: > > > > On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: > > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > > > >> index 4636d56..ef15709 100644 > > > >> --- a/arch/arm/kernel/traps.c > > > >> +++ b/arch/arm/kernel/traps.c > > > >> @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) > > > >> > > > >> flush_icache_range(vectors, vectors + PAGE_SIZE * 2); > > > >> modify_domain(DOMAIN_USER, DOMAIN_CLIENT); > > > >> + > > > >> + /* Enable imprecise aborts */ > > > >> + local_abt_enable(); > > > > Surely we want to enable this as early as possible? Now, putting this into > > > > head.S is ugly, as it duplicating it across all the proc*.S files, so why > > > > not setup_arch? > > > Sorry, I'm not sure to understand your last comment. > > > At least, I need it enabled before probing drivers (PCIe bus) > > > I've added imprecise abort enable code in traps.c, following Russel > > > King's advice, please see: > > > http://archive.arm.linux.org.uk/lurker/message/20140131.170827.d752a1cc.en.html > > > As abort bit is local to a cpu, i've also added it in smp.c, but this > > > may not be the right place ? > > > > > > Please elaborate, > > > > I was just suggesting that we move your local_abt_enable() call to > > setup_arch, since that's called before early_trap_init on the primary CPU. > > Why would we want to enable aborts before we've setup the vectors page > to handle an abort? That's akin to enabling interrupts and hoping there > isn't one pending... Should we require CPSR.A to me masked in Booting, for all CPUs that have it? By definition, we cannot dispatch those exceptions for a while, until some vectors have been set up; so it makes sense in the same way that requiring CPSR.[IF] to be set makes sense. The kernel should do its best to cope anyway, and immediately mask CPSR.A on entry if it's a v6 or later kernel. safe_svcmode_maskall was originally intended to do that, but it got watered down to cope with the Zaurus issues, so it now looks like most code paths don't mask CPSR.A there. I'm happy to take another look at it. Cheers ---Dave ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 14:42 ` Dave Martin @ 2014-02-10 15:19 ` Russell King - ARM Linux 2014-02-10 16:28 ` Dave Martin 0 siblings, 1 reply; 25+ messages in thread From: Russell King - ARM Linux @ 2014-02-10 15:19 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 02:42:28PM +0000, Dave Martin wrote: > Should we require CPSR.A to me masked in Booting, for all CPUs that have > it? If it's not masked at boot, then there can't be an imprecise exception pending. That's unlike interrupts, where a device could trigger an interrupt at any moment (eg, a timer expiring.) -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit". ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 15:19 ` Russell King - ARM Linux @ 2014-02-10 16:28 ` Dave Martin 2014-02-10 16:37 ` Russell King - ARM Linux 0 siblings, 1 reply; 25+ messages in thread From: Dave Martin @ 2014-02-10 16:28 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 03:19:34PM +0000, Russell King - ARM Linux wrote: > On Mon, Feb 10, 2014 at 02:42:28PM +0000, Dave Martin wrote: > > Should we require CPSR.A to me masked in Booting, for all CPUs that have > > it? > > If it's not masked at boot, then there can't be an imprecise exception > pending. Couldn't there still be a dangling abort condition triggered by the bootloader, which which doesn't raise the abort pin until after we entered the kernel? > That's unlike interrupts, where a device could trigger an interrupt at > any moment (eg, a timer expiring.) List as with interrupts, there's no way to drain or cancel pending aborts that aren't asserted yet, but whose cause conditions are already established. It's possible that Strongly-Ordered memory is sufficient to guarantee that any D-side abort becomes synchronous in some implementations but I don't think the arhitecture guarantees it. It certainly won't be guaranteed for any other memory type. For these reasons, imprecise aborts seem a lot like interrupts: you can mask them or handle them; but controlling when and whether they occur involves platform-specific assumptions, at least in theory. Cheers ---Dave ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 16:28 ` Dave Martin @ 2014-02-10 16:37 ` Russell King - ARM Linux 2014-02-10 17:28 ` Dave Martin 0 siblings, 1 reply; 25+ messages in thread From: Russell King - ARM Linux @ 2014-02-10 16:37 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 04:28:22PM +0000, Dave Martin wrote: > On Mon, Feb 10, 2014 at 03:19:34PM +0000, Russell King - ARM Linux wrote: > > On Mon, Feb 10, 2014 at 02:42:28PM +0000, Dave Martin wrote: > > > Should we require CPSR.A to me masked in Booting, for all CPUs that have > > > it? > > > > If it's not masked at boot, then there can't be an imprecise exception > > pending. > > Couldn't there still be a dangling abort condition triggered by the > bootloader, which which doesn't raise the abort pin until after we > entered the kernel? True, but the decompressor does disable them (see safe_svcmode_maskall), so any raised abort is likely to hit the boot loader's vectors at that time. They remain masked into the kernel from that point. If you're not using the decompressor then the A bit will be left as-is. Given that we've not yet had any failures, I'm inclined to just let the status-quo be for the kernel entry - if it does cause problems then it's clear that the right solution is that the A bit must be disabled. -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit". ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 16:37 ` Russell King - ARM Linux @ 2014-02-10 17:28 ` Dave Martin 0 siblings, 0 replies; 25+ messages in thread From: Dave Martin @ 2014-02-10 17:28 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 04:37:00PM +0000, Russell King - ARM Linux wrote: > On Mon, Feb 10, 2014 at 04:28:22PM +0000, Dave Martin wrote: > > On Mon, Feb 10, 2014 at 03:19:34PM +0000, Russell King - ARM Linux wrote: > > > On Mon, Feb 10, 2014 at 02:42:28PM +0000, Dave Martin wrote: > > > > Should we require CPSR.A to me masked in Booting, for all CPUs that have > > > > it? > > > > > > If it's not masked at boot, then there can't be an imprecise exception > > > pending. > > > > Couldn't there still be a dangling abort condition triggered by the > > bootloader, which which doesn't raise the abort pin until after we > > entered the kernel? > > True, but the decompressor does disable them (see safe_svcmode_maskall), > so any raised abort is likely to hit the boot loader's vectors at that > time. They remain masked into the kernel from that point. > > If you're not using the decompressor then the A bit will be left as-is. > > Given that we've not yet had any failures, I'm inclined to just let the > status-quo be for the kernel entry - if it does cause problems then it's > clear that the right solution is that the A bit must be disabled. OK, that seems a reasonable view. Cheers ---Dave ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-07 17:09 ` Will Deacon 2014-02-10 8:50 ` Fabrice Gasnier @ 2014-02-10 13:58 ` Russell King - ARM Linux 1 sibling, 0 replies; 25+ messages in thread From: Russell King - ARM Linux @ 2014-02-10 13:58 UTC (permalink / raw) To: linux-arm-kernel On Fri, Feb 07, 2014 at 05:09:03PM +0000, Will Deacon wrote: > On Fri, Feb 07, 2014 at 04:19:15PM +0000, Fabrice GASNIER wrote: > > +#define local_abt_enable() \ > > + ({ \ > > + unsigned long temp; \ > > + __asm__ __volatile__( \ > > + "mrs %0, cpsr @ sta\n" \ > > +" bic %0, %0, %1\n" \ > > +" msr cpsr_c, %0" \ > > + : "=r" (temp) \ > > + : "r" (PSR_A_BIT) \ > > Can you use "i" instead of a register for this constant? As the PSR A bit isn't in bits 7-0, cpsr_c isn't what's required here. It needs something different... that's why my previous patch for you didn't work. -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit". ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-07 16:19 ` [RFC PATCH] ARM: Add imprecise abort enable/disable macro Fabrice GASNIER 2014-02-07 17:09 ` Will Deacon @ 2014-02-10 14:16 ` Dave Martin 2014-02-10 14:44 ` Fabrice Gasnier 2014-02-10 14:54 ` Ben Dooks 1 sibling, 2 replies; 25+ messages in thread From: Dave Martin @ 2014-02-10 14:16 UTC (permalink / raw) To: linux-arm-kernel On Fri, Feb 07, 2014 at 05:19:15PM +0100, Fabrice GASNIER wrote: > This patch adds imprecise abort enable/disable macros. > It also enables imprecise aborts when starting kernel. Relying on imprecise aborts for hardware probing would be considered bad hardware and/or software design for ARM-specific stuff. PCI is more generic though, so we may have to put up with this to some extent. Can you point me to the affected probing code? I'm not very familiar with that stuff... > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > --- > arch/arm/include/asm/irqflags.h | 33 +++++++++++++++++++++++++++++++++ > arch/arm/kernel/smp.c | 1 + > arch/arm/kernel/traps.c | 4 ++++ > 3 files changed, 38 insertions(+) > > diff --git a/arch/arm/include/asm/irqflags.h b/arch/arm/include/asm/irqflags.h > index 3b763d6..82e3834 100644 > --- a/arch/arm/include/asm/irqflags.h > +++ b/arch/arm/include/asm/irqflags.h > @@ -51,6 +51,9 @@ static inline void arch_local_irq_disable(void) > > #define local_fiq_enable() __asm__("cpsie f @ __stf" : : : "memory", "cc") > #define local_fiq_disable() __asm__("cpsid f @ __clf" : : : "memory", "cc") > + > +#define local_abt_enable() __asm__("cpsie a @ __sta" : : : "memory", "cc") > +#define local_abt_disable() __asm__("cpsid a @ __cla" : : : "memory", "cc") > #else > > /* > @@ -130,6 +133,36 @@ static inline void arch_local_irq_disable(void) > : "memory", "cc"); \ > }) > > +/* > + * Enable Aborts > + */ > +#define local_abt_enable() \ > + ({ \ > + unsigned long temp; \ > + __asm__ __volatile__( \ > + "mrs %0, cpsr @ sta\n" \ > +" bic %0, %0, %1\n" \ > +" msr cpsr_c, %0" \ I suggest you use "cpsie/cpsid a" instead. This requires ARMv6, but the CPSR.A bit only exists on ARMv6 and later anyway. Poking that bit on earlier CPUs may cause unpredictable behaviour, so these macros should be no-ops for v5 and earlier. > + : "=r" (temp) \ > + : "r" (PSR_A_BIT) \ > + : "memory", "cc"); \ > + }) > + > +/* > + * Disable Aborts > + */ > +#define local_abt_disable() \ > + ({ \ > + unsigned long temp; \ > + __asm__ __volatile__( \ > + "mrs %0, cpsr @ cla\n" \ > +" orr %0, %0, %1\n" \ > +" msr cpsr_c, %0" \ > + : "=r" (temp) \ > + : "r" (PSR_A_BIT) \ > + : "memory", "cc"); \ > + }) > + > #endif > > /* > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c > index dc894ab..c2093cb 100644 > --- a/arch/arm/kernel/smp.c > +++ b/arch/arm/kernel/smp.c > @@ -377,6 +377,7 @@ asmlinkage void secondary_start_kernel(void) > > local_irq_enable(); > local_fiq_enable(); > + local_abt_enable(); > > /* > * OK, it's off to the idle thread for us > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 4636d56..ef15709 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) > > flush_icache_range(vectors, vectors + PAGE_SIZE * 2); > modify_domain(DOMAIN_USER, DOMAIN_CLIENT); > + > + /* Enable imprecise aborts */ > + local_abt_enable(); It would be good to clean up why aborts are not being consistently enabled on boot. Really, they should be enabled, except for a brief window during boot when the vectors are not mapped and the abort can't be dispatched. Cheers ---Dave ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 14:16 ` Dave Martin @ 2014-02-10 14:44 ` Fabrice Gasnier 2014-02-10 15:12 ` Dave Martin 2014-02-10 14:54 ` Ben Dooks 1 sibling, 1 reply; 25+ messages in thread From: Fabrice Gasnier @ 2014-02-10 14:44 UTC (permalink / raw) To: linux-arm-kernel On 02/10/2014 03:16 PM, Dave Martin wrote: > On Fri, Feb 07, 2014 at 05:19:15PM +0100, Fabrice GASNIER wrote: >> This patch adds imprecise abort enable/disable macros. >> It also enables imprecise aborts when starting kernel. > Relying on imprecise aborts for hardware probing would be considered bad > hardware and/or software design for ARM-specific stuff. > > PCI is more generic though, so we may have to put up with this to some > extent. Can you point me to the affected probing code? I'm not very > familiar with that stuff... Hi, I'm currently re-basing to prepare upstream of such a driver so, no code for now on my side, but, I saw others that have similar behavior : http://www.spinics.net/lists/linux-pci/msg26124.html Basically, I think all PCI drivers using hook_fault_code(16+6, ...) use similar mechanism. > >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >> --- >> arch/arm/include/asm/irqflags.h | 33 +++++++++++++++++++++++++++++++++ >> arch/arm/kernel/smp.c | 1 + >> arch/arm/kernel/traps.c | 4 ++++ >> 3 files changed, 38 insertions(+) >> >> diff --git a/arch/arm/include/asm/irqflags.h b/arch/arm/include/asm/irqflags.h >> index 3b763d6..82e3834 100644 >> --- a/arch/arm/include/asm/irqflags.h >> +++ b/arch/arm/include/asm/irqflags.h >> @@ -51,6 +51,9 @@ static inline void arch_local_irq_disable(void) >> >> #define local_fiq_enable() __asm__("cpsie f @ __stf" : : : "memory", "cc") >> #define local_fiq_disable() __asm__("cpsid f @ __clf" : : : "memory", "cc") >> + >> +#define local_abt_enable() __asm__("cpsie a @ __sta" : : : "memory", "cc") >> +#define local_abt_disable() __asm__("cpsid a @ __cla" : : : "memory", "cc") >> #else >> >> /* >> @@ -130,6 +133,36 @@ static inline void arch_local_irq_disable(void) >> : "memory", "cc"); \ >> }) >> >> +/* >> + * Enable Aborts >> + */ >> +#define local_abt_enable() \ >> + ({ \ >> + unsigned long temp; \ >> + __asm__ __volatile__( \ >> + "mrs %0, cpsr @ sta\n" \ >> +" bic %0, %0, %1\n" \ >> +" msr cpsr_c, %0" \ > I suggest you use "cpsie/cpsid a" instead. This requires ARMv6, but the > CPSR.A bit only exists on ARMv6 and later anyway. Poking that bit > on earlier CPUs may cause unpredictable behaviour, so these macros > should be no-ops for v5 and earlier. Thanks, I'll prepare a new patch that way. > >> + : "=r" (temp) \ >> + : "r" (PSR_A_BIT) \ >> + : "memory", "cc"); \ >> + }) >> + >> +/* >> + * Disable Aborts >> + */ >> +#define local_abt_disable() \ >> + ({ \ >> + unsigned long temp; \ >> + __asm__ __volatile__( \ >> + "mrs %0, cpsr @ cla\n" \ >> +" orr %0, %0, %1\n" \ >> +" msr cpsr_c, %0" \ >> + : "=r" (temp) \ >> + : "r" (PSR_A_BIT) \ >> + : "memory", "cc"); \ >> + }) >> + >> #endif >> >> /* >> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c >> index dc894ab..c2093cb 100644 >> --- a/arch/arm/kernel/smp.c >> +++ b/arch/arm/kernel/smp.c >> @@ -377,6 +377,7 @@ asmlinkage void secondary_start_kernel(void) >> >> local_irq_enable(); >> local_fiq_enable(); >> + local_abt_enable(); >> >> /* >> * OK, it's off to the idle thread for us >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >> index 4636d56..ef15709 100644 >> --- a/arch/arm/kernel/traps.c >> +++ b/arch/arm/kernel/traps.c >> @@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) >> >> flush_icache_range(vectors, vectors + PAGE_SIZE * 2); >> modify_domain(DOMAIN_USER, DOMAIN_CLIENT); >> + >> + /* Enable imprecise aborts */ >> + local_abt_enable(); > It would be good to clean up why aborts are not being consistently > enabled on boot. I did a bit of analysis and summarized it in this thread. I've not been further: http://archive.arm.linux.org.uk/lurker/message/20140203.164322.edba427a.en.html BR, Fabrice > > Really, they should be enabled, except for a brief window during > boot when the vectors are not mapped and the abort can't be dispatched. > > Cheers > ---Dave ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 14:44 ` Fabrice Gasnier @ 2014-02-10 15:12 ` Dave Martin 2014-02-10 15:24 ` Russell King - ARM Linux 0 siblings, 1 reply; 25+ messages in thread From: Dave Martin @ 2014-02-10 15:12 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 03:44:50PM +0100, Fabrice Gasnier wrote: > On 02/10/2014 03:16 PM, Dave Martin wrote: > >On Fri, Feb 07, 2014 at 05:19:15PM +0100, Fabrice GASNIER wrote: > >>This patch adds imprecise abort enable/disable macros. > >>It also enables imprecise aborts when starting kernel. > >Relying on imprecise aborts for hardware probing would be considered bad > >hardware and/or software design for ARM-specific stuff. > > > >PCI is more generic though, so we may have to put up with this to some > >extent. Can you point me to the affected probing code? I'm not very > >familiar with that stuff... > Hi, > > I'm currently re-basing to prepare upstream of such a driver so, no > code for now on my side, > but, I saw others that have similar behavior : > http://www.spinics.net/lists/linux-pci/msg26124.html > Basically, I think all PCI drivers using hook_fault_code(16+6, ...) > use similar mechanism. Hmmm. There seem to be a few problems here. Firstly, blindly adding 4 to PC is obviouly not right, partly because we might be running an unrelated thread by the time the abort fires, and also because the affected instruction might not be 4 bytes in size in a Thumb kernel. Secondly, do we ever release the fault code hook? If not, it looks like we just ignore all imprecise aborts from the moment that driver is loaded, whatever causes them ... not ideal. Are the aborts triggered by the PCI common code? Do you know which precise code triggers the aborts? I wonder whether it would be possible to arch hooks to do the problematic probing another way. Cheers ---Dave > > > >>Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > >>--- > >> arch/arm/include/asm/irqflags.h | 33 +++++++++++++++++++++++++++++++++ > >> arch/arm/kernel/smp.c | 1 + > >> arch/arm/kernel/traps.c | 4 ++++ > >> 3 files changed, 38 insertions(+) > >> > >>diff --git a/arch/arm/include/asm/irqflags.h b/arch/arm/include/asm/irqflags.h > >>index 3b763d6..82e3834 100644 > >>--- a/arch/arm/include/asm/irqflags.h > >>+++ b/arch/arm/include/asm/irqflags.h > >>@@ -51,6 +51,9 @@ static inline void arch_local_irq_disable(void) > >> #define local_fiq_enable() __asm__("cpsie f @ __stf" : : : "memory", "cc") > >> #define local_fiq_disable() __asm__("cpsid f @ __clf" : : : "memory", "cc") > >>+ > >>+#define local_abt_enable() __asm__("cpsie a @ __sta" : : : "memory", "cc") > >>+#define local_abt_disable() __asm__("cpsid a @ __cla" : : : "memory", "cc") > >> #else > >> /* > >>@@ -130,6 +133,36 @@ static inline void arch_local_irq_disable(void) > >> : "memory", "cc"); \ > >> }) > >>+/* > >>+ * Enable Aborts > >>+ */ > >>+#define local_abt_enable() \ > >>+ ({ \ > >>+ unsigned long temp; \ > >>+ __asm__ __volatile__( \ > >>+ "mrs %0, cpsr @ sta\n" \ > >>+" bic %0, %0, %1\n" \ > >>+" msr cpsr_c, %0" \ > >I suggest you use "cpsie/cpsid a" instead. This requires ARMv6, but the > >CPSR.A bit only exists on ARMv6 and later anyway. Poking that bit > >on earlier CPUs may cause unpredictable behaviour, so these macros > >should be no-ops for v5 and earlier. > Thanks, > I'll prepare a new patch that way. > > > >>+ : "=r" (temp) \ > >>+ : "r" (PSR_A_BIT) \ > >>+ : "memory", "cc"); \ > >>+ }) > >>+ > >>+/* > >>+ * Disable Aborts > >>+ */ > >>+#define local_abt_disable() \ > >>+ ({ \ > >>+ unsigned long temp; \ > >>+ __asm__ __volatile__( \ > >>+ "mrs %0, cpsr @ cla\n" \ > >>+" orr %0, %0, %1\n" \ > >>+" msr cpsr_c, %0" \ > >>+ : "=r" (temp) \ > >>+ : "r" (PSR_A_BIT) \ > >>+ : "memory", "cc"); \ > >>+ }) > >>+ > >> #endif > >> /* > >>diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c > >>index dc894ab..c2093cb 100644 > >>--- a/arch/arm/kernel/smp.c > >>+++ b/arch/arm/kernel/smp.c > >>@@ -377,6 +377,7 @@ asmlinkage void secondary_start_kernel(void) > >> local_irq_enable(); > >> local_fiq_enable(); > >>+ local_abt_enable(); > >> /* > >> * OK, it's off to the idle thread for us > >>diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > >>index 4636d56..ef15709 100644 > >>--- a/arch/arm/kernel/traps.c > >>+++ b/arch/arm/kernel/traps.c > >>@@ -900,6 +900,10 @@ void __init early_trap_init(void *vectors_base) > >> flush_icache_range(vectors, vectors + PAGE_SIZE * 2); > >> modify_domain(DOMAIN_USER, DOMAIN_CLIENT); > >>+ > >>+ /* Enable imprecise aborts */ > >>+ local_abt_enable(); > >It would be good to clean up why aborts are not being consistently > >enabled on boot. > I did a bit of analysis and summarized it in this thread. I've not > been further: > http://archive.arm.linux.org.uk/lurker/message/20140203.164322.edba427a.en.html > > BR, > Fabrice > > > >Really, they should be enabled, except for a brief window during > >boot when the vectors are not mapped and the abort can't be dispatched. > > > >Cheers > >---Dave > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 15:12 ` Dave Martin @ 2014-02-10 15:24 ` Russell King - ARM Linux 2014-02-10 16:36 ` Fabrice Gasnier 0 siblings, 1 reply; 25+ messages in thread From: Russell King - ARM Linux @ 2014-02-10 15:24 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 03:12:47PM +0000, Dave Martin wrote: > Firstly, blindly adding 4 to PC is obviouly not right, partly because we > might be running an unrelated thread by the time the abort fires, and > also because the affected instruction might not be 4 bytes in size in a > Thumb kernel. Exactly. We ended up on some platforms having special accessors for PCI where we included a number of 'mov r0, r0' instructions after the accessor so we could properly cope with them - but this required knowledge that we were going to only receive an imprecise abort from these accessors and only for a few cycles after the instruction. However, that's not true with modern architectures. The point they're received will _not_ be the load/store which resulted in the abort, and in the case of a write, they could be many hundreds of cycles later, especially if the write has been buffered. So adding four to the PC is definitely a very /bad/ thing to do. -- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit". ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 15:24 ` Russell King - ARM Linux @ 2014-02-10 16:36 ` Fabrice Gasnier 0 siblings, 0 replies; 25+ messages in thread From: Fabrice Gasnier @ 2014-02-10 16:36 UTC (permalink / raw) To: linux-arm-kernel On 02/10/2014 04:24 PM, Russell King - ARM Linux wrote: > On Mon, Feb 10, 2014 at 03:12:47PM +0000, Dave Martin wrote: >> Firstly, blindly adding 4 to PC is obviouly not right, partly because we >> might be running an unrelated thread by the time the abort fires, and >> also because the affected instruction might not be 4 bytes in size in a >> Thumb kernel. > Exactly. We ended up on some platforms having special accessors for PCI > where we included a number of 'mov r0, r0' instructions after the accessor > so we could properly cope with them - but this required knowledge that > we were going to only receive an imprecise abort from these accessors > and only for a few cycles after the instruction. > > However, that's not true with modern architectures. The point they're > received will _not_ be the load/store which resulted in the abort, and > in the case of a write, they could be many hundreds of cycles later, > especially if the write has been buffered. What about putting a memory barrier after a load/store ? CPU should wait for the operation to complete right ? > > So adding four to the PC is definitely a very /bad/ thing to do. > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 14:16 ` Dave Martin 2014-02-10 14:44 ` Fabrice Gasnier @ 2014-02-10 14:54 ` Ben Dooks 2014-02-10 15:21 ` Dave Martin 1 sibling, 1 reply; 25+ messages in thread From: Ben Dooks @ 2014-02-10 14:54 UTC (permalink / raw) To: linux-arm-kernel On 10/02/14 14:16, Dave Martin wrote: > On Fri, Feb 07, 2014 at 05:19:15PM +0100, Fabrice GASNIER wrote: >> This patch adds imprecise abort enable/disable macros. >> It also enables imprecise aborts when starting kernel. > > Relying on imprecise aborts for hardware probing would be considered bad > hardware and/or software design for ARM-specific stuff. > > PCI is more generic though, so we may have to put up with this to some > extent. Can you point me to the affected probing code? I'm not very > familiar with that stuff... The marvell pcie always had the option of delivering any bus errors as imprecise aborts. However it was /annoying/ and therefore easier just to turn it off and rely on the hardware returning 0xffff for any configuration area it couldn't get to. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 14:54 ` Ben Dooks @ 2014-02-10 15:21 ` Dave Martin 2014-02-10 16:38 ` Ben Dooks 0 siblings, 1 reply; 25+ messages in thread From: Dave Martin @ 2014-02-10 15:21 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 02:54:22PM +0000, Ben Dooks wrote: > On 10/02/14 14:16, Dave Martin wrote: > >On Fri, Feb 07, 2014 at 05:19:15PM +0100, Fabrice GASNIER wrote: > >>This patch adds imprecise abort enable/disable macros. > >>It also enables imprecise aborts when starting kernel. > > > >Relying on imprecise aborts for hardware probing would be considered bad > >hardware and/or software design for ARM-specific stuff. > > > >PCI is more generic though, so we may have to put up with this to some > >extent. Can you point me to the affected probing code? I'm not very > >familiar with that stuff... > > The marvell pcie always had the option of delivering any bus > errors as imprecise aborts. However it was /annoying/ and therefore You don't say ;) > easier just to turn it off and rely on the hardware returning 0xffff > for any configuration area it couldn't get to. Does PCI have any way of finding out which parts of the configuration space are there before you are forced to go poking around in invalid address space? I'm guessing there may not be, otherwise this convsersation might not be happening ... but I don't know too much about PCI. Maybe some driver-specific probing hook would make sense, so that we only need to use the russian roulette approach on hardware that forces us to. Cheers ---Dave ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 15:21 ` Dave Martin @ 2014-02-10 16:38 ` Ben Dooks 2014-02-11 15:38 ` Dave Martin 0 siblings, 1 reply; 25+ messages in thread From: Ben Dooks @ 2014-02-10 16:38 UTC (permalink / raw) To: linux-arm-kernel On 10/02/14 15:21, Dave Martin wrote: > On Mon, Feb 10, 2014 at 02:54:22PM +0000, Ben Dooks wrote: >> On 10/02/14 14:16, Dave Martin wrote: >>> On Fri, Feb 07, 2014 at 05:19:15PM +0100, Fabrice GASNIER wrote: >>>> This patch adds imprecise abort enable/disable macros. >>>> It also enables imprecise aborts when starting kernel. >>> >>> Relying on imprecise aborts for hardware probing would be considered bad >>> hardware and/or software design for ARM-specific stuff. >>> >>> PCI is more generic though, so we may have to put up with this to some >>> extent. Can you point me to the affected probing code? I'm not very >>> familiar with that stuff... >> >> The marvell pcie always had the option of delivering any bus >> errors as imprecise aborts. However it was /annoying/ and therefore > > You don't say ;) > >> easier just to turn it off and rely on the hardware returning 0xffff >> for any configuration area it couldn't get to. > > Does PCI have any way of finding out which parts of the configuration > space are there before you are forced to go poking around in invalid > address space? > > I'm guessing there may not be, otherwise this convsersation might not > be happening ... but I don't know too much about PCI. IIRC for configuration accesses you have to wait for the PCIe core to get a response from the other end. The systems I've seen either poll for completion or hold the transaction until the pcie core has finished working. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH] ARM: Add imprecise abort enable/disable macro 2014-02-10 16:38 ` Ben Dooks @ 2014-02-11 15:38 ` Dave Martin 0 siblings, 0 replies; 25+ messages in thread From: Dave Martin @ 2014-02-11 15:38 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 10, 2014 at 04:38:09PM +0000, Ben Dooks wrote: > On 10/02/14 15:21, Dave Martin wrote: [...] > >Does PCI have any way of finding out which parts of the configuration > >space are there before you are forced to go poking around in invalid > >address space? > > > >I'm guessing there may not be, otherwise this convsersation might not > >be happening ... but I don't know too much about PCI. > > IIRC for configuration accesses you have to wait for the PCIe core > to get a response from the other end. The systems I've seen either > poll for completion or hold the transaction until the pcie core has > finished working. So presumably if the other end isn't there, then it's up to the PCIe implementation to decide how to signal that back to the CPU, or are things more constrained than that? I sill don't understand whether the failing probe is triggered directly from the PCI common code in the kernel or whether it comes from the specific bus driver. If the latter, hacks for working round this at least won't touch the common code, which is the preferable outcome. Cheers ---Dave ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-02-11 15:38 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-07 16:19 [RFC PATCH] ARM: Enable imprecise external aborts earlier Fabrice GASNIER 2014-02-07 16:19 ` [RFC PATCH] ARM: Add imprecise abort enable/disable macro Fabrice GASNIER 2014-02-07 17:09 ` Will Deacon 2014-02-10 8:50 ` Fabrice Gasnier 2014-02-10 9:00 ` Ben Dooks 2014-02-10 13:32 ` Fabrice Gasnier 2014-02-10 11:17 ` Will Deacon 2014-02-10 13:54 ` Fabrice Gasnier 2014-02-10 13:56 ` Russell King - ARM Linux 2014-02-10 14:12 ` Will Deacon 2014-02-10 14:42 ` Dave Martin 2014-02-10 15:19 ` Russell King - ARM Linux 2014-02-10 16:28 ` Dave Martin 2014-02-10 16:37 ` Russell King - ARM Linux 2014-02-10 17:28 ` Dave Martin 2014-02-10 13:58 ` Russell King - ARM Linux 2014-02-10 14:16 ` Dave Martin 2014-02-10 14:44 ` Fabrice Gasnier 2014-02-10 15:12 ` Dave Martin 2014-02-10 15:24 ` Russell King - ARM Linux 2014-02-10 16:36 ` Fabrice Gasnier 2014-02-10 14:54 ` Ben Dooks 2014-02-10 15:21 ` Dave Martin 2014-02-10 16:38 ` Ben Dooks 2014-02-11 15:38 ` Dave Martin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).