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

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

[Sean Anderson]

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
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
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
 
 endmenu
-- 
2.28.0

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

[Heinrich Schuchardt]

On 9/5/20 3:22 PM, Sean Anderson 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
> 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
> 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


OF_BOARD_FIXUP is also defined in dts/Kconfig.
Moving the "default" line to dts/Kconfig would simplify the code.

Why do we need the dependency on OF_SEPARATE? Isn't RISCV_SMODE enough?

Best regards

Heinrich

>
>  endmenu
>

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

[Sean Anderson]

On 9/6/20 7:18 AM, Heinrich Schuchardt wrote:
> On 9/5/20 3:22 PM, Sean Anderson 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
>> 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
>> 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
> 
> 
> OF_BOARD_FIXUP is also defined in dts/Kconfig.
> Moving the "default" line to dts/Kconfig would simplify the code.

Probably.

> 
> Why do we need the dependency on OF_SEPARATE? Isn't RISCV_SMODE enough?

Because there is no need to do this fixup if we are using sbi's device
tree. From the commit referenced above:

> Starting from OpenSBI v0.7, the SBI firmware inserts/fixes up the
> reserved memory node for PMP protected memory regions. All RISC-V
> boards need to copy the reserved memory node from the device tree
> provided by the firmware to the device tree used by U-Boot.

--Sean

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

[Bin Meng]

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?

>
>  endmenu
> --

Regards,
Bin

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

[Sean Anderson]

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
though OF_SEPARATE is enabled results in garbage being written to the
actual device tree. Without this patch, booting on the K210 randomly
fails.

> 
>>
>>  endmenu
>> --
> 
> Regards,
> Bin
> 

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

[Bin Meng]

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?

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

What garbage data is written?

> actual device tree. Without this patch, booting on the K210 randomly
> fails.

Regards,
Bin

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

[Sean Anderson]

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

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

[Bin Meng]

On Sat, Sep 12, 2020 at 2:25 AM Sean Anderson <seanga2@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@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?

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.

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

Regards,
Bin

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

[Sean Anderson]

On 9/14/20 2:38 AM, Bin Meng wrote:
> On Sat, Sep 12, 2020 at 2:25 AM Sean Anderson <seanga2@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@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?
> 
> 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

[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@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@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?
> > 
> > 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@andestech.com>