[U-Boot] [PATCH] cmd_bootm: Add command line arguments to Plan 9
This patch introduces support for command line arguments to Plan 9. Plan 9 generally dedicates a small region of kernel memory (known as CONFADDR) for runtime configuration. A new environment variable named confaddr was introduced to indicate this location when copying arguments.
Signed-off-by: Steven Stallion sstallion@gmail.com --- common/cmd_bootm.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 05130b6..5c62271 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1533,6 +1533,7 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[], bootm_headers_t *images) { void (*entry_point)(void); + char *s;
if ((flag != 0) && (flag != BOOTM_STATE_OS_GO)) return 1; @@ -1544,6 +1545,24 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[], } #endif
+ if ((s = getenv("confaddr")) != NULL) { + char *confaddr = (char *)simple_strtoul(s, NULL, 16); + + if (argc > 2) { + int i; + + s = confaddr; + for (i = 2; i < argc; i++) { + if (i > 2) + *s++ = '\n'; + strcpy(s, argv[i]); + s += strlen(argv[i]); + } + } else if ((s = getenv("bootargs")) != NULL) { + strcpy(confaddr, s); + } + } + entry_point = (void (*)(void))images->ep;
printf("## Transferring control to Plan 9 (at address %08lx) ...\n",
Dear Steven Stallion,
In message 1370563140-31368-1-git-send-email-sstallion@gmail.com you wrote:
This patch introduces support for command line arguments to Plan 9. Plan 9 generally dedicates a small region of kernel memory (known as CONFADDR) for runtime configuration. A new environment variable named confaddr was introduced to indicate this location when copying arguments.
Please make this code configurable, so that people who never intend to use Plan 9 do not suffer from the increased code size.
Then please run your patch through ckeckpatch - as is, this reports two errors:
ERROR: do not use assignment in if condition
Finally:
- if ((s = getenv("confaddr")) != NULL) {
char *confaddr = (char *)simple_strtoul(s, NULL, 16);if (argc > 2) {int i;s = confaddr;for (i = 2; i < argc; i++) {if (i > 2)*s++ = '\n';strcpy(s, argv[i]);s += strlen(argv[i]);}} else if ((s = getenv("bootargs")) != NULL) {strcpy(confaddr, s);}- }
This design looks pretty much inconsistent and non-intuitive to me.
There is absolutely no length checking in this code. Is the size of the available area truely unlimited? I doubt that...
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 ?
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?
And how does Plan 9 learn where to find this date? I cannot see how you pass this address on to Plan 9?
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.
Best regards,
Wolfgang Denk
This patch introduces support for command line arguments to Plan 9. Plan 9 generally dedicates a small region of kernel memory (known as CONFADDR) for runtime configuration. A new environment variable named confaddr was introduced to indicate this location when copying arguments.
Signed-off-by: Steven Stallion sstallion@gmail.com --- Changes for v2: - Corrected checkpatch misses - Refactored common code to copy command line arguments - Added documentation for odd Plan 9 behavior
common/cmd_bootm.c | 36 +++++++++++++++++++++++++++++------- doc/README.plan9 | 18 ++++++++++++++++++ 2 files changed, 47 insertions(+), 7 deletions(-) create mode 100644 doc/README.plan9
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 05130b6..960a68f 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1346,6 +1346,19 @@ static void fixup_silent_linux(void) } #endif /* CONFIG_SILENT_CONSOLE */
+#if defined(CONFIG_BOOTM_NETBSD) || defined(CONFIG_BOOTM_PLAN9) +static void copy_args(char *dest, int argc, char * const argv[], char delim) +{ + int i; + + for (i = 0; i < argc; i++) { + if (i > 0) + *dest++ = delim; + strcpy(dest, argv[i]); + dest += strlen(argv[i]); + } +} +#endif
/*******************************************************************/ /* OS booting routines */ @@ -1408,13 +1421,7 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[], for (i = 2, len = 0; i < argc; i += 1) len += strlen(argv[i]) + 1; cmdline = malloc(len); - - for (i = 2, len = 0; i < argc; i += 1) { - if (i > 2) - cmdline[len++] = ' '; - strcpy(&cmdline[len], argv[i]); - len += strlen(argv[i]); - } + copy_args(cmdline, argc-2, argv+2, ' '); } else if ((cmdline = getenv("bootargs")) == NULL) { cmdline = ""; } @@ -1533,6 +1540,7 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[], bootm_headers_t *images) { void (*entry_point)(void); + char *s;
if ((flag != 0) && (flag != BOOTM_STATE_OS_GO)) return 1; @@ -1544,6 +1552,20 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[], } #endif
+ /* See README.plan9 */ + s = getenv("confaddr"); + if (s != NULL) { + char *confaddr = (char *)simple_strtoul(s, NULL, 16); + + if (argc > 2) { + copy_args(confaddr, argc-2, argv+2, '\n'); + } else { + s = getenv("bootargs"); + if (s != NULL) + strcpy(confaddr, s); + } + } + entry_point = (void (*)(void))images->ep;
printf("## Transferring control to Plan 9 (at address %08lx) ...\n", diff --git a/doc/README.plan9 b/doc/README.plan9 new file mode 100644 index 0000000..2d3d0e0 --- /dev/null +++ b/doc/README.plan9 @@ -0,0 +1,18 @@ +Plan 9 from Bell Labs kernel images require additional setup to pass +configuration information to the kernel. An environment variable named +confaddr must be defined with the same value as CONFADDR (see mem.h). +Use of this facility is optional, but should be preferable to manual +configuration. + +When booting an image, arguments supplied to the bootm command will be +copied to CONFADDR. If no arguments are specified, the contents of the +bootargs environment variable will be copied. + +If no command line arguments or bootargs are defined, CONFADDR is left +uninitialized to permit manual configuration. For example, PC-style +configuration could be simulated by issuing a fatload in bootcmd: + + # setenv bootcmd fatload mmc 0 $confaddr plan9.ini; ...; bootm + +Steven Stallion +June 2013
This patch introduces support for command line arguments to Plan 9. Plan 9 generally dedicates a small region of kernel memory (known as CONFADDR) for runtime configuration. A new environment variable named confaddr was introduced to indicate this location when copying arguments.
Signed-off-by: Steven Stallion sstallion@gmail.com --- Changes for v2: - Corrected checkpatch misses - Refactored common code to copy command line arguments - Added documentation for odd Plan 9 behavior Changes for v3: - Corrected patch Subject
common/cmd_bootm.c | 36 +++++++++++++++++++++++++++++------- doc/README.plan9 | 18 ++++++++++++++++++ 2 files changed, 47 insertions(+), 7 deletions(-) create mode 100644 doc/README.plan9
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 05130b6..960a68f 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1346,6 +1346,19 @@ static void fixup_silent_linux(void) } #endif /* CONFIG_SILENT_CONSOLE */
+#if defined(CONFIG_BOOTM_NETBSD) || defined(CONFIG_BOOTM_PLAN9) +static void copy_args(char *dest, int argc, char * const argv[], char delim) +{ + int i; + + for (i = 0; i < argc; i++) { + if (i > 0) + *dest++ = delim; + strcpy(dest, argv[i]); + dest += strlen(argv[i]); + } +} +#endif
/*******************************************************************/ /* OS booting routines */ @@ -1408,13 +1421,7 @@ static int do_bootm_netbsd(int flag, int argc, char * const argv[], for (i = 2, len = 0; i < argc; i += 1) len += strlen(argv[i]) + 1; cmdline = malloc(len); - - for (i = 2, len = 0; i < argc; i += 1) { - if (i > 2) - cmdline[len++] = ' '; - strcpy(&cmdline[len], argv[i]); - len += strlen(argv[i]); - } + copy_args(cmdline, argc-2, argv+2, ' '); } else if ((cmdline = getenv("bootargs")) == NULL) { cmdline = ""; } @@ -1533,6 +1540,7 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[], bootm_headers_t *images) { void (*entry_point)(void); + char *s;
if ((flag != 0) && (flag != BOOTM_STATE_OS_GO)) return 1; @@ -1544,6 +1552,20 @@ static int do_bootm_plan9(int flag, int argc, char * const argv[], } #endif
+ /* See README.plan9 */ + s = getenv("confaddr"); + if (s != NULL) { + char *confaddr = (char *)simple_strtoul(s, NULL, 16); + + if (argc > 2) { + copy_args(confaddr, argc-2, argv+2, '\n'); + } else { + s = getenv("bootargs"); + if (s != NULL) + strcpy(confaddr, s); + } + } + entry_point = (void (*)(void))images->ep;
printf("## Transferring control to Plan 9 (at address %08lx) ...\n", diff --git a/doc/README.plan9 b/doc/README.plan9 new file mode 100644 index 0000000..2d3d0e0 --- /dev/null +++ b/doc/README.plan9 @@ -0,0 +1,18 @@ +Plan 9 from Bell Labs kernel images require additional setup to pass +configuration information to the kernel. An environment variable named +confaddr must be defined with the same value as CONFADDR (see mem.h). +Use of this facility is optional, but should be preferable to manual +configuration. + +When booting an image, arguments supplied to the bootm command will be +copied to CONFADDR. If no arguments are specified, the contents of the +bootargs environment variable will be copied. + +If no command line arguments or bootargs are defined, CONFADDR is left +uninitialized to permit manual configuration. For example, PC-style +configuration could be simulated by issuing a fatload in bootcmd: + + # setenv bootcmd fatload mmc 0 $confaddr plan9.ini; ...; bootm + +Steven Stallion +June 2013
On Mon, Jun 10, 2013 at 01:00:09AM -0700, Steven Stallion wrote:
This patch introduces support for command line arguments to Plan 9. Plan 9 generally dedicates a small region of kernel memory (known as CONFADDR) for runtime configuration. A new environment variable named confaddr was introduced to indicate this location when copying arguments.
Signed-off-by: Steven Stallion sstallion@gmail.com
With a slight re-factor due to some changes from Simon Glass, applied to u-boot/master, thanks!
participants (3)
-
Steven Stallion -
Tom Rini -
Wolfgang Denk