CONFIG_NEON=y causes platform lockups with certain application/platform combinations

@Dave,

I just simply disabled the NEON in fsl-imx51 kernel now. The git tree is here:
http://kernel.ubuntu.com/git?p=roc/ubuntu-lucid.git;a=shortlog;h=refs/heads/lp507416

And please download the kernel package here to do some test on your hardware:
http://people.canonical.com/~roc/kernel/lp507416/

Thanks,
-Bryan

On Babbage 2.0, installing and booting Bryan's kernel from here

http://people.canonical.com/~roc/kernel/lp507416/

I get the following results:

/proc/cpuinfo no longer lists neon among the CPU features;
AT_HWCAP in /proc/self/auxv no longer reports HWCAP_NEON;
ffplay now sources the ffmpeg libraries from /usr/lib/ instead of /usr/lib/neon/, and appears to work stably (albeit slower)

I added a simple dynamic checking patch in the kernel and build the kernel again, please help me to test on your side.
So in this kernel, CONFIG_NEON is enabled but it won't export NEON bit in HWCAP on TO1 and TO2.

Kernel:
http://people.canonical.com/~roc/kernel/neon/

Git branch:
http://kernel.ubuntu.com/git?p=roc/ubuntu-lucid.git;a=shortlog;h=refs/heads/neon

Thanks,
-Bryan

The patch looks sensible, and appears to work for me on Babbage 2.0.

Can someone else please test on Babbage 3.0?

Execution of NEON instructions and NEON context switching still appear to work OK on Babbage 2.0: See the attachment:
$ gcc -g -o neon-check neon-check.s
$ gcc -g -o neon-probe neon-probe.s
$ ./neon-check &
$ ./neon-probe &

(If you're not using the lucid tools, you may need extra flags, such as -march=armv7-a -mfpu-neon)

neon-check writes known values into some NEON registers and then loops forever, checking that the registers do not change.
neon-probe writes different values into the NEON registers, trying to provoke a failure.

Both processes should run forever if NEON instructions can be executed and context switching is working properly.

If CONFIG_NEON=n, neon-check should SIGILL immediately (on the standard karmic or jaunty kernel for example)

If context switching corruption is detected, neon-check should print an error message and terminate prematurely. (This may currently happen on Babbage 3.0 while https://bugs.launchpad.net/ubuntu/+source/linux-fsl-imx51/+bug/507503 remains unfixed. However, the test is not exhaustive; not all NEON registers are checked for corruption. The test could easily be extended however.)

Testing Bryan's kernel on a Babbage 3.0 with a recent lucid daily:

proc/cpuinfo

Processor : ARMv7 Processor rev 1 (v7l)
BogoMIPS : 799.53
Features : swp half thumb fastmult vfp edsp vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant : 0x2
CPU part : 0xc08
CPU revision : 1

Hardware : Freescale MX51 Babbage Board
Revision : 51120
Serial : 0000000000000000

@Jamie,

Are you sure this is running on your BB3.0 board. The cpuinfo is the same as my BB 2.5 board. BB3 is supposed to be 51130 instead of 51120.

Thanks,
-Bryan

@Dave,

After some discussion with Alex and Oliver, I think we still need to disable the whole CONFIG_NEON for our lucid kernel. Although my dynamic chip rev checking patch can make some testing pass, we still have some potential issues from user space NEON applications.

There are 2 types NEON applications:
Type 1 applications will be built twice and will generate 2 version of .so or executable files. One is -neon version and the other is -none-neon one. Then these applications will determine whether NEON is available from the hardware, if
NEON is there, it will run neon version code. Otherwise run none neon version.
Type 2 applications will enable NEON instruction code when building. And it will not determine the NEON flag from kernel. Such as Chromium.

According to the impact from second type applications, I think we will switch back to turn on the CONFIG_NEON.

Thanks,
-Bryan

this is my 2.5 board (with the current archive kernel though, installed from alpha2) showing 51025 as the revision number ...

ogra@babbage2:~$ cat /proc/cpuinfo
Processor : ARMv7 Processor rev 1 (v7l)
BogoMIPS : 159.90
Features : swp half thumb fastmult vfp edsp neon vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant : 0x2
CPU part : 0xc08
CPU revision : 1

Hardware : Freescale MX51 Babbage Board
Revision : 51025
Serial : 0000000000000000

@Bryan: in the case of chromium which you describe, the application ought to be fixed to NOT use NEON by default -- or it wont work on e.g. Marvell v7 (ARMADA) -- instead of using it if the kernel has it. In general, packages built for Ubuntu should not look at the currently running kernel to decide to which level to optimize for. Apps should use the configure target triplet and toolchain defaults.

One example package doing this is ffmpeg; it detects whether the toolchain defaults to enabling NEON to build or not a NEON flavour.

If Chromium can not be built without NEON at all, then the kernel needs not change, we just need to make sure we don't use it on hardware without NEON.

Apologies, it seems in my recent office move I mixed up the boards and plugged a 2.5 board in instead of the 3.0. Below is the real output from a 3.0 board.

Processor : ARMv7 Processor rev 5 (v7l)
BogoMIPS : 799.53
Features : swp half thumb fastmult vfp edsp neon vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant : 0x2
CPU part : 0xc08
CPU revision : 5

Hardware : Freescale MX51 Babbage Board
Revision : 51130
Serial : 0000000000000000

Thanks a lot, Jamie. That is the right thing.

After some email with Rob from Freescale, for TO2 rev is 51120 and TO3 rev should be 51130. Oliver's revision might be a bug in U-Boot or something.

So I think using 'cpu_is_mx51_rev(CHIP_REV_3_0)' to do dynamic testing is a safe way to workaround this NEON issue on old silicon.

How do you guys think of this?

the uboot-imx package now carries the patch below:

--- uboot-imx-2009.01/include/asm-arm/arch-mx51/mx51.h 2010-01-20 15:41:00.918768862 +0000
+++ uboot-imx-2009.01.new/include/asm-arm/arch-mx51/mx51.h 2010-01-20 15:42:58.338756978 +0000
@@ -393,8 +393,8 @@
 #define CHIP_REV_1_0 0x10
 #define CHIP_REV_1_1 0x11
 #define CHIP_REV_2_0 0x20
-#define CHIP_REV_2_5 0x25
-#define CHIP_REV_3_0 0x30
+#define CHIP_REV_2_5 0x120
+#define CHIP_REV_3_0 0x130

 #define BOARD_REV_1_0 0x0
 #define BOARD_REV_2_0 0x1

which properly results in:

ogra@babbage2:~$ cat /proc/cpuinfo |grep ^Revision
Revision : 51120

so uboot shouldnt be an issue anymore and your kernel patch should work fine under both bootloaders

> After some email with Rob from Freescale, for TO2 rev is
> 51120 and TO3 rev should be 51130. Oliver's revision might be
> a bug in U-Boot or something.

Babbage 2.0 still gives the strange value 51020 in /proc/cpuinfo (using
redboot 200930-0ubuntu1 I think).

> So I think using 'cpu_is_mx51_rev(CHIP_REV_3_0)' to do
> dynamic testing is a safe way to workaround this NEON issue
> on old silicon.

This won't match 51020 or 51120, so I guess it should work.

--
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Regarding wrong board version for EVK3.0, it was fixed before. The attachment is the patch.

Regarding wrong board version for EVK3.0, it was fixed before. The attachment is the patch.

This bug was fixed in the package linux-fsl-imx51 - 2.6.31-603.5

---------------
linux-fsl-imx51 (2.6.31-603.5) lucid; urgency=low

  [ Bryan Wu ]

  * [Config] Enable devtmpfs for fsl-imx51 kernel
    - LP: #512321
  * SAUCE: IMX51: only export NEON flag to userspace on Freescale iMX51
    rev3.x or later silicon
    - LP: #507416

  [ Heiko Carstens ]

  * (pre-stable) driver-core: fix devtmpfs crash on s390
    - LP: #512370

  [ Kay Sievers ]

  * (pre-stable) Driver-Core: devtmpfs - set root directory mode to 0755
    - LP: #512370

  [ Upstream Kernel Changes ]

  * ENGR00119069 V4L2 capture: Do not change current crop setting for tvin.
  * ENGR00118830 Update NAND driver scan scheme to support new nand type
  * ENGR00118946 imx23: enable wake up from USB 5V
  * ENGR00119150 Bluetooth: Handsfree audio has noise
  * ENGR00118610 MX23: Add double buffering for PXP
  * ENGR00118872 Write bootstream to kernel by kobs with BCH ECC
  * ENGR00119199: ipu: add clock nodes for pixel clocks
  * ENGR00119202: Fix DVFS-PER related bugs.
  * ENGR00119203 MX23: Correct VDDD value for CPU frequency 360 MHz
  * ENGR00119004 v4l2 output: fix kernel dump
  * ENGR00119242 Add wait timeout support to void dead loop in NAND driver
  * ENGR00119263: MX51 : Fix pll_set_rate function
  * ENGR00119075 fix iMX23 USB initialization cause wrong power status
  * ENGR00118892 MX23: iMX233 disable lcd clock when LCD off
  * ENGR00119063 MX23: fix system halt rather than reboot when watchdog
    timeout
  * ENGR00119324 Put GPMI NAND flash scan scheme code to the common NAND
    directory
  * ENGR00114151 800x600-16@60 video not play correctly
  * ENGR00119275 ipuv3: dmfc size control
  * ENGR00119443 [MX23_BSP] GPMI driver computes wrong block size for
    K9GAG08U0D
  * ENGR00119136 ipuv3: support 720p for ipu lib
  * ENGR00119274 TVE: HDTV can not work
  * ENGR00119104 MX23 ALSA: Resolve the problem of record from line-in
  * ENGR00119081 V4l2 capture:Support NV12 output pixel format for still
    capture
  * ENGR00119179 TVE: fix system hang for tvout
  * ENGR00119070 IPUv3 FB:Support DP local alpha in pixel
  * ENGR00119432 MX35 system can't re-boot up when SD/MMC boot is used.
  * ENGR00119296 Fix iMX23 display abnormal when change frequency
  * ENGR00119484 Update NFC INT wait timeout value to 1s
  * ENGR00119504 MX25: Change nand partition for bootloader to 3M byte
  * ENGR00119532 MX35: Change nand partition for bootloader to 3M bytes
  * ENGR00119305 imx23: fix ethernet standby issue
  * ENGR00115370 ipuv3: add display control
  * ENGR00119531 ipuv3: dmfc setting restore during resume
  * Driver Core: devtmpfs - kernel-maintained tmpfs-based /dev
    - LP: #512321
 -- Andy Whitcroft <email address hidden> Wed, 27 Jan 2010 10:30:45 +0000

Re-opening the bug to discuss some new issues.

Both of these problems are effectively present in the previous Ubuntu releases on imx51, as well as lucid.

1) CONFIG_NEON=n doesn't actually disable the use of NEON instructions (I misinderstood things here). It should be straightforward to improve the situation here: for platforms with known problems, or when CONFIG_NEON=n, we can set the ASEDIS bit in the CP15 Coprocessor Access Control Register on boot. This disables the part of the NEON instruction space containing the instructions which can cause unaligned accesses to occur.

2) It appears that all unaligned accesses (not just NEON) can cause problems. This may apply to cached memory, but has not definitely been observed. However, the problem is observable for uncached memory. This means that memory-mapped drivers such as the framebuffer or media accelerators can provide userspace processes with a way to hit the bug using ordinary unaligned loads and stores.

The problem is that the kernel does not trap ordinary loads and stores on ARMv6 and above, because these processors support the unaligned access natively.

arch/arm/mm/alignment.c:
static int __init alignment_init(void)
{
        ...
        if (cpu_architecture() >= CPU_ARCH_ARMv6 && (cr_alignment & CR_U)) {
                cr_alignment &= ~CR_A;
                cr_no_alignment &= ~CR_A;
                set_cr(cr_alignment);
                ai_usermode = UM_FIXUP;
        }

In principle we could support the full faulting by modifying the if() condition, however there are two problems with this:
    a) there may be a performance impact, since more loads and stores will be emulated via the kernel
    b) some extra implementation is needed in arch/arm/mm/alignment.c:do_alignment_t32_to_handler() to recognise the 32-bit Thumb-2 load and store instructions (since all processors which have these instructions also have native unaligned access support); recognising these instructions is not implemented at present, which means that such instructions will either SIGILL, or spin as they are continuously refaulted.

@Dave,

Appreciate for this useful information.

If I understand correctly, I think our goal is to make one kernel for all the imx51 silicon. On the good silicon, we enable all the NEON and VFP. On the bad silicon, we disable the NEON and add some hack in unaligned access code.

So we are going to do following things in kernel,
0, turn on CONFIG_NEON=y
1. dynamic detect the silicon rev
2. if rev < 3, we disable NEON by setting ASEDIS
3. if rev < 3, we add some hack in alignment_init() of arch/arm/mm/alignment.c.
4. my patch -- 'remove NEON flag of HWCAP dynamically' -- is still helpful for user space to run non-NEON version code.

But, I just think if we add some hack in alignment_init to handle all the unaligned access for both NEON or none NEON, we don't need to disable NEON at all, since this is an unaligned access issue not a specific NEON one.

> If I understand correctly, I think our goal is to make one
> kernel for all the imx51 silicon. On the good silicon, we

I think this is the right approach--- we want to avoid fragmentation.

> enable all the NEON and VFP. On the bad silicon, we disable
> the NEON and add some hack in unaligned access code.
>
> So we are going to do following things in kernel,
> 0, turn on CONFIG_NEON=y
> 1. dynamic detect the silicon rev
> 2. if rev < 3, we disable NEON by setting ASEDIS
> 3. if rev < 3, we add some hack in alignment_init() of
arch/arm/mm/alignment.c.
> 4. my patch -- 'remove NEON flag of HWCAP dynamically' -- is still helpful
for user space to run non-NEON version code.
>
> But, I just think if we add some hack in alignment_init to
> handle all the unaligned access for both NEON or none NEON,
> we don't need to disable NEON at all, since this is an
> unaligned access issue not a specific NEON one.

I _think_ you are correct on this point, but I haven't tried it; also, the
ASEDIS fix is much easier than enabling full alignment faulting (which will
require new code which may more than double the size of
do_alignment_t32_to_handler() to translate the additional instructions).

Full alignment faulting will have a performance impact, but I don't know how
much without trying it, and Ubuntu is unlikely even to boot properly if the
necessary extensions in of do_alignment_t32_to_handler() are missing. The
impact could be severe if there are a lot of memcpy() of unaligned buffers
etc., though this may not be the case (or memcpy() might be clever enough to
avoid it).

However, if we do have full alignment faulting and leave ASEDIS clear, we
may have the nice side-effect that aligned NEON code will be able to
execute, whilst being able to nuke unaligned use with SIGILL.

I'll try and feed back on what amount and complexity of extra code might be
needed.

It looks like it's possible to implement the additional unaligned access emulation for Thumb-2, but it may involve a non-trivial amount of work.

In particular, loads and stores behave differently for ThumbEE compared with normal Thumb-2. ThumbEE is not much used at present, so it would probably be OK simply to SIGILL in such cases though, without implementing all the emulation for ThumbEE specifically.

A couple of possible ways forward:

1) Currently, I've been working on the assumption that because lucid is mostly Thumb-2 it won't work at all if we turn on aligned access faulting with the current kernel. This might not be true. A couple of things we could try with the current kernel:

   a) Hack the kernel to turn on full unaligned access faulting, run a karmic filesystem (which is ARM, not Thumb-2, and so should work without problems) --- look at the kernel log messages and the fixups counted in /proc/cpu/alignment

   b) Hack the kernel to turn on full unaligned access faulting, run a lucid filesystem --- see whether it works, and whether the kernel generates any log messages about failed alignment fixups.

2) We take a pragmatic approach and consider that lucid only needs to be "as good as" karmic on i.MX51 TO2. If this is considered acceptable, we can simply build with CONFIG_NEON=y and lie about HWCAP_NEON for <=TO2 --- this is actually functionally equivalent to the karmic/lucid configuration from the point of view of userspace, except that well-behaved NEON code becomes usable (which was not previously the case).

We could alternatively use CONFIG_NEON=y ... and dynamically lie about HCWAP_NEON and set ASEDIS for <=TO2. This makes it harder for (non-privileged) user code to cause problems; so this configuration is a bit more robust than jaunty/karmic, at the expense of preventing well-behaved NEON code from working.

Bryan, Martin - After chatting with Freescale I'm proposing to SRU the NEON detection patch to Karmic. Are you both still of the opinion that the ASEDIS fix for rev < 3 is still unnecessary ?

Oliver - will the uboot-imx supply the correct version since I assume the patch mentioned in https://bugs.edge.launchpad.net/ubuntu/+source/linux-fsl-imx51/+bug/507416/comments/12 does not exist in karmic ?

we dont use uboot-imx in ubuntu yet (the latest code drop shuold have a patch for setting the revision numbers right though), redboot (which we use across all releases) definately sets the proper revision numbers.

Karmic SRU

Justification: Enable NEON support for Freescale imx51 rev 3 boards
Impact: Applications that _could_ use NEON instructions to speedup encode/decode operations (youtube, etc) are forced to operate in full emulation mode.
Patch description: Detect NEON capability at kernel boot time and set the capability bit if rev >= 3.

Patch attached.

closing the uboot task, for karmic uboot-imx doesnt exist, in lucid a patch is added that spits out the right revision numbers

Accepted linux-fsl-imx51 into karmic-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

For Karmic -proposed, I think we still need to provide another patch to set ASEDIS for <=TO2. I am working on this and will provide patch soon.

-Bryan

After discussing with Dave, I found ASEDIS is no available in Cortex A8 based SoC. It is available in Cortex A9. So our plan to disable NEON hardware entirely by setting ASEDIS on TO2 silicon is impossible now.

Generally, for this silicon issue there is no more workaround so far. And the patches in -proposed will not cause any regression. It is safe for our SRU.

-Bryan

Close it for Lucid, if find another workaround method, please reopen it.

Emmet asked me to drop this kernel from karmic-proposed, since it was incomplete and needs more fixes.

Re-accepted into karmic-proposed; the removal was a misunderstanding, sorry. Please test and give feedback here.

Thanks!

CONFIG_NEON does not seem to be enabled in the proposed kernel

neon certainly seems to be turned on now, however the testcase attached to this bug fails (and fails in lucid as well). I get illegal instruction when running either one of them, and get the following in dmesg:
Alignment trap: not handling instruction f920079f at [<000083f0>]
Unhandled fault: alignment exception (0x001) at 0x000084b4

@Dave Martin - is this something to be concerned with?

Hi there,

Try the attached modification. It looks like I forgot to force the correct data alignment in the test program.

Something must have changed in libc in the meantime, altering the link map slightly and pushing the data off by a word. In this case, an alignment fault is the correct thing to happen, and we wouldn't expect the kernel to fix it up.

Looks like I just got lucky first time around ;)

Cheers
—Dave

Thanks Dave, that worked! The kernel in question here has neon turned on, and passes the updated regression test attached to this bug.

This bug was fixed in the package linux-fsl-imx51 - 2.6.31-111.27

---------------
linux-fsl-imx51 (2.6.31-111.27) karmic-proposed; urgency=low

  [ Bryan Wu ]

  * [Config] built in SMSC_PHY driver for fsl-imx51
    - LP: #546649
  * SAUCE: fsl-imx51: sync karmic fec.c driver with lucid fec.c
    - LP: #546649
  * [Config]: turn off CONFIG_FIXED_PHY for fsl-imx51
    - LP: #546649

linux-fsl-imx51 (2.6.31-110.26) karmic-proposed; urgency=low

  [ Stefan Bader ]

  * Rebased to 2.6.31-21.59
  * [Config] Enable CONFIG_NEON
    - LP: #507416

  [ Ubuntu: 2.6.31-21.59 ]

  * [Config] generic-pae switch to M586TSC
    - LP: #519448
  * (pre-stable) drm/i915: Increase fb alignment to 64k
    - LP: #404064
  * Input: i8042 - bypass AUX IRQ delivery test on laptops
    - LP: #534448
  * SAUCE: Fix volume hotkeys for Dell Studio 1557
    - LP: #465250
  * SAUCE: aufs: Fix header files inclusion in debug.h
    - LP: #517151
  * [Config] Enable all CGROUP configuration options
    - LP: #480739
  * Revert "[Upstream] acerhdf: Limit modalias matching to supported
    boards"
    - LP: #509730
  * [Config] ext3 defaults to ordered mode
    - LP: #510067
  * [Config] Fix sub-flavours package conflicts
    - LP: #454827
  * PCI/cardbus: Add a fixup hook and fix powerpc
    - LP: #455723
  * fnctl: f_modown should call write_lock_irqsave/restore
    - LP: #519436
  * ACPI: enable C2 and Turbo-mode on Nehalem notebooks on A/C
    - LP: #516325
  * tg3: Add 57788, remove 57720
    - LP: #515390
  * HID: ignore all recent SoundGraph iMON devices
    - LP: #488443
  * Input: ALPS - add interleaved protocol support (Dell E6x00 series)
    - LP: #296610
  * acerhdf: limit modalias matching to supported
    - LP: #509730
  * ASoC: Do not write to invalid registers on the wm9712.
    - LP: #509730
  * cifs: NULL out tcon, pSesInfo, and srvTcp pointers when chasing DFS
    referrals
    - LP: #509730
  * clockevents: Prevent clockevent_devices list corruption on cpu hotplug
    - LP: #509730
  * dma: at_hdmac: correct incompatible type for argument 1 of
    'spin_lock_bh'
    - LP: #509730
  * drivers/net/usb: Correct code taking the size of a pointer
    - LP: #509730
  * Libertas: fix buffer overflow in lbs_get_essid()
    - LP: #509730
  * md: Fix unfortunate interaction with evms
    - LP: #509730
  * pata_cmd64x: fix overclocking of UDMA0-2 modes
    - LP: #509730
  * pata_hpt3x2n: fix clock turnaround
    - LP: #509730
  * SCSI: fc class: fix fc_transport_init error handling
    - LP: #509730
  * sound: sgio2audio/pdaudiocf/usb-audio: initialize PCM buffer
    - LP: #509730
  * USB: emi62: fix crash when trying to load EMI 6|2 firmware
    - LP: #509730
  * USB: Fix a bug on appledisplay.c regarding signedness
    - LP: #509730
  * USB: musb: gadget_ep0: avoid SetupEnd interrupt
    - LP: #509730
  * USB: option: support hi speed for modem Haier CE100
    - LP: #490068, #509730
  * x86, cpuid: Add "volatile" to asm in native_cpuid()
    - LP: #509730
  * e100: Use pci pool to work around GFP_ATOMIC order 5 memory allocation
    failure
    - LP: #509730
  * e100: Fix broken cbs accounting due to missing memset.
    - LP: #509730
  * hostap: Revert a toxic part...