Message ID | 1322586763-12791-1-git-send-email-dave.martin@linaro.org |
---|---|
State | Superseded |
Headers | show |
On Tue, Nov 29, 2011 at 05:12:43PM +0000, Dave Martin wrote: > - cmp r6, #0x30 @ present prior to r3p0 > + ALT_UP_B(1f) > + ALT_SMP(cmp r6, #0x30) @ present prior to r3p0 Why not look at the definition of these before you (ab)use them?
On Tue, Nov 29, 2011 at 05:35:37PM +0000, Russell King - ARM Linux wrote: > On Tue, Nov 29, 2011 at 05:12:43PM +0000, Dave Martin wrote: > > - cmp r6, #0x30 @ present prior to r3p0 > > + ALT_UP_B(1f) > > + ALT_SMP(cmp r6, #0x30) @ present prior to r3p0 > > Why not look at the definition of these before you (ab)use them? Sorry -- I did look, but it looks like I confused myself afterwards, because of the way the instruction to skip the SMP block has to be inserted in between the first and second instructions of the block being skipped, rather than at the start (as would happen in a conditional branch). Do you have any concerns about that other than the ALT_SMP/UP being the wrong way around? One thing which strikes me about ALT_UP_B() is that the likely use is to skip over the SMP case code (which may consist of many instructions). This means that if the SMP code is not there at all (as in a UP kernel) no branch is necessary. But the definition is more conservative, so that we're left with a branch jumping to the next instruction even in a UP kernel. The effect is that we really can branch to anywhere with ALT_UP_B() -- a potentially useful feature, but one which we don't currently appear to make use of. An alternative would be for the ALT_UP_B() actually to disappear in a UP kernel. So far as I can see, such an implementation would be compatible with all existing uses of this macro. What do you think? Cheers ---Dave
On Tue, Nov 29, 2011 at 06:06:18PM +0000, Dave Martin wrote: > One thing which strikes me about ALT_UP_B() is that the likely use is > to skip over the SMP case code (which may consist of many instructions). > This means that if the SMP code is not there at all (as in a UP kernel) > no branch is necessary. It's there to solve the case in the IRQ entry code, where it's use is already bounded by a #ifdef CONFIG_SMP. > An alternative would be for the ALT_UP_B() actually to disappear in a > UP kernel. So far as I can see, such an implementation would be > compatible with all existing uses of this macro. What do you think? That sounds dangerous, adding unexpected behaviour to the code. For example: #ifdef CONFIG_SMP /* * XXX * * this macro assumes that irqstat (r2) and base (r6) are * preserved from get_irqnr_and_base above */ ALT_SMP(test_for_ipi r0, r2, r6, lr) ALT_UP_B(9997f) movne r1, sp adrne lr, BSYM(1b) bne do_IPI #endif If the surrounding ifdef goes away, and the ALT_UP_B() ends up being optimized to nothing, then we no longer skip over this code. So no, it's not compatible with existing uses.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 44789ef..e5bbb2f 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1281,7 +1281,7 @@ config ARM_ERRATA_743622 config ARM_ERRATA_751472 bool "ARM errata: Interrupted ICIALLUIS may prevent completion of broadcasted operation" - depends on CPU_V7 && SMP + depends on CPU_V7 help This option enables the workaround for the 751472 Cortex-A9 (prior to r3p0) erratum. An interrupted ICIALLUIS operation may prevent the diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S index 2c559ac..9b06d0b 100644 --- a/arch/arm/mm/proc-v7.S +++ b/arch/arm/mm/proc-v7.S @@ -364,10 +364,12 @@ __v7_setup: mcreq p15, 0, r10, c15, c0, 1 @ write diagnostic register #endif #ifdef CONFIG_ARM_ERRATA_751472 - cmp r6, #0x30 @ present prior to r3p0 + ALT_UP_B(1f) + ALT_SMP(cmp r6, #0x30) @ present prior to r3p0 mrclt p15, 0, r10, c15, c0, 1 @ read diagnostic register orrlt r10, r10, #1 << 11 @ set bit #11 mcrlt p15, 0, r10, c15, c0, 1 @ write diagnostic register +1: #endif 3: mov r10, #0
Activation conditions for a workaround should not be encoded in the workaround's direct dependencies if this makes otherwise reasonable configuration choices impossible. This patches uses the SMP/UP patching facilities instead to compile out the workaround if the configuration means that it is definitely not needed. This means that configs for buggy silicon can simply select ARM_ERRATA_751472, without preventing a UP kernel from being built or duplicatiing knowledge about when to activate the workaround. This seems the correct why to do things, because the erratum is a property of the silicon, irrespective of what the kernel config happens to be. Signed-off-by: Dave Martin <dave.martin@linaro.org> --- arch/arm/Kconfig | 2 +- arch/arm/mm/proc-v7.S | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)