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

[Simon Glass]

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.

>
> 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.

>
> > > > 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.

>
> > 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.

>
> > 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.

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...

Regards,
Simon