[PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode

[Sean Anderson <seanga2@gmail.com>]

On 9/11/20 10:43 AM, Bin Meng wrote:
> On Fri, Sep 11, 2020 at 6:20 PM Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 9/11/20 3:29 AM, Bin Meng wrote:
>>> Hi Sean,
>>>
>>> On Sat, Sep 5, 2020 at 9:22 PM Sean Anderson <seanga2@gmail.com> wrote:
>>>>
>>>> It is unsafe to enable OF_BOARD_FIXUP only based on OF_SEPARATE.
>>>> OF_SEPARATE may indicate that the user wishes U-Boot to use a different
>>>> device tree than one obtained via OF_PRIOR_STAGE. However, OF_SEPARATE may
>>>> also indicate that the device tree which would be obtained via
>>>> OF_PRIOR_STAGE is invalid, nonexistant, or otherwise unusable. In this
>>>
>>> typo: nonexistent
>>>
>>>> latter case, enabling OF_BOARD_FIXUP will result in corruption of the
>>>> device tree. To remedy this, only enable OF_BOARD_FIXUP if U-Boot is
>>>> configured for S-Mode.
>>>>
>>>> Fixes: 1c17e55594a394ced7de88d91be294eaf8c564c1
>>>
>>> nits: the format should be: commit_id ("description")>
>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>>> ---
>>>>
>>>>  arch/riscv/Kconfig | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>>> index 009a545fcf..13fac51483 100644
>>>> --- a/arch/riscv/Kconfig
>>>> +++ b/arch/riscv/Kconfig
>>>> @@ -288,6 +288,6 @@ config STACK_SIZE_SHIFT
>>>>         default 14
>>>>
>>>>  config OF_BOARD_FIXUP
>>>> -       default y if OF_SEPARATE
>>>> +       default y if OF_SEPARATE && RISCV_SMODE
>>>
>>> Is that your board is running U-Boot M-mode with OF_SEPARATE that does not work?
>>
>> Yes, because the reason we use OF_SEPARATE is because no device tree is
>> passed to U-Boot. Trying to use the device tree passed to U-Boot even
> 
> I don't get it. If no device tree is passed to U-Boot, why using
> OF_SEPARATE in the first place?

Because it has to come from somewhere. Where else would U-Boot get the
device tree?

>> though OF_SEPARATE is enabled results in garbage being written to the
> 
> What garbage data is written?

It might not be garbage written. I didn't document the exact failure
mode at the time I discovered this bug, so I went back to try and
reproduce it for a more thorough analysis. However, I was unable to
reproduce this bug, even on the branch where I originally triggered it.
I documented my reasoning behind this patch at [1]. In my testing, I
could only trigger a "periodic-32" bug.

In any case, this behavior could still cause problems in the future.
From my testing, on the k210, a1 usually holds some address on the ROM's
stack. However, if it (for instance) instead held an address which
raised a load access fault, or was misaligned, then booting would fail.
In the general case, I was very surpised that U-Boot was using the value
of a1 on entry even with OF_SEPARATE specified. I would expect it only
to use that value if configured with OF_PRIOR_STAGE.

--Sean

[1] https://patchwork.ozlabs.org/project/uboot/patch/20200815155237.467720-12-seanga2 at gmail.com/#2520514