[U-Boot] [PATCH 0/6] Allow sandbox to use config_distro_bootcmd
Testing whether images will correctly boot with the standard distro bootcmds can be rather time-consuming as it tends to require flashing the images and booting on a device. Ditto for testing changes to config_distro_bootcmd.
Adding support for sandbox to run distro bootcmds makes things a lot more convenient.
Sjoerd Simons (6): sandbox: only do sandboxfs for hostfs interface sandbox: Add support for bootz sandbox: Implement host dev [device] config_distro_bootcmd.h: Add shared block definition for the host interface pxe: Ensure all memory access is to mapped memory sandbox: add config_distro_defaults and config_distro_bootcmd
arch/sandbox/cpu/cpu.c | 20 +++++++++++ common/cmd_pxe.c | 80 +++++++++++++++++++++++------------------ common/cmd_sandbox.c | 48 +++++++++++++++++++++++++ fs/sandbox/sandboxfs.c | 5 ++- include/config_distro_bootcmd.h | 13 +++++++ include/configs/sandbox.h | 29 +++++++++++++-- 6 files changed, 157 insertions(+), 38 deletions(-)
Only do sandbox filesystem access when using the hostfs device interface, rather then falling back to it in all cases. This prevents confusion situations due to the fallback being taken rather then an unsupported error being raised.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk --- fs/sandbox/sandboxfs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c index a920bc0..c6540c6 100644 --- a/fs/sandbox/sandboxfs.c +++ b/fs/sandbox/sandboxfs.c @@ -10,7 +10,10 @@
int sandbox_fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info) { - return 0; + /* Only accept a NULL block_dev_desc_t for the sandbox, which is when + * hostfs interface is used + */ + return rbdd != NULL; }
int sandbox_fs_read_at(const char *filename, loff_t pos, void *buffer,
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Only do sandbox filesystem access when using the hostfs device interface, rather then falling back to it in all cases. This prevents confusion situations due to the fallback being taken rather then an unsupported error being raised.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
Reviewed-by: Simon Glass sjg@chromium.org
See nit below.
fs/sandbox/sandboxfs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c index a920bc0..c6540c6 100644 --- a/fs/sandbox/sandboxfs.c +++ b/fs/sandbox/sandboxfs.c @@ -10,7 +10,10 @@
int sandbox_fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info) {
return 0;
/* Only accept a NULL block_dev_desc_t for the sandbox, which is when
/* * Only accept
* hostfs interface is used*/return rbdd != NULL;}
int sandbox_fs_read_at(const char *filename, loff_t pos, void *buffer,
2.1.4
Regards, Simon
Hi Sjoerd,
On 20 February 2015 at 12:22, Simon Glass sjg@chromium.org wrote:
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Only do sandbox filesystem access when using the hostfs device interface, rather then falling back to it in all cases. This prevents confusion situations due to the fallback being taken rather then an unsupported error being raised.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
Reviewed-by: Simon Glass sjg@chromium.org
See nit below.
fs/sandbox/sandboxfs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c index a920bc0..c6540c6 100644 --- a/fs/sandbox/sandboxfs.c +++ b/fs/sandbox/sandboxfs.c @@ -10,7 +10,10 @@
int sandbox_fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info) {
return 0;
/* Only accept a NULL block_dev_desc_t for the sandbox, which is when/*
- Only accept
* hostfs interface is used*/return rbdd != NULL;}
int sandbox_fs_read_at(const char *filename, loff_t pos, void *buffer,
2.1.4
Can you please respin this series? I would like to apply it to a -next tree.
Regards, Simon
Add dummy bootz_setup implementation allowing the u-boot sandbox to run bootz. This recognizes both ARM and x86 zImages to validate a valid zImage was loaded.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk --- arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c index 1aa397c..c71fb86 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -7,6 +7,7 @@ #include <dm/root.h> #include <os.h> #include <asm/state.h> +#include <asm/io.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -40,6 +41,25 @@ unsigned long __attribute__((no_instrument_function)) timer_get_us(void) return os_get_nsec() / 1000; }
+int bootz_setup(ulong image, ulong *start, ulong *end) +{ + uint8_t *zimage = (uint8_t *)map_sysmem(image, 0); + uint8_t arm_magic[] = { 0x18, 0x28, 0x6f, 0x01 }; + + if (memcmp(zimage + 0x202, "HdrS", 4) == 0) { + printf ("setting up x86 zImage\n"); + } else if (memcmp (zimage + 0x24, arm_magic, 4) == 0) { + printf ("setting up ARM zImage\n"); + } else { + printf ("Unrecognized zImage\n"); + return 1; + } + + *start = 0xdead; + *end = 0xbeef; + return 0; +} + int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) { if (flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)) {
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Add dummy bootz_setup implementation allowing the u-boot sandbox to run bootz. This recognizes both ARM and x86 zImages to validate a valid zImage was loaded.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
Can you please split the do_bootm_linux() code out into arch/sandbox/lib/bootm, and add your patch there?
1 file changed, 20 insertions(+)
diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c index 1aa397c..c71fb86 100644 --- a/arch/sandbox/cpu/cpu.c +++ b/arch/sandbox/cpu/cpu.c @@ -7,6 +7,7 @@ #include <dm/root.h> #include <os.h> #include <asm/state.h> +#include <asm/io.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -40,6 +41,25 @@ unsigned long __attribute__((no_instrument_function)) timer_get_us(void) return os_get_nsec() / 1000; }
+int bootz_setup(ulong image, ulong *start, ulong *end) +{
- uint8_t *zimage = (uint8_t *)map_sysmem(image, 0);
Do you need the cast?
- uint8_t arm_magic[] = { 0x18, 0x28, 0x6f, 0x01 };
- if (memcmp(zimage + 0x202, "HdrS", 4) == 0) {
- printf ("setting up x86 zImage\n");
- } else if (memcmp (zimage + 0x24, arm_magic, 4) == 0) {
- printf ("setting up ARM zImage\n");
- } else {
The indentation and code style looks wrong here. Did you run patman?
- printf ("Unrecognized zImage\n");
- return 1;
- }
- *start = 0xdead;
- *end = 0xbeef;
- return 0;
+}
int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) { if (flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)) { -- 2.1.4
Regards, Simon
A common pattern to check if a certain device exists (e.g. in config_distro_bootcmd) is to use: <interface> dev [device]
Implement host dev [device] so this pattern can be used for sandbox host devices.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk --- common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c index 4286969..7447d43 100644 --- a/common/cmd_sandbox.c +++ b/common/cmd_sandbox.c @@ -10,6 +10,8 @@ #include <sandboxblockdev.h> #include <asm/errno.h>
+static int sandbox_curr_device = -1; + static int do_sandbox_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { @@ -124,3 +126,49 @@ U_BOOT_CMD( "sb commands use the "hostfs" device. The "host" device is used\n" "with standard IO commands such as fatls or ext2load" ); + + +static int do_host_dev(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int dev; + char *ep; + block_dev_desc_t *blk_dev; + int ret; + + if (argc < 2 || strcmp(argv[1], "dev")) + return CMD_RET_USAGE; + + if (argc == 2) { + if (sandbox_curr_device < 0) { + printf("No current host device\n"); + return 1; + } + printf ("Current host device %d\n", sandbox_curr_device); + return 0; + } + + dev = simple_strtoul(argv[2], &ep, 16); + if (*ep) { + printf("** Bad device specification %s **\n", argv[2]); + return CMD_RET_USAGE; + } + + ret = host_get_dev_err(dev, &blk_dev); + if (ret) { + if (ret == -ENOENT) + puts("Not bound to a backing file\n"); + else if (ret == -ENODEV) + puts("Invalid host device number\n"); + + return 1; + } + + sandbox_curr_device = dev; + return 0; +} + +U_BOOT_CMD( + host, 3, 2, do_host_dev, + "Set or get current host device", "dev [dev] - Set or retrieve the current host device" +);
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
A common pattern to check if a certain device exists (e.g. in config_distro_bootcmd) is to use: <interface> dev [device]
Implement host dev [device] so this pattern can be used for sandbox host devices.
I don't see where this actually affects anything? In other words, does it really use the device you select, or just ignore you?
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c index 4286969..7447d43 100644 --- a/common/cmd_sandbox.c +++ b/common/cmd_sandbox.c @@ -10,6 +10,8 @@ #include <sandboxblockdev.h> #include <asm/errno.h>
+static int sandbox_curr_device = -1;
static int do_sandbox_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { @@ -124,3 +126,49 @@ U_BOOT_CMD( "sb commands use the "hostfs" device. The "host" device is used\n" "with standard IO commands such as fatls or ext2load" );
+static int do_host_dev(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
int dev;char *ep;block_dev_desc_t *blk_dev;int ret;if (argc < 2 || strcmp(argv[1], "dev"))return CMD_RET_USAGE;if (argc == 2) {if (sandbox_curr_device < 0) {printf("No current host device\n");return 1;}printf ("Current host device %d\n", sandbox_curr_device);return 0;}dev = simple_strtoul(argv[2], &ep, 16);if (*ep) {printf("** Bad device specification %s **\n", argv[2]);return CMD_RET_USAGE;}ret = host_get_dev_err(dev, &blk_dev);if (ret) {if (ret == -ENOENT)puts("Not bound to a backing file\n");
Just use printf(), we should avoid puts() now.
else if (ret == -ENODEV)puts("Invalid host device number\n");return 1;}sandbox_curr_device = dev;return 0;+}
+U_BOOT_CMD(
host, 3, 2, do_host_dev,"Set or get current host device", "dev [dev] - Set or retrieve the current host device"
Can we make this command part of the 'sb' command? Like 'sh host ...'.
+);
2.1.4
Regards, Simon
On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
A common pattern to check if a certain device exists (e.g. in config_distro_bootcmd) is to use: <interface> dev [device]
Implement host dev [device] so this pattern can be used for sandbox host devices.
I don't see where this actually affects anything? In other words, does it really use the device you select, or just ignore you?
It gets ignored, it's only real usage is to detect whether a device exists.
To step back the reason I implemented it here is just to make it simpler to integrate with the command boot commands as it's common blockdevice macro is:
#define BOOTENV_SHARED_BLKDEV_BODY(devtypel) \ "if " #devtypel " dev ${devnum}; then " \ "setenv devtype " #devtypel "; " \ "run scan_dev_for_boot_part; " \ "fi\0"
Which follows exactly that pattern.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c index 4286969..7447d43 100644 --- a/common/cmd_sandbox.c +++ b/common/cmd_sandbox.c @@ -10,6 +10,8 @@ #include <sandboxblockdev.h>
ret = host_get_dev_err(dev, &blk_dev);if (ret) {if (ret == -ENOENT)puts("Not bound to a backing file\n");Just use printf(), we should avoid puts() now.
ok
else if (ret == -ENODEV)puts("Invalid host device number\n");return 1;}sandbox_curr_device = dev;return 0;+}
+U_BOOT_CMD(
host, 3, 2, do_host_dev,"Set or get current host device", "dev [dev] - Set or retrieve the current host device"Can we make this command part of the 'sb' command? Like 'sh host ...'.
In principle, sure, however that breaks the consistency with other commands which all use <interface> <interface-commands>, where <interface> matches the names used in the fs and partition commands. Which is exactly the consistency _distro_bootcmd takes advantage of in the macro i mentioned above.
My thinking was that the sb set of commands are used to manage/setup the sandbox interface (e.g. setting up the host device bindings) while in this case the host commands are for the more standard interaction with host interface devices.
Another way of addressing this and essentially side-stepping this discussion would be to add a a new generic dev command to allow testing device existance e.g. dev exists host 0 dev exists mmc 0
And switch _distro_bootcmd to that instead of it relying on all block interfaces having a dev subcommand with the same semantics.
Opinions ? :)
HI Sjoerd,
On 28 February 2015 at 07:00, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
A common pattern to check if a certain device exists (e.g. in config_distro_bootcmd) is to use: <interface> dev [device]
Implement host dev [device] so this pattern can be used for sandbox host devices.
I don't see where this actually affects anything? In other words, does it really use the device you select, or just ignore you?
It gets ignored, it's only real usage is to detect whether a device exists.
To step back the reason I implemented it here is just to make it simpler to integrate with the command boot commands as it's common blockdevice macro is:
#define BOOTENV_SHARED_BLKDEV_BODY(devtypel) \ "if " #devtypel " dev ${devnum}; then " \ "setenv devtype " #devtypel "; " \ "run scan_dev_for_boot_part; " \ "fi\0"
Which follows exactly that pattern.
I see.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c index 4286969..7447d43 100644 --- a/common/cmd_sandbox.c +++ b/common/cmd_sandbox.c @@ -10,6 +10,8 @@ #include <sandboxblockdev.h>
ret = host_get_dev_err(dev, &blk_dev);if (ret) {if (ret == -ENOENT)puts("Not bound to a backing file\n");Just use printf(), we should avoid puts() now.
ok
else if (ret == -ENODEV)puts("Invalid host device number\n");return 1;}sandbox_curr_device = dev;return 0;+}
+U_BOOT_CMD(
host, 3, 2, do_host_dev,"Set or get current host device", "dev [dev] - Set or retrieve the current host device"Can we make this command part of the 'sb' command? Like 'sh host ...'.
In principle, sure, however that breaks the consistency with other commands which all use <interface> <interface-commands>, where <interface> matches the names used in the fs and partition commands. Which is exactly the consistency _distro_bootcmd takes advantage of in the macro i mentioned above.
My thinking was that the sb set of commands are used to manage/setup the sandbox interface (e.g. setting up the host device bindings) while in this case the host commands are for the more standard interaction with host interface devices.
Another way of addressing this and essentially side-stepping this discussion would be to add a a new generic dev command to allow testing device existance e.g. dev exists host 0 dev exists mmc 0
And switch _distro_bootcmd to that instead of it relying on all block interfaces having a dev subcommand with the same semantics.
Opinions ? :)
How about we change the existing 'sb' command to 'host'? Arguably 'sb' is a bit of a silly name. We could maintain an alias for a while.
Regards, Simon
On Mon, 2015-03-02 at 14:58 -0700, Simon Glass wrote:
HI Sjoerd,
How about we change the existing 'sb' command to 'host'? Arguably 'sb' is a bit of a silly name. We could maintain an alias for a while.
I'm fine with that. I'll prepare an update patchset doing the alias and addressing your other comments pn this series.
Thanks for the feedback!
Define the common shared block environment for the host interface in preperation for the sandbox build to use config_distro_bootcmd.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk --- include/config_distro_bootcmd.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 07a0b3b..ff0e3a8 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -32,6 +32,18 @@ #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \ #devtypel #instance " "
+#ifdef CONFIG_SANDBOX +#define BOOTENV_SHARED_HOST BOOTENV_SHARED_BLKDEV(host) +#define BOOTENV_DEV_HOST BOOTENV_DEV_BLKDEV +#define BOOTENV_DEV_NAME_HOST BOOTENV_DEV_NAME_BLKDEV +#else +#define BOOTENV_SHARED_HOST +#define BOOTENV_DEV_HOST \ + BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX +#define BOOTENV_DEV_NAME_HOST \ + BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX +#endif + #ifdef CONFIG_CMD_MMC #define BOOTENV_SHARED_MMC BOOTENV_SHARED_BLKDEV(mmc) #define BOOTENV_DEV_MMC BOOTENV_DEV_BLKDEV @@ -151,6 +163,7 @@ #define BOOTENV_DEV(devtypeu, devtypel, instance) \ BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance) #define BOOTENV \ + BOOTENV_SHARED_HOST \ BOOTENV_SHARED_MMC \ BOOTENV_SHARED_USB \ BOOTENV_SHARED_SATA \
+Stephen who knows more about this stuff
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Define the common shared block environment for the host interface in preperation for the sandbox build to use config_distro_bootcmd.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
include/config_distro_bootcmd.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 07a0b3b..ff0e3a8 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -32,6 +32,18 @@ #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \ #devtypel #instance " "
+#ifdef CONFIG_SANDBOX +#define BOOTENV_SHARED_HOST BOOTENV_SHARED_BLKDEV(host) +#define BOOTENV_DEV_HOST BOOTENV_DEV_BLKDEV +#define BOOTENV_DEV_NAME_HOST BOOTENV_DEV_NAME_BLKDEV +#else +#define BOOTENV_SHARED_HOST +#define BOOTENV_DEV_HOST \
BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX+#define BOOTENV_DEV_NAME_HOST \
Can we use upper case in #defines?
BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX+#endif
#ifdef CONFIG_CMD_MMC #define BOOTENV_SHARED_MMC BOOTENV_SHARED_BLKDEV(mmc) #define BOOTENV_DEV_MMC BOOTENV_DEV_BLKDEV @@ -151,6 +163,7 @@ #define BOOTENV_DEV(devtypeu, devtypel, instance) \ BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance) #define BOOTENV \
BOOTENV_SHARED_HOST \ BOOTENV_SHARED_MMC \ BOOTENV_SHARED_USB \ BOOTENV_SHARED_SATA \-- 2.1.4
Regards, Simon
On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
+Stephen who knows more about this stuff
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Define the common shared block environment for the host interface in preperation for the sandbox build to use config_distro_bootcmd.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
include/config_distro_bootcmd.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 07a0b3b..ff0e3a8 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -32,6 +32,18 @@ #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \ #devtypel #instance " "
+#ifdef CONFIG_SANDBOX +#define BOOTENV_SHARED_HOST BOOTENV_SHARED_BLKDEV(host) +#define BOOTENV_DEV_HOST BOOTENV_DEV_BLKDEV +#define BOOTENV_DEV_NAME_HOST BOOTENV_DEV_NAME_BLKDEV +#else +#define BOOTENV_SHARED_HOST +#define BOOTENV_DEV_HOST \
BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX+#define BOOTENV_DEV_NAME_HOST \
Can we use upper case in #defines?
The reason for this is that it is consistent with all other block device blocks in that file e.g:
#define BOOTENV_DEV_SATA \ BOOT_TARGET_DEVICES_references_SATA_without_CONFIG_CMD_SATA
So i'd prefer to to keep it that way. Btw, note that this is used for compile time error reporting rather then something you'd refer to in code.
+Stephen for real this time
Hi Sjoerd,
On 28 February 2015 at 07:05, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
+Stephen who knows more about this stuff
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Define the common shared block environment for the host interface in preperation for the sandbox build to use config_distro_bootcmd.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
include/config_distro_bootcmd.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 07a0b3b..ff0e3a8 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -32,6 +32,18 @@ #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \ #devtypel #instance " "
+#ifdef CONFIG_SANDBOX +#define BOOTENV_SHARED_HOST BOOTENV_SHARED_BLKDEV(host) +#define BOOTENV_DEV_HOST BOOTENV_DEV_BLKDEV +#define BOOTENV_DEV_NAME_HOST BOOTENV_DEV_NAME_BLKDEV +#else +#define BOOTENV_SHARED_HOST +#define BOOTENV_DEV_HOST \
BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX+#define BOOTENV_DEV_NAME_HOST \
Can we use upper case in #defines?
The reason for this is that it is consistent with all other block device blocks in that file e.g:
#define BOOTENV_DEV_SATA \ BOOT_TARGET_DEVICES_references_SATA_without_CONFIG_CMD_SATA
So i'd prefer to to keep it that way. Btw, note that this is used for compile time error reporting rather then something you'd refer to in code.
OK I see.
Regards, Simon
On 02/28/2015 07:14 AM, Simon Glass wrote:
+Stephen for real this time
Hi Sjoerd,
On 28 February 2015 at 07:05, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
+Stephen who knows more about this stuff
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Define the common shared block environment for the host interface in preperation for the sandbox build to use config_distro_bootcmd.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
include/config_distro_bootcmd.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 07a0b3b..ff0e3a8 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -32,6 +32,18 @@ #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \ #devtypel #instance " "
+#ifdef CONFIG_SANDBOX +#define BOOTENV_SHARED_HOST BOOTENV_SHARED_BLKDEV(host) +#define BOOTENV_DEV_HOST BOOTENV_DEV_BLKDEV +#define BOOTENV_DEV_NAME_HOST BOOTENV_DEV_NAME_BLKDEV +#else +#define BOOTENV_SHARED_HOST +#define BOOTENV_DEV_HOST \
BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX+#define BOOTENV_DEV_NAME_HOST \
Can we use upper case in #defines?
The reason for this is that it is consistent with all other block device blocks in that file e.g:
#define BOOTENV_DEV_SATA \ BOOT_TARGET_DEVICES_references_SATA_without_CONFIG_CMD_SATA
So i'd prefer to to keep it that way. Btw, note that this is used for compile time error reporting rather then something you'd refer to in code.
OK I see.
Yes, in this case that mixed-case variable name is essentially an error message to the user. The mixed case hopefully makes it more legible since only other variable names are in upper case, and the rest of the warning text is lower case. We can't use e.g. #error here, since this error can only be detected in the middle of a macro expansion rather than via some top-level singleton #if/#ifdef check.
Hi,
On 2 March 2015 at 09:54, Stephen Warren swarren@wwwdotorg.org wrote:
On 02/28/2015 07:14 AM, Simon Glass wrote:
+Stephen for real this time
Hi Sjoerd,
On 28 February 2015 at 07:05, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
+Stephen who knows more about this stuff
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Define the common shared block environment for the host interface in preperation for the sandbox build to use config_distro_bootcmd.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
include/config_distro_bootcmd.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 07a0b3b..ff0e3a8 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -32,6 +32,18 @@ #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \ #devtypel #instance " "
+#ifdef CONFIG_SANDBOX +#define BOOTENV_SHARED_HOST BOOTENV_SHARED_BLKDEV(host) +#define BOOTENV_DEV_HOST BOOTENV_DEV_BLKDEV +#define BOOTENV_DEV_NAME_HOST BOOTENV_DEV_NAME_BLKDEV +#else +#define BOOTENV_SHARED_HOST +#define BOOTENV_DEV_HOST \
BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX+#define BOOTENV_DEV_NAME_HOST \
Can we use upper case in #defines?
The reason for this is that it is consistent with all other block device blocks in that file e.g:
#define BOOTENV_DEV_SATA \ BOOT_TARGET_DEVICES_references_SATA_without_CONFIG_CMD_SATA
So i'd prefer to to keep it that way. Btw, note that this is used for compile time error reporting rather then something you'd refer to in code.
OK I see.
Yes, in this case that mixed-case variable name is essentially an error message to the user. The mixed case hopefully makes it more legible since only other variable names are in upper case, and the rest of the warning text is lower case. We can't use e.g. #error here, since this error can only be detected in the middle of a macro expansion rather than via some top-level singleton #if/#ifdef check.
Makes sense. Maybe we could add a comment to this effect?
Regards, Simon
Properly map memory through map_sysmem so that pxe can be used from the sandbox.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk --- common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 34 deletions(-)
diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c index 7e32c95..ec81e70 100644 --- a/common/cmd_pxe.c +++ b/common/cmd_pxe.c @@ -13,6 +13,7 @@ #include <errno.h> #include <linux/list.h> #include <fs.h> +#include <asm/io.h>
#include "menu.h" #include "cli.h" @@ -188,11 +189,11 @@ static int do_get_any(cmd_tbl_t *cmdtp, const char *file_path, char *file_addr) * * Returns 1 for success, or < 0 on error. */ -static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr) +static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, unsigned long file_addr) { size_t path_len; char relfile[MAX_TFTP_PATH_LEN+1]; - char addr_buf[10]; + char addr_buf[18]; int err;
err = get_bootfile_path(file_path, relfile, sizeof(relfile)); @@ -215,7 +216,7 @@ static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
printf("Retrieving file: %s\n", relfile);
- sprintf(addr_buf, "%p", file_addr); + sprintf(addr_buf, "%lx", file_addr);
return do_getfile(cmdtp, relfile, addr_buf); } @@ -227,11 +228,12 @@ static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr) * * Returns 1 on success, or < 0 for error. */ -static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr) +static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, unsigned long file_addr) { unsigned long config_file_size; char *tftp_filesize; int err; + void *buf;
err = get_relfile(cmdtp, file_path, file_addr);
@@ -250,7 +252,9 @@ static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr if (strict_strtoul(tftp_filesize, 16, &config_file_size) < 0) return -EINVAL;
- *(char *)(file_addr + config_file_size) = '\0'; + buf = map_sysmem (file_addr + config_file_size, 1); + * (char *)buf = '\0'; + unmap_sysmem (buf);
return 1; } @@ -266,7 +270,7 @@ static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr * * Returns 1 on success or < 0 on error. */ -static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void *pxefile_addr_r) +static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, unsigned long pxefile_addr_r) { size_t base_len = strlen(PXELINUX_DIR); char path[MAX_TFTP_PATH_LEN+1]; @@ -287,7 +291,7 @@ static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void *pxefile_a * * Returns 1 on success or < 0 on error. */ -static int pxe_uuid_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r) +static int pxe_uuid_path(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r) { char *uuid_str;
@@ -305,7 +309,7 @@ static int pxe_uuid_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r) * * Returns 1 on success or < 0 on error. */ -static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r) +static int pxe_mac_path(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r) { char mac_str[21]; int err; @@ -325,7 +329,7 @@ static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r) * * Returns 1 on success or < 0 on error. */ -static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, void *pxefile_addr_r) +static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r) { char ip_addr[9]; int mask_pos, err; @@ -384,9 +388,9 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) * Keep trying paths until we successfully get a file we're looking * for. */ - if (pxe_uuid_path(cmdtp, (void *)pxefile_addr_r) > 0 || - pxe_mac_path(cmdtp, (void *)pxefile_addr_r) > 0 || - pxe_ipaddr_paths(cmdtp, (void *)pxefile_addr_r) > 0) { + if (pxe_uuid_path(cmdtp, pxefile_addr_r) > 0 || + pxe_mac_path(cmdtp, pxefile_addr_r) > 0 || + pxe_ipaddr_paths(cmdtp, pxefile_addr_r) > 0) { printf("Config file found\n");
return 0; @@ -394,7 +398,7 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
while (pxe_default_paths[i]) { if (get_pxelinux_path(cmdtp, pxe_default_paths[i], - (void *)pxefile_addr_r) > 0) { + pxefile_addr_r) > 0) { printf("Config file found\n"); return 0; } @@ -427,7 +431,7 @@ static int get_relfile_envaddr(cmd_tbl_t *cmdtp, const char *file_path, const ch if (strict_strtoul(envaddr, 16, &file_addr) < 0) return -EINVAL;
- return get_relfile(cmdtp, file_path, (void *)file_addr); + return get_relfile(cmdtp, file_path, file_addr); }
/* @@ -1054,7 +1058,7 @@ static int parse_integer(char **c, int *dst) return 1; }
-static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level); +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b, struct pxe_menu *cfg, int nest_level);
/* * Parse an include statement, and retrieve and parse the file it mentions. @@ -1064,12 +1068,13 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in * include, nest_level has already been incremented and doesn't need to be * incremented here. */ -static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base, +static int handle_include(cmd_tbl_t *cmdtp, char **c, unsigned long base, struct pxe_menu *cfg, int nest_level) { char *include_path; char *s = *c; int err; + char *buf;
err = parse_sliteral(c, &include_path);
@@ -1086,20 +1091,22 @@ static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base, return err; }
- return parse_pxefile_top(cmdtp, base, cfg, nest_level); + buf = map_sysmem (base, 0); + return parse_pxefile_top(cmdtp, buf, base, cfg, nest_level); }
/* * Parse lines that begin with 'menu'. * - * b and nest are provided to handle the 'menu include' case. - * - * b should be the address where the file currently being parsed is stored. + * base and nest are provided to handle the 'menu include' case. * - * nest_level should be 1 when parsing the top level pxe file, 2 when parsing - * a file it includes, 3 when parsing a file included by that file, and so on. + * base should point to a location where it's safe to store the file, and + * nest_level should indicate how many nested includes have occurred. For this + * include, nest_level has already been incremented and doesn't need to be + * incremented here. */ -static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b, int nest_level) +static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, + unsigned long base, int nest_level) { struct token t; char *s = *c; @@ -1114,7 +1121,7 @@ static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b, break;
case T_INCLUDE: - err = handle_include(cmdtp, c, b + strlen(b) + 1, cfg, + err = handle_include(cmdtp, c, base, cfg, nest_level + 1); break;
@@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg) * * Returns 1 on success, < 0 on error. */ -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level) +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b, + struct pxe_menu *cfg, int nest_level) { struct token t; - char *s, *b, *label_name; + char *s, *label_name, *base; int err;
- b = p; + base = p;
if (nest_level > MAX_NEST_LEVEL) { printf("Maximum nesting (%d) exceeded\n", MAX_NEST_LEVEL); @@ -1303,7 +1311,9 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in switch (t.type) { case T_MENU: cfg->prompt = 1; - err = parse_menu(cmdtp, &p, cfg, b, nest_level); + err = parse_menu(cmdtp, &p, cfg, + b + ALIGN(strlen(base), 4), + nest_level); break;
case T_TIMEOUT: @@ -1328,7 +1338,7 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in break;
case T_INCLUDE: - err = handle_include(cmdtp, &p, b + ALIGN(strlen(b), 4), cfg, + err = handle_include(cmdtp, &p, b + ALIGN(strlen(base), 4), cfg, nest_level + 1); break;
@@ -1385,9 +1395,10 @@ static void destroy_pxe_menu(struct pxe_menu *cfg) * files it includes). The resulting pxe_menu struct can be free()'d by using * the destroy_pxe_menu() function. */ -static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg) +static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, unsigned long menucfg) { struct pxe_menu *cfg; + char *buf;
cfg = malloc(sizeof(struct pxe_menu));
@@ -1398,7 +1409,8 @@ static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg)
INIT_LIST_HEAD(&cfg->labels);
- if (parse_pxefile_top(cmdtp, menucfg, cfg, 1) < 0) { + buf = map_sysmem (menucfg, 0); + if (parse_pxefile_top(cmdtp, buf, menucfg, cfg, 1) < 0) { destroy_pxe_menu(cfg); return NULL; } @@ -1556,7 +1568,7 @@ do_pxe_boot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return 1; }
- cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r)); + cfg = parse_pxefile(cmdtp, pxefile_addr_r);
if (cfg == NULL) { printf("Error parsing config file\n"); @@ -1663,12 +1675,12 @@ static int do_sysboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return 1; }
- if (get_pxe_file(cmdtp, filename, (void *)pxefile_addr_r) < 0) { + if (get_pxe_file(cmdtp, filename, pxefile_addr_r) < 0) { printf("Error reading config file\n"); return 1; }
- cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r)); + cfg = parse_pxefile(cmdtp, pxefile_addr_r);
if (cfg == NULL) { printf("Error parsing config file\n");
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Properly map memory through map_sysmem so that pxe can be used from the sandbox.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
Please run your patches through patman as you seem to have style violations. Also can you add some notes about how you have tested this on real hardware?
common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 34 deletions(-)
diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c index 7e32c95..ec81e70 100644 --- a/common/cmd_pxe.c +++ b/common/cmd_pxe.c @@ -13,6 +13,7 @@ #include <errno.h> #include <linux/list.h> #include <fs.h> +#include <asm/io.h>
#include "menu.h" #include "cli.h" @@ -188,11 +189,11 @@ static int do_get_any(cmd_tbl_t *cmdtp, const char *file_path, char *file_addr)
- Returns 1 for success, or < 0 on error.
*/ -static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr) +static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, unsigned long file_addr) { size_t path_len; char relfile[MAX_TFTP_PATH_LEN+1];
char addr_buf[10];
char addr_buf[18]; int err; err = get_bootfile_path(file_path, relfile, sizeof(relfile));@@ -215,7 +216,7 @@ static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
printf("Retrieving file: %s\n", relfile);
sprintf(addr_buf, "%p", file_addr);
sprintf(addr_buf, "%lx", file_addr); return do_getfile(cmdtp, relfile, addr_buf);} @@ -227,11 +228,12 @@ static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
- Returns 1 on success, or < 0 for error.
*/ -static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr) +static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, unsigned long file_addr) { unsigned long config_file_size; char *tftp_filesize; int err;
void *buf;
Can we make this char * please to remove the cast below?
err = get_relfile(cmdtp, file_path, file_addr);@@ -250,7 +252,9 @@ static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr if (strict_strtoul(tftp_filesize, 16, &config_file_size) < 0) return -EINVAL;
*(char *)(file_addr + config_file_size) = '\0';
buf = map_sysmem (file_addr + config_file_size, 1);* (char *)buf = '\0';unmap_sysmem (buf); return 1;} @@ -266,7 +270,7 @@ static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr
- Returns 1 on success or < 0 on error.
*/ -static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void *pxefile_addr_r) +static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, unsigned long pxefile_addr_r) { size_t base_len = strlen(PXELINUX_DIR); char path[MAX_TFTP_PATH_LEN+1]; @@ -287,7 +291,7 @@ static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void *pxefile_a
- Returns 1 on success or < 0 on error.
*/ -static int pxe_uuid_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r) +static int pxe_uuid_path(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r) { char *uuid_str;
@@ -305,7 +309,7 @@ static int pxe_uuid_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
- Returns 1 on success or < 0 on error.
*/ -static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r) +static int pxe_mac_path(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r) { char mac_str[21]; int err; @@ -325,7 +329,7 @@ static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
- Returns 1 on success or < 0 on error.
*/ -static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, void *pxefile_addr_r) +static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r) { char ip_addr[9]; int mask_pos, err; @@ -384,9 +388,9 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) * Keep trying paths until we successfully get a file we're looking * for. */
if (pxe_uuid_path(cmdtp, (void *)pxefile_addr_r) > 0 ||pxe_mac_path(cmdtp, (void *)pxefile_addr_r) > 0 ||pxe_ipaddr_paths(cmdtp, (void *)pxefile_addr_r) > 0) {
if (pxe_uuid_path(cmdtp, pxefile_addr_r) > 0 ||pxe_mac_path(cmdtp, pxefile_addr_r) > 0 ||pxe_ipaddr_paths(cmdtp, pxefile_addr_r) > 0) { printf("Config file found\n"); return 0;@@ -394,7 +398,7 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
while (pxe_default_paths[i]) { if (get_pxelinux_path(cmdtp, pxe_default_paths[i],
(void *)pxefile_addr_r) > 0) {
pxefile_addr_r) > 0) { printf("Config file found\n"); return 0; }@@ -427,7 +431,7 @@ static int get_relfile_envaddr(cmd_tbl_t *cmdtp, const char *file_path, const ch if (strict_strtoul(envaddr, 16, &file_addr) < 0) return -EINVAL;
return get_relfile(cmdtp, file_path, (void *)file_addr);
return get_relfile(cmdtp, file_path, file_addr);}
/* @@ -1054,7 +1058,7 @@ static int parse_integer(char **c, int *dst) return 1; }
-static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level); +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b, struct pxe_menu *cfg, int nest_level);
/*
- Parse an include statement, and retrieve and parse the file it mentions.
@@ -1064,12 +1068,13 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in
- include, nest_level has already been incremented and doesn't need to be
- incremented here.
*/ -static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base, +static int handle_include(cmd_tbl_t *cmdtp, char **c, unsigned long base, struct pxe_menu *cfg, int nest_level) { char *include_path; char *s = *c; int err;
char *buf; err = parse_sliteral(c, &include_path);@@ -1086,20 +1091,22 @@ static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base, return err; }
return parse_pxefile_top(cmdtp, base, cfg, nest_level);
buf = map_sysmem (base, 0);return parse_pxefile_top(cmdtp, buf, base, cfg, nest_level);
You should call unmap_sysmem() before the return.
}
/*
- Parse lines that begin with 'menu'.
- b and nest are provided to handle the 'menu include' case.
- b should be the address where the file currently being parsed is stored.
- base and nest are provided to handle the 'menu include' case.
- nest_level should be 1 when parsing the top level pxe file, 2 when parsing
- a file it includes, 3 when parsing a file included by that file, and so on.
- base should point to a location where it's safe to store the file, and
- nest_level should indicate how many nested includes have occurred. For this
- include, nest_level has already been incremented and doesn't need to be
*/
- incremented here.
-static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b, int nest_level) +static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg,
unsigned long base, int nest_level){ struct token t; char *s = *c; @@ -1114,7 +1121,7 @@ static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b, break;
case T_INCLUDE:
err = handle_include(cmdtp, c, b + strlen(b) + 1, cfg,
err = handle_include(cmdtp, c, base, cfg, nest_level + 1); break;@@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg)
- Returns 1 on success, < 0 on error.
*/ -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level) +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b,
struct pxe_menu *cfg, int nest_level)
s/b/base/ or something more meaningful (fix above/below also)
{ struct token t;
char *s, *b, *label_name;
char *s, *label_name, *base; int err;
b = p;
base = p;
This worries me - assigning a pointer to a ulong.
if (nest_level > MAX_NEST_LEVEL) { printf("Maximum nesting (%d) exceeded\n", MAX_NEST_LEVEL);@@ -1303,7 +1311,9 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in switch (t.type) { case T_MENU: cfg->prompt = 1;
err = parse_menu(cmdtp, &p, cfg, b, nest_level);
err = parse_menu(cmdtp, &p, cfg,b + ALIGN(strlen(base), 4),nest_level); break; case T_TIMEOUT:@@ -1328,7 +1338,7 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in break;
case T_INCLUDE:
err = handle_include(cmdtp, &p, b + ALIGN(strlen(b), 4), cfg,
err = handle_include(cmdtp, &p, b + ALIGN(strlen(base), 4), cfg, nest_level + 1); break;@@ -1385,9 +1395,10 @@ static void destroy_pxe_menu(struct pxe_menu *cfg)
- files it includes). The resulting pxe_menu struct can be free()'d by using
- the destroy_pxe_menu() function.
*/ -static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg) +static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, unsigned long menucfg) { struct pxe_menu *cfg;
char *buf; cfg = malloc(sizeof(struct pxe_menu));@@ -1398,7 +1409,8 @@ static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg)
INIT_LIST_HEAD(&cfg->labels);
if (parse_pxefile_top(cmdtp, menucfg, cfg, 1) < 0) {
buf = map_sysmem (menucfg, 0);if (parse_pxefile_top(cmdtp, buf, menucfg, cfg, 1) < 0) { destroy_pxe_menu(cfg); return NULL;
Need unmap_sysmem() after.
}@@ -1556,7 +1568,7 @@ do_pxe_boot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return 1; }
cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r));
cfg = parse_pxefile(cmdtp, pxefile_addr_r); if (cfg == NULL) { printf("Error parsing config file\n");@@ -1663,12 +1675,12 @@ static int do_sysboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return 1; }
if (get_pxe_file(cmdtp, filename, (void *)pxefile_addr_r) < 0) {
if (get_pxe_file(cmdtp, filename, pxefile_addr_r) < 0) { printf("Error reading config file\n"); return 1; }
cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r));
cfg = parse_pxefile(cmdtp, pxefile_addr_r); if (cfg == NULL) { printf("Error parsing config file\n");-- 2.1.4
Regards, Simon
On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Properly map memory through map_sysmem so that pxe can be used from the sandbox.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
Please run your patches through patman as you seem to have style violations. Also can you add some notes about how you have tested this on real hardware?
Will do for the next round together with addressing your comments. One confused me tough, see below.
common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 34 deletions(-)
@@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg)
- Returns 1 on success, < 0 on error.
*/ -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level) +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b,
struct pxe_menu *cfg, int nest_level)s/b/base/ or something more meaningful (fix above/below also)
{ struct token t;
char *s, *b, *label_name;
char *s, *label_name, *base; int err;
b = p;
base = p;This worries me - assigning a pointer to a ulong.
base is a pointer here though. So this comment confuses me.
Hi Sjoerd,
On 28 February 2015 at 07:12, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Properly map memory through map_sysmem so that pxe can be used from the sandbox.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
Please run your patches through patman as you seem to have style violations. Also can you add some notes about how you have tested this on real hardware?
Will do for the next round together with addressing your comments. One confused me tough, see below.
common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 34 deletions(-)
@@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg)
- Returns 1 on success, < 0 on error.
*/ -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level) +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b,
struct pxe_menu *cfg, int nest_level)s/b/base/ or something more meaningful (fix above/below also)
{ struct token t;
char *s, *b, *label_name;
char *s, *label_name, *base; int err;
b = p;
base = p;This worries me - assigning a pointer to a ulong.
base is a pointer here though. So this comment confuses me.
Actually it confused me. I don't think it's good to use base as a pointer or a ulong in the same file. Maybe use base for the ulong and base_ptr for the pointer. Or base_addr and base. But code is harder to read if different functions in the file have different conventions.
Regards, Simon
Make the sandbox setup more generic/examplary by including config_distro_defaults.h and config_distro_bootcmd.h.
Among other things this makes it easy to test whether images will boot though with the standard distro bootcmds by running e.g: u-boot -c 'sb bind 0 myimage.img ; boot'
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk --- include/configs/sandbox.h | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 5c11650..742bda6 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -75,7 +75,6 @@ #define CONFIG_CMDLINE_EDITING #define CONFIG_COMMAND_HISTORY #define CONFIG_AUTO_COMPLETE -#define CONFIG_BOOTDELAY 3
#define CONFIG_ENV_SIZE 8192 #define CONFIG_ENV_IS_NOWHERE @@ -125,10 +124,21 @@
/* include default commands */ #include <config_cmd_default.h> +#include <config_distro_defaults.h> + +#define BOOT_TARGET_DEVICES(func) \ + func(HOST, host, 1) \ + func(HOST, host, 0) + +#include <config_distro_bootcmd.h>
/* We don't have networking support yet */ #undef CONFIG_CMD_NET #undef CONFIG_CMD_NFS +#undef CONFIG_CMD_PING + +/* Can't boot elf images */ +#undef CONFIG_CMD_ELF
#define CONFIG_CMD_HASH #define CONFIG_HASH_VERIFY @@ -169,16 +179,29 @@ #define CONFIG_CROS_EC_KEYB #define CONFIG_KEYBOARD
-#define CONFIG_EXTRA_ENV_SETTINGS "stdin=serial,cros-ec-keyb\0" \ +#define EXTRA_ENV_SETTINGS "stdin=serial,cros-ec-keyb\0" \ "stdout=serial,lcd\0" \ "stderr=serial,lcd\0" #else
-#define CONFIG_EXTRA_ENV_SETTINGS "stdin=serial\0" \ +#define EXTRA_ENV_SETTINGS "stdin=serial\0" \ "stdout=serial,lcd\0" \ "stderr=serial,lcd\0" #endif
+#define MEM_LAYOUT_ENV_SETTINGS \ + "bootm_size=0x10000000\0" \ + "kernel_addr_r=0x1000000\0" \ + "fdt_addr_r=0xc00000\0" \ + "ramdisk_addr_r=0x2000000\0" \ + "scriptaddr=0x1000\0" \ + "pxefile_addr_r=0x2000\0" + +#define CONFIG_EXTRA_ENV_SETTINGS \ + BOOTENV \ + MEM_LAYOUT_ENV_SETTINGS \ + EXTRA_ENV_SETTINGS + #define CONFIG_GZIP_COMPRESSED #define CONFIG_BZIP2 #define CONFIG_LZO
On 19 February 2015 at 15:41, Sjoerd Simons sjoerd.simons@collabora.co.uk wrote:
Make the sandbox setup more generic/examplary by including config_distro_defaults.h and config_distro_bootcmd.h.
Among other things this makes it easy to test whether images will boot though with the standard distro bootcmds by running e.g: u-boot -c 'sb bind 0 myimage.img ; boot'
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
include/configs/sandbox.h | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)
Reviewed by: Simon Glass sjg@chromium.org
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 5c11650..742bda6 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -75,7 +75,6 @@ #define CONFIG_CMDLINE_EDITING #define CONFIG_COMMAND_HISTORY #define CONFIG_AUTO_COMPLETE -#define CONFIG_BOOTDELAY 3
#define CONFIG_ENV_SIZE 8192 #define CONFIG_ENV_IS_NOWHERE @@ -125,10 +124,21 @@
/* include default commands */ #include <config_cmd_default.h> +#include <config_distro_defaults.h>
+#define BOOT_TARGET_DEVICES(func) \
- func(HOST, host, 1) \
- func(HOST, host, 0)
It looks like you support two host devices. Is that somewhat arbitrary?
+#include <config_distro_bootcmd.h>
/* We don't have networking support yet */ #undef CONFIG_CMD_NET #undef CONFIG_CMD_NFS +#undef CONFIG_CMD_PING
+/* Can't boot elf images */ +#undef CONFIG_CMD_ELF
#define CONFIG_CMD_HASH #define CONFIG_HASH_VERIFY @@ -169,16 +179,29 @@ #define CONFIG_CROS_EC_KEYB #define CONFIG_KEYBOARD
-#define CONFIG_EXTRA_ENV_SETTINGS "stdin=serial,cros-ec-keyb\0" \ +#define EXTRA_ENV_SETTINGS "stdin=serial,cros-ec-keyb\0" \ "stdout=serial,lcd\0" \ "stderr=serial,lcd\0" #else
-#define CONFIG_EXTRA_ENV_SETTINGS "stdin=serial\0" \ +#define EXTRA_ENV_SETTINGS "stdin=serial\0" \ "stdout=serial,lcd\0" \ "stderr=serial,lcd\0" #endif
+#define MEM_LAYOUT_ENV_SETTINGS \
"bootm_size=0x10000000\0" \"kernel_addr_r=0x1000000\0" \"fdt_addr_r=0xc00000\0" \"ramdisk_addr_r=0x2000000\0" \"scriptaddr=0x1000\0" \"pxefile_addr_r=0x2000\0"+#define CONFIG_EXTRA_ENV_SETTINGS \
BOOTENV \MEM_LAYOUT_ENV_SETTINGS \EXTRA_ENV_SETTINGS#define CONFIG_GZIP_COMPRESSED #define CONFIG_BZIP2
#define CONFIG_LZO
2.1.4
Regards, Simon
participants (3)
-
Simon Glass -
Sjoerd Simons -
Stephen Warren