[PATCH v8 00/13] bootstd: Convert rockchip and add various fixes and tweaks

[Simon Glass]

Hi Tom,

On Sun, 9 Apr 2023 at 18:07, Tom Rini <trini at konsulko.com> wrote:
>
> On Sun, Apr 09, 2023 at 08:18:31AM +1200, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sun, 9 Apr 2023 at 03:14, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Sat, Apr 08, 2023 at 12:08:37PM +1200, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sat, 8 Apr 2023 at 09:55, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Sat, Apr 08, 2023 at 09:35:48AM +1200, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Sat, 8 Apr 2023 at 08:22, Tom Rini <trini at konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sat, Apr 08, 2023 at 07:53:10AM +1200, Simon Glass wrote:
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > On Sat, 8 Apr 2023 at 07:39, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Apr 07, 2023 at 10:36:38PM +1200, Simon Glass wrote:
> > > > > > > > >
> > > > > > > > > > This series converts rockchip boards over to use standard boot. It also
> > > > > > > > > > fixes various problems which have come up recently, showing differences
> > > > > > > > > > between the current implementation and the distroboot scripts.
> > > > > > > > > >
> > > > > > > > > > This should get us closer to being able to turn down the scripts.
> > > > > > > > >
> > > > > > > > > Alright, so I grabbed a few parts of this series to investigate the
> > > > > > > > > points I'm trying to grasp better, and I think this is going the wrong
> > > > > > > > > track. We should start off by dropping "default y" from BOOTSTD, and
> > > > > > > > > then start adding "default y if" for SoCs as we convert them. The end
> > > > > > > > > goal should be that we get to the point where we can "default y if ARM
> > > > > > > > > || RISCV || X86" or perhaps "default y !(PPC || M68K || ...)" as it's
> > > > > > > > > just a few architectures that haven't ended up being converted. But
> > > > > > > > > today, there's too much churn on platforms that aren't making any use of
> > > > > > > > > this. And I don't think this is going to be functionally worse than all
> > > > > > > > > of the places we "imply DISTRO_DEFAULTS" today, as functionally we can
> > > > > > > > > replace that with "imply BOOTSTD" as they get migrated.
> > > > > > > >
> > > > > > > > That would really be a backward step. I'm not sure what to say at this
> > > > > > > > point. I've put a lot of effort in trying to get this over the line,
> > > > > > > > but the only way we get feedback is when it is applied.
> > > > > > >
> > > > > > > Having bootstd enabled and not functional (because boot_targets aren't
> > > > > > > set) isn't helping the migration happen. And the hard part of the
> > > > > > > migration isn't knowing it's possible, or enabling 1-2 options, it's
> > > > > > > testing it and also it really just being 1-2 options.
> > > > > >
> > > > > > Standard boot does not need boot_targets to be set. It works fine
> > > > > > without it. It just goes through the boot devices in a pre-defined
> > > > > > order, from fastest to slowest. It matches what most boards do anyway.
> > > > > > The main reason we kept it is for compatibility with distro boot.
> > > > >
> > > > > What most boards do today is just sit at the prompt and wait for input,
> > > > > which this changes, which is part of the big source of size churn here.
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > > I don't think testing in advance is a feasible approach in general.
> > > > > > See for example the rpi series which hasn't got any comment. It likely
> > > > > > won't until it is applied. That's how we get feedback. We have months
> > > > > > to resolve issues and I believe that the code is fundamentally sound.
> > > > >
> > > > > We need some spot testing here and there to see how things react and how
> > > > > people use it. You found a lot of things with just rk3399, and now the
> > > > > rest of rockchip looks to be fairly direct. You found more things doing
> > > > > x86. When you can convert some other SoC and the change is just dropping
> > > > > distro_bootcmd and calling bootflow scan instead (or that + setting the
> > > > > order again), that'll be good.
> > > >
> > > > So long as we are aware that we generally only find problems when
> > > > patches land, yes. The QEMU stuff is easier since it doesn't need a
> > > > board. It's also not all that useful in the real world :-) I'd like to
> > > > try a programmatic conversion, too, although I haven't looked at it
> > > > yet.
> > >
> > > Well, I think you're misjudging when we get most of the testing done.
> > > It's at that last one or two -rc points where it seems people test
> > > things, and surprises are very much not welcome then, which is why I
> > > want more unit testing of things like this.  Especially as we're just
> > > getting started on the conversions.
> >
> > The unit tests are in test/boot and there's quite a bit. But it does
> > not cover some of the minor details though, e.g. how the fdt file is
> > selected. It can certainly be added, once we know what the correct
> > behaviour is. I think it needs more users, before I can tell how close
> > we are.
> >
> > The nice thing with bootstd is that we can create unit tests for this
> > behaviour. That isn't possible with the scripts.
>
> Yes, writing tests for the code is good.  This is not a substitute for
> booting up assorted previously functional OS images / installers on the
> platform. That's how we ran in to the rk3399 problems.

Yes. Really we should get these into some sort of CI.

>
> > > > > But I'm not sure that changing the platforms that don't today opt-in to
> > > > > distro_bootcmd (which has been a thing for a long time now) to force
> > > > > opt-in to this is the right call. It might be in some cases (mediatek
> > > > > maybe? Or maybe no, everyone does android of some flavour so a different
> > > > > bootstd option) but not others (those *_evm_r5_defconfig boards).
> > > >
> > > > Fair enough, so long as we actually turn down distro_bootcmd. So long
> > > > as it is still there, boards will enable it. See SPL_FIT_GENERATOR.
> > >
> > > Yes, it will take follow-through to get everyone converted, and making
> > > sure the new tool covers all the use cases.
> > >
> > > > > > > > What churn are you seeing? Do you mean:
> > > > > > > >
> > > > > > > > disable BOOTSTD for boards with custom commands? You asked for that patch
> > > > > > > > disabling BOOTSTD_DEFAULTS? You asked for that patch
> > > > > > > > enable BOOTSTD_DEFAULTS by default? We can drop it if you like
> > > > > > >
> > > > > > > We need all 3 of those patches because without the 3rd you don't get a
> > > > > > > good experience when you do enable bootstd for a platform. The problem
> > > > > > > is that unless you have distro_bootcmd today you're getting between 63kB
> > > > > > > (a lot of mediatek platforms) and 5k (silk, as a semi-random example) of
> > > > > > > growth because bootstd is on and now is the default bootcmd, when before
> > > > > > > they had nothing. And probably had board docs saying "now do ... to
> > > > > > > boot". And that's largely setting aside the *_r5_* platforms that I know
> > > > > > > are just doing something else, and could disable it.
> > > > > >
> > > > > > Er, I thought you wanted it to default on if the boards has no
> > > > > > bootcmd? If not, we can disable it for those as well. If you don't
> > > > > > want any increase we can disable it for boards without DISTRO_DEFAULTS
> > > > > > too. After all, presumably those boards are doing something custom
> > > > > > anyway.
> > > > >
> > > > > I think I might have originally, but now that I'm looking at the results
> > > > > it was too optimistic. Every branch I build I look at the per-board
> > > > > breakdown, not just the summary. And it's too much on all of these
> > > > > platforms that had no default bootcmd today.
> > > >
> > > > OK. I don't mind about that. We've ended up in a bit of a rabbit hole I think.
> > > >
> > > > >
> > > > > > > We want to convert everyone doing distro_bootcmd over to this, that's
> > > > > > > good. The problem is we don't have a symbol today that means "we want
> > > > > > > distro_bootcmd" and also isn't overloaded (DISTRO_DEFAULTS is overloaded
> > > > > > > in this sense).
> > > > > > >
> > > > > > > The wrong direction part of this series is that for platforms that
> > > > > > > aren't in the middle of converting we're increasing their size between
> > > > > > > somewhat and very very much, and we haven't tested that it'll work. And
> > > > > > > yes, there's some automatic guessing logic, which hasn't been tested on
> > > > > > > these platforms either, so we don't actually know if going from no
> > > > > > > bootcmd (and so drops to prompt) to attempts to autoboot something is an
> > > > > > > improvement.
> > > > > >
> > > > > > So, the wrong direction comes from the last three patches. Is that right?
> > > > >
> > > > > The wrong direction comes from enabling bootstd (and so, a bootcmd and
> > > > > distro boot) on platforms that just sit at the prompt today.  We are not
> > > > > making a good heuristic guess at what the should be doing.  Reaching out
> > > > > to the maintainers to get them to do the conversion, especially once
> > > > > it's just a matter for most of them of just enabling bootstd and then
> > > > > distro, if they want that or once bootmeth android gets done, for those
> > > > > platforms) is how we get them moved smoothly.
> > > >
> > > > We should know which boards sit at the prompt today, by the fact that
> > > > they don't have a bootcmd. Presumably that is the way the maintainer
> > > > wants it, so I agree we should avoid changing it.
> > >
> > > Well, doing:
> > > for C in `(cd configs;ls)`;do make -sj $C && mv .config configs/$C;done
> > > to give me everyones full config to browse, there's 564 boards calling
> > > distro_bootcmd, but 465 of them are direct "run distro_bootcmd" and
> > > nothing more. Those are the ones that'll be easy to migrate, once we've
> > > got another migration or two done.
> >
> > Hmm it would be nice if moveconfig let you summarise values of CONFIG vars.
>
> Could be interesting, could be too much work to bother with, hard to
> say.

I'm not sure either, but anyway we are not there yet.

>
> > Some of these will need tweaking, e.g. to replace the custom command
> > with something that uses bootstd.
>
> Well, yes and no?  They'll require looking at the use case and
> understanding what's going on. We've got some platforms that need to
> make sure we have fdtfile/uuid set (how are we doing that today in the
> generic distro bootmeth?) and others are "try distro, then try android",
> which means we need the android bootmeth to exist. Others still are a
> fall back.

Makes sense. I wonder if someone will write an Android bootmeth?

>
> > > > > > Fundamentally the problem I have is that I know where I would like
> > > > > > this to head, which is everything using standard boot and turning down
> > > > > > the scripts. But it feels like every time I touch bootstd we have to
> > > > > > have the EFI discussion again. You can imagine how I feel about
> > > > > > disabling BOOTSTD by default...it would basically kill it.
> > > > >
> > > > > Well, we enabled bootstd by default too quickly perhaps then, and just
> > > > > like we narrowed down EFI_LOADER defaults, we need to narrow this down
> > > > > until it's easily convertable.
> > > >
> > > > It feels like it took a year to get that moving. There was so much EFI
> > > > discussion that I really don't want to revisit.
> > >
> > > Yes, and there too I started off with "well, this works for everyone,
> > > so, OK" and then saw that "Well, OK, a lot of things don't need/want
> > > that, really".
> > >
> > > > > > This is not really an arch-specific thing, nor an SoC-specific thing.
> > > > > > The underlying logic is the same for everything. The reason I think we
> > > > > > need to do a few cases before we enable it everywhere is that we need
> > > > > > to find the little tweaks needed in that logic.
> > > > >
> > > > > It's a generic framework to a board specific thing.
> > > > >
> > > > > > How about we apply the first patches in this series, skipping the last
> > > > > > three, then apply the rpi series as well. That should get people
> > > > > > actually using it and we can iron out the problems. It also keeps
> > > > > > things moving. We have months before the release.
> > > > > >
> > > > > > Enabling by default can come later once we decide what we want to do
> > > > > > about size increases, boards that don't use DISTRO_DEFAULTS and boards
> > > > > > that don't have a boot command.
> > > > >
> > > > > How about disabling it by default and imply'ing it for everything that
> > > > > implies/selects DISTRO_DEFAULTS today, since the part of bootstd that is
> > > > > done is the distro bootmeth?
> > > >
> > > > I'd really like to move forwards, instead of creating another barrier
> > > > to this migration. There was a huge amount of work in making sure that
> > > > the incremental size of bootstd was small, so it could be cheaply
> > > > enabled. This all went off the rails because, as you correctly pointed
> > > > out, enabling the bootstd commands does ~nothing if there is no actual
> > > > boot command set. I wish I had just stopped then to clarify the goals,
> > > > because this has all burned a lot of time and energy.
> > > >
> > > > From my side the best thing to do would be to get the currently
> > > > outstanding migrations into -master ASAP so people can try them out.
> > > > Despite the delays we still have months left for testing on this
> > > > release. Then I (or perhaps maintainers??) can work on some new ones
> > > > for -next when that opens.
> > >
> > > Yes, we can take the fixes in this series.  And yes, we can take
> > > finishing moving the rest of rockchip over, since I put in the line to
> > > make sure we still grab all of the defaults generic distro needs. And I
> > > am hopeful Peter will have the time to test the Pi conversion on all the
> > > hardware he wants to test it on (along with booting up whatever OS
> > > combinations).
> > >
> > > Another one of those big chunk of boards is the sunxi families, if you
> > > want to find some boards and boot some distros.
> >
> > OK. Let's get rpi in and I can look at sunxi for -next
> >
> > >
> > > > Once we get to the point where every bootstd series doesn't raise a
> > > > discussion about EFI, I will feel a lot more comfortable about
> > > > changing defaults. I hope you can understand that...
> > >
> > > Well, that's going to be the case until you've handed bootmeth_distro
> > > off to someone else, most likely.  Generic distro boot means EFI boot.
> > > And I only mentioned EFI here as an example of should have pushed back
> > > on "enable this for everyone" sooner.
> >
> > Would someone like to take it over? Note that bootmeth_distro is just
> > the extlinux.conf stuff.
> >
> > >
> > > Because that's the key here, on the 564 platforms that use
> > > distro_bootcmd today, there shouldn't be any growth when we switch to
> > > bootstd, and there's even more platforms that enable
> > > CONFIG_DISTRO_DEFAULTS than there are that use distro_bootcmd so the
> > > cases like xilinx mini should be the exception, not the rule.  And
> > > that's what I keep circling back to. The logic to keep the defaults that
> > > generic distro support needs are (almost) always already set on the
> > > platforms that are using generic distro boot, so we shouldn't grow in
> > > size when bootstd turns it on.  And if I'm changing my mind about
> > > forcing this on, on platforms that hadn't been doing generic distro
> > > before, I guess I'm changing my mind, sorry, there's too many platforms
> > > that are doing other things (not generic distro or Android or Special
> > > Things).
> >
> > Yes I agree there should be no size growth for platforms which use
> > distro_bootcmd today and move to bootstd. That is easy enough to check
> > as part of migration. For the rpi and rockchip series the size reduces
> > from where we are today by 1-12KB.
>
> It shouldn't change it at all tho, let alone reduce? Why is it reducing?



>
> > Re the bootstd default, we should talk about that.
>
> Yes.
>
> > So basically, the plan for migrations is something like this:
> > - switch over some group of boards (change from DISTRO_DEFAULTS to
> > BOOTSTD_DEFAULTS in the process)
>
> Yes.
>
> > - test by booting various distros to see if there are new corner cases
> > (do we have a list?)
>
> Specifically? No, just distrowatch for random ones outside of the big
> common ones. We don't need to test everything of course.

OK

>
> > - check there is no size growth
> > - send patches
> > - discussion will focus on code review and testing
> >
> > Does that sound right?
>
> I think so, yes.

OK good.

>
> > Do you think maintainers might be interested in migrating their boards?
>
> I know some are, yes. But it will also require figuring out what and how
> to extend what we have today. Look at configs/j721e_evm_a72_defconfig
> which is first run distro boot, if that fails then we have a more
> advanced OS and so initialize all of the other procs (remoteproc stuff,
> in tree already) and start that. Moving all of that to bootstd is a
> want, but I don't know if doc/develop/bootstd.rst is sufficient today
> for someone at TI to pick up and run with it, do you?

Oh wow that is really doing a lot of stuff. Unfortunately it is
relying on pieces of the distro boot script. It needs a look.

>
> > Anyway, I'll resend the series without the last three patches which
> > generated this discussion.
> >
> > As above I also see a bit of a gap in testing which would be good to
> > plug. The bootmeths rely on CI to test with two faked OS images. But
> > we might be able to add unit tests for these, with a bit of thought. I
> > will see what I can do.
>
> OK. And once we have stuff converted that's in my local farm, at least
> an occasional don't interrupt and just autoboot to whatever-I-installed
> should happen.

Yes I do that every now and then but I don't have many distros
installed - I think three.

Regards,
Simon