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

[Leo Liang]

On Mon, Sep 14, 2020 at 07:57:04AM -0400, Sean Anderson wrote:
> On 9/14/20 2:38 AM, Bin Meng wrote:
> > On Sat, Sep 12, 2020 at 2:25 AM Sean Anderson <seanga2 at gmail.com> wrote:
> >>
> >> On 9/11/20 10:43 AM, Bin Meng wrote:
> >>> On Fri, Sep 11, 2020 at 6:20 PM Sean Anderson <seanga2 at 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 at 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 at 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?
> > 
> > Sounds like there was some misunderstanding on "passed to U-Boot" ..
> > But I got it now.
> > 
> >>
> >>>> 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
> > 
> > So U-Boot on K210 boots with M-mode from the K210 ROM, and the ROM
> > code does not hold DTB address in a1 before jumping to U-Boot, right?
> > 
> >> 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.
> > 
> > Because U-Boot S-mode needs to fix up the DT when OF_SEPERATE is used.
> 
> Right. It's just unexpected because OF_SEPARATE appears to imply to both
> use a separate device tree and to not use the passed-in device tree.
> This is because it is mutually exclusive with OF_PRIOR_STAGE. However,
> with OF_BOARD_FIXUP, it's as if one has selected both OF_SEPARATE and
> OF_PRIOR_STAGE at once. I think defaulting OF_BOARD_FIXUP to y only
> S-Mode is more likely to result in unsurprising behavior on new boards.
> 
> --Sean

Reviewed-by: Leo Liang <ycliang at andestech.com>