Merging code with arch condition into head.S

Merging code with arch condition into head.S

[Rafał Miłecki]

Hi,

I'd like to ask for help with writing my arcm/arm/boot/compressed/head-foo.S.

There is probably a bug in bootloader on *a lot* market-available
devices that needs a very early fix executed during decompression. If
we don't implement it, Broadcom Northstar devices hang in 25% of tries
and BCM53573 devices don't boot at all.

The problem is ARCH_BCM_5301X is used with ARCH_MULTI_V7, so we really
shouldn't merge some extra code into head.S and execute it
unconditionally. I'd like to ask if you can suggest some idea for a
sane architecture check.

Ideally I'd read it from DT, but I don't think I can read machine
"compatible" from ASM. As alternative I could try reading SoC chip ID
from ChipCommon component which Broadcom always maps at 0x18000000.
Would that be safe enough? Could reading from such offset do any harm
on other devices?

Maybe you have some better ideas?

I was looking for similar solutions in the kernel, but I found none.
There are e.g. head-sa1100.S and head-sharpsl.S but they seem to be
used on architectures that don't use ARCH_MULTI_V7.

-- 
Rafa?

Merging code with arch condition into head.S

[Ard Biesheuvel]

On 18 August 2016 at 12:35, Rafa? Mi?ecki <zajec5@gmail.com> wrote:
> Hi,
>
> I'd like to ask for help with writing my arcm/arm/boot/compressed/head-foo.S.
>
> There is probably a bug in bootloader on *a lot* market-available
> devices that needs a very early fix executed during decompression. If
> we don't implement it, Broadcom Northstar devices hang in 25% of tries
> and BCM53573 devices don't boot at all.
>

Could you elaborate on the nature of the fix?

> The problem is ARCH_BCM_5301X is used with ARCH_MULTI_V7, so we really
> shouldn't merge some extra code into head.S and execute it
> unconditionally. I'd like to ask if you can suggest some idea for a
> sane architecture check.
>
> Ideally I'd read it from DT, but I don't think I can read machine
> "compatible" from ASM. As alternative I could try reading SoC chip ID
> from ChipCommon component which Broadcom always maps at 0x18000000.
> Would that be safe enough? Could reading from such offset do any harm
> on other devices?
>

Yes.

> Maybe you have some better ideas?
>
> I was looking for similar solutions in the kernel, but I found none.
> There are e.g. head-sa1100.S and head-sharpsl.S but they seem to be
> used on architectures that don't use ARCH_MULTI_V7.
>

Generally, it is not the kernel's job to fix broken bootloaders,
especially not in generic kernels. What kind of bootloader does this
bug exist in?

-- 
Ard.

Merging code with arch condition into head.S

[Rafał Miłecki]

On 18 August 2016 at 12:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 18 August 2016 at 12:35, Rafa? Mi?ecki <zajec5@gmail.com> wrote:
>> I'd like to ask for help with writing my arcm/arm/boot/compressed/head-foo.S.
>>
>> There is probably a bug in bootloader on *a lot* market-available
>> devices that needs a very early fix executed during decompression. If
>> we don't implement it, Broadcom Northstar devices hang in 25% of tries
>> and BCM53573 devices don't boot at all.
>>
>
> Could you elaborate on the nature of the fix?

I'm not ARM expert and I don't have any Broadcom datasheets, so I have
a very little view of the issue. It seems that to make booting
reliable, I have to clear 2 following bits in the SCTLR register:
#define CR_M (1 << 0) /* MMU enable */
#define CR_C (1 << 2) /* Dcache enable */
and then invalidate the cache.

Old OpenWrt implementation that comes with its own
v7_all_dcache_invalidate ENTRY:
http://git.openwrt.org/?p=15.05/openwrt.git;a=blob;f=target/linux/bcm53xx/patches-3.18/300-ARM-BCM5301X-Disable-MMU-and-Dcache-for-decompression.patch;hb=HEAD

Newer implementation that re-uses part of existing
__armv7_mmu_cache_flush ENTRY:
https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/bcm53xx/patches-4.4/300-ARM-BCM5301X-Disable-MMU-and-Dcache-for-decompression.patch;h=62bda2e8303ec7361226984253f31d52a8ec80c2;hb=HEAD

The problem I recently discovered is that this new implementation
(without independent v7_all_dcache_invalidate) works on Northstar but
doesn't work on BCM53573. These are details anyway, I hope you got
some overview of the situation.


>> The problem is ARCH_BCM_5301X is used with ARCH_MULTI_V7, so we really
>> shouldn't merge some extra code into head.S and execute it
>> unconditionally. I'd like to ask if you can suggest some idea for a
>> sane architecture check.
>>
>> Ideally I'd read it from DT, but I don't think I can read machine
>> "compatible" from ASM. As alternative I could try reading SoC chip ID
>> from ChipCommon component which Broadcom always maps at 0x18000000.
>> Would that be safe enough? Could reading from such offset do any harm
>> on other devices?
>>
>
> Yes.

Is "Yes." an answer to the last question? Also: yes, it could harm
other devices?


>> Maybe you have some better ideas?
>>
>> I was looking for similar solutions in the kernel, but I found none.
>> There are e.g. head-sa1100.S and head-sharpsl.S but they seem to be
>> used on architectures that don't use ARCH_MULTI_V7.
>>
>
> Generally, it is not the kernel's job to fix broken bootloaders,
> especially not in generic kernels. What kind of bootloader does this
> bug exist in?

It's CFE bootloader developed by Broadcom. You can find some sources
of it in some GPL packages, but those are usually vendor & device
specific. Whenever you flash a new image on your Broadcom-based home
router, the process doesn't involve bootloader, you just flash kernel
& userspace on some predefined flash partition.. So in this situation
I'm trying to find an upstream-able solution for many users who for
sure won't be able to recompile & reflash their CFE bootloaders.

-- 
Rafa?

Merging code with arch condition into head.S

[Andrew Lunn]

On Thu, Aug 18, 2016 at 03:23:41PM +0200, Rafa?? Mi??ecki wrote:
> On 18 August 2016 at 12:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 18 August 2016 at 12:35, Rafa?? Mi??ecki <zajec5@gmail.com> wrote:
> >> I'd like to ask for help with writing my arcm/arm/boot/compressed/head-foo.S.
> >>
> >> There is probably a bug in bootloader on *a lot* market-available
> >> devices that needs a very early fix executed during decompression. If
> >> we don't implement it, Broadcom Northstar devices hang in 25% of tries
> >> and BCM53573 devices don't boot at all.
> >>
> >
> > Could you elaborate on the nature of the fix?
> 
> I'm not ARM expert and I don't have any Broadcom datasheets, so I have
> a very little view of the issue. It seems that to make booting
> reliable, I have to clear 2 following bits in the SCTLR register:
> #define CR_M (1 << 0) /* MMU enable */
> #define CR_C (1 << 2) /* Dcache enable */
> and then invalidate the cache.

So it leaves the caches enabled?

Been there, done that. This is violating the boot loader contract to
the kernel at start time. There are a couple of Marvell Kirkwood
systems what have this problem, and it was extensively discussed at
the time. Basically, it is too late to fix by the time the kernel has
started running. The kernel is already corrupt.

https://lists.debian.org/debian-arm/2012/08/msg00128.html
https://patchwork.kernel.org/patch/3427771/

You need to find another solution, e.g. in intermediate boot loader,
or do what Debian does and append a few instructions to the kernel
when it is installed on a hardware with this issue.

     Andrew

Merging code with arch condition into head.S

[Rafał Miłecki]

On 18 August 2016 at 15:47, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Aug 18, 2016 at 03:23:41PM +0200, Rafa?? Mi??ecki wrote:
>> On 18 August 2016 at 12:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > On 18 August 2016 at 12:35, Rafa?? Mi??ecki <zajec5@gmail.com> wrote:
>> >> I'd like to ask for help with writing my arcm/arm/boot/compressed/head-foo.S.
>> >>
>> >> There is probably a bug in bootloader on *a lot* market-available
>> >> devices that needs a very early fix executed during decompression. If
>> >> we don't implement it, Broadcom Northstar devices hang in 25% of tries
>> >> and BCM53573 devices don't boot at all.
>> >>
>> >
>> > Could you elaborate on the nature of the fix?
>>
>> I'm not ARM expert and I don't have any Broadcom datasheets, so I have
>> a very little view of the issue. It seems that to make booting
>> reliable, I have to clear 2 following bits in the SCTLR register:
>> #define CR_M (1 << 0) /* MMU enable */
>> #define CR_C (1 << 2) /* Dcache enable */
>> and then invalidate the cache.
>
> So it leaves the caches enabled?
>
> Been there, done that. This is violating the boot loader contract to
> the kernel at start time. There are a couple of Marvell Kirkwood
> systems what have this problem, and it was extensively discussed at
> the time. Basically, it is too late to fix by the time the kernel has
> started running. The kernel is already corrupt.
>
> https://lists.debian.org/debian-arm/2012/08/msg00128.html
> https://patchwork.kernel.org/patch/3427771/
>
> You need to find another solution, e.g. in intermediate boot loader,
> or do what Debian does and append a few instructions to the kernel
> when it is installed on a hardware with this issue.

Oh, that looks bad. Thanks a lot for the links, discussion on Jason's
patch was very helpful to understand the problem. I guess I'm quite
out of luck here :(

I think I'll stick to the modified (out of tree) kernel for now, maybe
in a long term this intermediate loader could be a good idea. Did you
ever see solution like this? I never wrote my own loaders, so it'd
take me quite some time to learn required things.

-- 
Rafa?

Merging code with arch condition into head.S

[Andrew Lunn]

> I think I'll stick to the modified (out of tree) kernel for now, maybe
> in a long term this intermediate loader could be a good idea. Did you
> ever see solution like this? I never wrote my own loaders, so it'd
> take me quite some time to learn required things.

Things might of moved on since then, but try Jason Cooper and the
Impedance Matcher.

	  Andrew

Merging code with arch condition into head.S

[Jason Cooper]

Hi!

On Aug 18, 2016, at 10:44, Andrew Lunn <andrew@lunn.ch> wrote:

>> I think I'll stick to the modified (out of tree) kernel for now, maybe
>> in a long term this intermediate loader could be a good idea. Did you
>> ever see solution like this? I never wrote my own loaders, so it'd
>> take me quite some time to learn required things.
> 
> Things might of moved on since then, but try Jason Cooper and the
> Impedance Matcher.

Search for pxa-impedance-matcher on github. I think under zonque?  That's Daniel Mack. He wrote it. It was Nico's suggestion, though. I just expanded it to some mvebu boards. 

hth,

Jason.

Sorry for hacked up email, I'm on the road atm.