* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode @ 2020-09-05 13:22 Sean Anderson 2020-09-06 11:18 ` Heinrich Schuchardt 2020-09-11 7:29 ` Bin Meng 0 siblings, 2 replies; 10+ messages in thread From: Sean Anderson @ 2020-09-05 13:22 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-05 13:22 [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode Sean Anderson @ 2020-09-06 11:18 ` Heinrich Schuchardt 2020-09-06 12:56 ` Sean Anderson 2020-09-11 7:29 ` Bin Meng 1 sibling, 1 reply; 10+ messages in thread From: Heinrich Schuchardt @ 2020-09-06 11:18 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-06 11:18 ` Heinrich Schuchardt @ 2020-09-06 12:56 ` Sean Anderson 0 siblings, 0 replies; 10+ messages in thread From: Sean Anderson @ 2020-09-06 12:56 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-05 13:22 [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode Sean Anderson 2020-09-06 11:18 ` Heinrich Schuchardt @ 2020-09-11 7:29 ` Bin Meng 2020-09-11 10:20 ` Sean Anderson 1 sibling, 1 reply; 10+ messages in thread From: Bin Meng @ 2020-09-11 7:29 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-11 7:29 ` Bin Meng @ 2020-09-11 10:20 ` Sean Anderson 2020-09-11 14:43 ` Bin Meng 0 siblings, 1 reply; 10+ messages in thread From: Sean Anderson @ 2020-09-11 10:20 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-11 10:20 ` Sean Anderson @ 2020-09-11 14:43 ` Bin Meng 2020-09-11 18:25 ` Sean Anderson 0 siblings, 1 reply; 10+ messages in thread From: Bin Meng @ 2020-09-11 14:43 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-11 14:43 ` Bin Meng @ 2020-09-11 18:25 ` Sean Anderson 2020-09-14 6:38 ` Bin Meng 0 siblings, 1 reply; 10+ messages in thread From: Sean Anderson @ 2020-09-11 18:25 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-11 18:25 ` Sean Anderson @ 2020-09-14 6:38 ` Bin Meng 2020-09-14 11:57 ` Sean Anderson 0 siblings, 1 reply; 10+ messages in thread From: Bin Meng @ 2020-09-14 6:38 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-14 6:38 ` Bin Meng @ 2020-09-14 11:57 ` Sean Anderson 2020-09-16 9:41 ` Leo Liang 0 siblings, 1 reply; 10+ messages in thread From: Sean Anderson @ 2020-09-14 11:57 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode 2020-09-14 11:57 ` Sean Anderson @ 2020-09-16 9:41 ` Leo Liang 0 siblings, 0 replies; 10+ messages in thread From: Leo Liang @ 2020-09-16 9:41 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-16 9:41 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-05 13:22 [PATCH] riscv: Only enable OF_BOARD_FIXUP for S-Mode Sean Anderson 2020-09-06 11:18 ` Heinrich Schuchardt 2020-09-06 12:56 ` Sean Anderson 2020-09-11 7:29 ` Bin Meng 2020-09-11 10:20 ` Sean Anderson 2020-09-11 14:43 ` Bin Meng 2020-09-11 18:25 ` Sean Anderson 2020-09-14 6:38 ` Bin Meng 2020-09-14 11:57 ` Sean Anderson 2020-09-16 9:41 ` Leo Liang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox