[RFC PATCH] ARM: Enable imprecise external aborts earlier

[RFC PATCH] ARM: Enable imprecise external aborts earlier

[Fabrice GASNIER]

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

[RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Fabrice GASNIER]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Will Deacon]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Fabrice Gasnier]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Ben Dooks]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Will Deacon]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Fabrice Gasnier]

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!

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Fabrice Gasnier]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Russell King - ARM Linux]

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".

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Russell King - ARM Linux]

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".

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Will Deacon]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Dave Martin]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Dave Martin]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Fabrice Gasnier]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Ben Dooks]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Dave Martin]

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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Russell King - ARM Linux]

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".

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Dave Martin]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Russell King - ARM Linux]

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".

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Dave Martin]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Fabrice Gasnier]

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.
>

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Russell King - ARM Linux]

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".

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Ben Dooks]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Dave Martin]

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

Re: [RFC PATCH] ARM: Add imprecise abort enable/disable macro

[Dave Martin]

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