Re: [U-Boot] [PATCH v3 1/6] spl: add Kconfig option to clear bss early

22 May 2019


      Simon Glass sjg@chromium.org schrieb am Mi., 22. Mai 2019, 21:34:
...
Hi Simon,
On Wed, 22 May 2019 at 02:05, Simon Goldschmidt
simon.k.r.goldschmidt@gmail.com wrote:
...
On Wed, May 22, 2019 at 2:53 AM Simon Glass sjg@chromium.org wrote:
...
Hi Andreas,
On Tue, 21 May 2019 at 15:01, Andreas Dannenberg dannenberg@ti.com
wrote:
...
...
...
Hi Simon (Glass),
On Sat, May 18, 2019 at 10:08:19AM -0600, Simon Glass wrote:
...
Hi Andreas,
On Mon, 6 May 2019 at 22:49, Andreas Dannenberg dannenberg@ti.com
wrote:
...
...
...
...
...
Hi Simon,
On Mon, May 06, 2019 at 09:51:56PM -0600, Simon Glass wrote:
> Hi Andreas,
>
> On Fri, 3 May 2019 at 14:25, Andreas Dannenberg <
dannenberg@ti.com> wrote:
...
...
...
...
...
> >
> > Simon,
> >
> > On Sat, Mar 30, 2019 at 09:18:08PM +0100, Simon Goldschmidt
wrote:
...
...
...
...
...
> > > Simon Glass sjg@chromium.org schrieb am Sa., 30. März
2019, 21:06:
...
...
...
...
...
> > >
> > > > Hi Simon,
> > > >
> > > > On Wed, 27 Mar 2019 at 13:40, Simon Goldschmidt
> > > > simon.k.r.goldschmidt@gmail.com wrote:
> > > > >
> > > > > This introduces a new Kconfig option SPL_CLEAR_BSS_F.
If enabled, it
...
...
...
...
...
> > > > clears
> > > > > the bss before calling board_init_f() instead of
clearing it before
...
...
...
...
...
> > > > calling
> > > > > board_init_r().
> > > > >
> > > > > This also ensures that variables placed in BSS can be
shared between
...
...
...
...
...
> > > > > board_init_f() and board_init_r() in SPL.
> > > > >
> > > > > Such global variables are used, for example, when
loading things from FAT
...
...
...
...
...
> > > > > before SDRAM is available: the full heap required for
FAT uses global
...
...
...
...
...
> > > > > variables and clearing BSS after board_init_f() would
reset the heap
...
...
...
...
...
> > > > state.
> > > > > An example for such a usage is socfpa_arria10 where an
FPGA configuration
...
...
...
...
...
> > > > > is required before SDRAM can be used.
> > > > >
> > > > > Make the new option depend on ARM for now until more
implementations
...
...
...
...
...
> > > > follow.
> > > > >
> > > >
> > > > I still have objections to this series and I think we
should discuss
...
...
...
...
...
> > > > other ways of solving this problem.
> > > >
> > > > Does socfgpa have SRAM that could be used before SDRAM
is available?
...
...
...
...
...
> > > > If so, can we not use that for the configuration? What
various are
...
...
...
...
...
> > > > actually in BSS that are needed before board_init_r() is
called? Can
...
...
...
...
...
> > > > they not be in a struct created from malloc()?
> > > >
> > >
> > > The problem is the board needs to load an FPGA
configuration from FAT
...
...
...
...
...
> > > before SDRAM is available. Yes, this is loaded into SRAM
of course, but the
...
...
...
...
...
> > > whole code until that is done uses so many malloc/free
iterations that The
...
...
...
...
...
> > > simple mall of implementation would require too much
memory.
...
...
...
...
...
> > >
> > > And it's the full malloc state variables only that use
BSS, not the FAT
...
...
...
...
...
> > > code.
> >
> > I've actually faced very similar issues working on our TI
AM654x "System
...
...
...
...
...
> > Firmware Loader" implementation (will post upstream soon),
where I need
...
...
...
...
...
> > to load this firmware and other files from media such as
MMC/FAT in a very
...
...
...
...
...
> > memory-constrained SPL pre-relocation environment *before* I
can bring up
...
...
...
...
...
> > DDR.
> >
> > Initially, I modified the fat.c driver to re-use memory so
it is not as
...
...
...
...
...
> > wasteful during SYS_MALLOC_SIMPLE. While I'm not proud of
this solution [1]
...
...
...
...
...
> > this allowed us to get going, allowing to load multiple
files without
...
...
...
...
...
> > issues in pre-relocation SPL.
>
> That seems to point the way to a useful solution I think. We
could
...
...
...
...
...
> have a struct containing allocated pointers which is private
to FAT,
...
...
...
...
...
> and just allocate them the first time.
The board_init_f()-based loader solution we use extends beyond
MMC/FAT,
...
...
...
...
...
but also for OSPI, X/Y-Modem, and (later) USB, network, etc.
Background:
On our "TI K3" devices we need to do a whole bunch of stuff
before
...
...
...
...
...
DDR is up with limited memory, namely loading and installing a
firmware
...
...
...
...
...
that controls the entire SoC called "System Firmware". It is
only after
...
...
...
...
...
this FW is loaded from boot media and successfully started that
I can
...
...
...
...
...
bring up DDR. So all this is done in SPL board_init_f(), which
as the
...
...
...
...
...
last step brings up DDR.
Not having BSS available to carry over certain state to the
board_init_r() world would lead to a bunch of hacky changes
across
...
...
...
...
...
the board I'm afraid, more below.
This is really unfortunate.
It seems to me that we have two choises:

Hack around with board_init_f() such as to remove the

distinction
...
...
...
...
between this and board_init_r().

Enter board_init_r() without DRAM ready, and deal with setting

it up there.
...
...
...
...
I feel that the second solution is worth exploring. We could have
some
...
...
...
...
board-specific init in board_init_r(). We already have
spl_board_init() so perhaps we could have spl_early_board_init()
called right near the top?
We can refactor a few of the functions in spl/spl.c so they can be
called from board-specific code if necessary. We could also add new
flags to global_data to control the behaviour of the SPL code, and
the
...
...
...
...
board code could set these.
Let me explore this option. I can probably make something work but I
don't yet see how to do it cleanly, maybe it becomes clearer once I
put
...
...
...
some code down. Currently by definition board_init_r() has DDR
available, and much of the code is geared towards it (for example the
calling of spl_relocate_stack_gd() before entering board_init_r()
which
...
...
...
will already switch over to DDR).
Also, and not to discourage that we can't improve upon the status
quo,
...
...
...
but there is already a ton of boards using such an "early BSS"
scheme...
...
...
...
$ git grep --show-function 'memset.*bss' | grep board_init_f | wc -l
35
Yes I know :-(
We should migrate these boards to use the generic SPL framework.
socfpga_gen5 is one of the architectures listed here. I'm not even sure
whether that's actually needed. However, it's hard to test, isn't it? How
do you actually tell BSS isn't used before entering board_init_r?
One way might be to link the SPL code without the call to
board_init_r() and then check the map to make sure BSS is empty.
That would be worth a try.
...
...
To be sure, we'd need to initialize unused memory to some magic
constant and check that it has been left untouched later (on boards
where BSS is available in board_init_r and remains in place when
moving on).
I prefer a build-time check. We might even be able to automate it.
Of course a build-time check is better here than a runtime check. I just
couldn't come up with one.
I think it would be really beneficial to add such a check to all boards so
we know what we're discussing: I'll bet there are many of them just using
bss early because No one noticed...
And in board-local code, that could even be ok, it's just not ok for code
shared with boards not having access to bss early.
Regards,
Simon
...
Regards,
Simon
...
Regards,
Simon
...
...
...
...
> I wonder if that would be enough for
>
> >
> > In the quest of creating something more upstream-friendly I
had then
...
...
...
...
...
> > switched to using full malloc in pre-relocation SPL so that
I didn't
...
...
...
...
...
> > have to hack the FAT driver, encountering similar issues
like you
...
...
...
...
...
> > brought up and got this working, but ultimately abandoned
this
...
...
...
...
...
> > approach after bundling all files needed to get loaded into
a single
...
...
...
...
...
> > image tree blob which no longer required any of those
solutions.
...
...
...
...
...
> >
> > What remained till today however is a need to preserve
specific BSS
...
...
...
...
...
> > state from pre-relocation SPL over to post-relocation SPL
environment,
...
...
...
...
...
> > namely flags set to avoid the (expensive) re-probing of
peripheral
...
...
...
...
...
> > drivers by the SPL loader. For that I introduced a Kconfig
option that
...
...
...
...
...
> > allows skipping the automatic clearing of BSS during
relocation [2].
...
...
...
...
...
> >
> > Seeing this very related discussion here got me thinking
about how else
...
...
...
...
...
> > I can carry over this "state" from pre- to post relocation
but that's
...
...
...
...
...
> > probably a discussion to be had once I post my "System
Firmware Loader
...
...
...
...
...
> > Series", probably next week.
>
> Since this is SPL I don't you mean 'relocation' here. I think
you mean
...
...
...
...
...
> board_init_f() to board_init_r()?
Yes that's what I mean. AFAIK relocation in SPL is still called
relocation
...
...
...
...
...
from what I have seen working on U-boot, it just relocates gd
and stack
...
...
...
...
...
but not the actual code (personally I find it misleading calling
what SPL
...
...
...
...
...
does "relocation", but I got used to it).
> You can use global_data to store state,
I thought the idea was to stay away from gd, so we can
eventually get
...
...
...
...
...
rid of it altogether?
Not that I know of. It is how we communicate state before we have
BSS.
...
...
...
Oh ok I see now, I guess I have taken the comment [1] from
arch/arm/lib/spl.c out of context.
[1]
https://github.com/u-boot/u-boot/blob/master/arch/arm/lib/spl.c#L22
...
...
Ah yes. That is referring to putting global_data in the data section.
Perhaps we can delete that code now and see what breaks?
...
...
It is how we communicate state before we have BSS.
Understood.
...
...
> or malloc() to allocate memory and put things there.
The challenge with potentially having to incorporate such a
custom
...
...
...
...
...
solution for state preservation into several drivers as explained
earlier (SPI, USB, network, etc.) is that it does not appear to
scale
...
...
...
...
...
well. I think using BSS instead would make all those additions
cleaner
...
...
...
...
...
and simpler.
> But using BSS seems wrong to me.
I've seen you saying this a few times :)
Why?
Driver model does its own allocate of memory and this is all
attached
...
...
...
...
to the DM structures.
Drivers themselves cannot assume they are the only instance
running,
...
...
...
...
so data should be attached to their private-data pointers.
Similarly
...
...
...
...
for uclasses, if we put everything in the uclass-private data,
then we
...
...
...
...
don't need BSS and don't have any problems dealing with whether it
is
...
...
...
...
available yet. In general, BSS creates a lot of problems early in
U-Boot's execution, and we don't actually need to use it.
Yes, understood. The DM concept with private-data pointers is quite
clean from an OOP point of view.
...
If you look at the DM design you'll see that we try to avoid
malloc()
...
...
...
...
and BSS as much as possible. I suppose this series is another
example
...
...
...
...
of why :-)
...
> If you are doing something in board_init_f() in SPL that needs
BSS,
...
...
...
...
...
> can you not just move that code to board_init_r()?
I need to access media drivers in board_init_f(), for which
currently
...
...
...
...
...
I'm using BSS so I can preserve some limited state, such as that
the
...
...
...
...
...
peripheral init was done, so that it doesn't get re-done by the
actual
...
...
...
...
...
SPL loader later. board_init_r() requires DDR to be available
which I
...
...
...
...
...
can't use without doing all that work in board_init_f() first to
load/start the system controller firmware, so it's a bit of a
chicken
...
...
...
...
...
and egg issue here.
Let's try moving the egg into board_init_r()  and putting the
chicken
...
...
...
...
after it, as mentioned above.
Well I'll give this a shot.
...
...
My system firmware loader patch series is about ready and I was
planning
...
...
...
...
...
on posting it tomorrow. How about with the entire approach being
in the
...
...
...
...
...
open we use this as an opportunity to re-look at potential
alternative
...
...
...
...
...
solutions...
Sure. I hope I've explained my POV above.
Yes thanks for the review and comments.
You're welcome, and good luck with it.
Regards,
Simon