[U-Boot] [PATCH] cmd_bootm: Add command line arguments to Plan 9

[Wolfgang Denk]

Dear Steven,

please always keep the mailing list on Cc: - this is especially
important for patch reviews, where such messages need to be picked up
for patchwork, too.

In message <CAGGHmKHmLAd_85SgHyC=cEUmHu8u4ENQyj3Wt3rqyVdzAtWoSw at mail.gmail.com> you wrote:
>
> > Please make this code configurable, so that people who never intend to
> > use Plan 9 do not suffer from the increased code size.
> 
> This is already done, if you look at the do_bootm_plan9 function, you'll
> see it is guarded by CONFIG_BOOTM_PLAN9. These changes only affect users
> that are booting Plan 9.

I see.  Hm... I wonder which version of U-Boot your patch is against?
The line numbers in your patch are off by at least 126 lines, and
common/cmd_bootm.c has not been touched for many months ?

> > ERROR: do not use assignment in if condition
> 
> I noticed the errors, however the style and format of my changes are the
> same as those in other functions in cmd_bootm; do_bootm_netbsd probably
> being the closest example. I did not watch to introduce style drift.

But we should not add more bad style code either.  Feel free to send a
cleanup patch for the existing code parts.  In any case, do not add
more such stuff.


> It's as unlimited as you have memory :-) That said, adjacent pages to
> CONFADDR are zeroed out at boot, so there is no possibility of overflow
> once you branch to the kernel.

Adjacent pages being zeroed - that means that you _are_ actually
limited to one page size?

> > Why do you make this (completely undocumented!!!) distinction between
> > "bootm" being used with one or more arguments?  Why can you not
> > simply _always_ use bootargs ?
> 
> This is to support passing arguments via bootm. This behavior is consistent
> with NetBSD.

...which I consider a really weird desiign that IMO should not be
followed without need.

If you decide to do so nevertheless, then please

1) document the behaviour
2) factor out the common code instead of copying it

> > What if the user did not set the "confaddr" environment variable?
> > Then the memory reagion there is left uninitialized?  Does this not
> > cause undefined behaviour when booting Plan 9?
> 
> There are checks which account for uninitialized memory at boot. It's ugly,
> but it's how the OS deals with configuration. I don't like it either.

How does it detect if there are valid arguments (versus random crap)
in the memory range starting at "confaddr"?  I can see no checksum or
similar?

> And how does Plan 9 learn where to find this date? I cannot see how
> > you pass this address on to Plan 9?
> 
> Like most things in Plan 9, it is a compiled offset (defined in mem.h).
> CONFADDR is fixed, so as long as the configuration is dumped to the right
> location (which can change between kernels), it will work.

But then makes no sense to use a "confaddr" environment variable for
this - the user has no real choice of setting this variable: either it
matches the fixed CONFADDR value, in which case it works, or it is
different, in which case it will silently fail.  This is bad.
I think you should use a CONFIG_SYS_CONFADDR constant instead.

> Even worse - this code is actually pretty dangerous: "confaddr" is
> > neither a reserved name, nor is it in any way exotic enough to be sure
> > nobody else would use this in his environment.  But if somebody does,
> > he will suddenly find that some (for him random) data is copied unex-
> > pectedly to that memory range.  This may cause nasty crashes or other
> > undefined behaviour which is very difficult to debug.
> 
> True, but it's no different than loadaddr. This only affects Plan 9 users,
> and this behavior will be documented for the kernels which support U-Boot.
> I don't think there is too much danger here, though documentation will be
> important.

It is not documented in U-Boot.  But actually I think this is void -
using an envrionment variable here makes no sense in the first place.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To be is to program.