Allow sandbox to use config_distro_bootcmd

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 <[email protected]>

---
 fs/sandbox/sandboxfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/sandbox/sandboxfs.c +0 -0

index a920bc0..c6540c6 100644

fs/sandbox/sandboxfs.c +3 -1

+        * hostfs interface is used
+        */
+       return rbdd != NULL;
 }
 
 int sandbox_fs_read_at(const char *filename, loff_t pos, void *buffer,
--

2.1.4

Hi Sjoerd,

Quoted 6 lines · Sjoerd Simons

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 <[email protected]>

Reviewed-by Simon Glass <[email protected]>

See nit below.

Quoted 10 lines

---
 fs/sandbox/sandboxfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/sandbox/sandboxfs.c +0 -0

index a920bc0..c6540c6 100644
@@ -10,7 +10,10 @@
 

fs/sandbox/sandboxfs.c line 13

Quoted 4 lines

fs/sandbox/sandboxfs.c +1 -1

 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

Quoted 8 lines

fs/sandbox/sandboxfs.c +3 -1

+        * hostfs interface is used
+        */
+       return rbdd != NULL;
 }
 
 int sandbox_fs_read_at(const char *filename, loff_t pos, void *buffer,
--

2.1.4

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 <[email protected]>

---
 arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++

 1 file changed, 20 insertions(+)

arch/sandbox/cpu/cpu.c +1 -0

index 1aa397c..c71fb86 100644
@@ -7,6 +7,7 @@
 #include <dm/root.h>
 #include <os.h>
 #include <asm/state.h>
+#include <asm/io.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 

arch/sandbox/cpu/cpu.c +9 -1

+    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

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 <[email protected]>

---
 common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

common/cmd_sandbox.c +24 -0

index 4286969..7447d43 100644
@@ -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;
+       }
+

common/cmd_sandbox.c +1 -1

+);
--

2.1.4

Hi Sjoerd,

On 19 February 2015 at 15:41, Sjoerd Simons
<[email protected]> wrote:

Quoted 5 lines

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?

Quoted 11 lines

Signed-off-by Sjoerd Simons <[email protected]>

---
 common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

common/cmd_sandbox.c +0 -0

index 4286969..7447d43 100644
@@ -10,6 +10,8 @@
 #include <sandboxblockdev.h>

common/cmd_sandbox.c U_BOOT_CMD( · line 160

Quoted 4 lines

common/cmd_sandbox.c +4 -0

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

Quoted 7 lines

common/cmd_sandbox.c +7 -0

+               else if (ret == -ENODEV)
+                       puts("Invalid host device number\n");
+
+               return 1;
+       }
+
+       sandbox_curr_device = dev;

common/cmd_sandbox.c U_BOOT_CMD( · line 173

Quoted 6 lines

common/cmd_sandbox.c +6 -0

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

Quoted 4 lines · Simon Glass

Hi Sjoerd,

On 19 February 2015 at 15:41, Sjoerd Simons
<[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

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.

Quoted 13 lines

Quoted 12 lines

Signed-off-by Sjoerd Simons <[email protected]>

---
 common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

common/cmd_sandbox.c +0 -0

index 4286969..7447d43 100644
@@ -10,6 +10,8 @@
 #include <sandboxblockdev.h>

Quoted 6 lines

common/cmd_sandbox.c U_BOOT_CMD( · line 160

Quoted 4 lines

common/cmd_sandbox.c +4 -0

+       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

Quoted 7 lines

Quoted 7 lines

common/cmd_sandbox.c +7 -0

+               else if (ret == -ENODEV)
+                       puts("Invalid host device number\n");
+
+               return 1;
+       }
+
+       sandbox_curr_device = dev;

Quoted 8 lines

common/cmd_sandbox.c U_BOOT_CMD( · line 173

Quoted 6 lines

common/cmd_sandbox.c +6 -0

+       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,

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.

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 <[email protected]>

---
 include/config_distro_bootcmd.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

include/config_distro_bootcmd.h +0 -0

index 07a0b3b..ff0e3a8 100644
@@ -32,6 +32,18 @@
 #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
        #devtypel #instance " "

include/config_distro_bootcmd.h +4 -1

+       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

+Stephen who knows more about this stuff

Quoted 16 lines · Sjoerd Simons

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 <[email protected]>

---
 include/config_distro_bootcmd.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

include/config_distro_bootcmd.h +0 -0

index 07a0b3b..ff0e3a8 100644
@@ -32,6 +32,18 @@
 #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
        #devtypel #instance " "
 

include/config_distro_bootcmd.h line 43

Quoted 9 lines

include/config_distro_bootcmd.h +9 -0

+#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?

Quoted 20 lines · Simon Glass

+Stephen who knows more about this stuff

Quoted 16 lines · Sjoerd Simons

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 <[email protected]>

---
 include/config_distro_bootcmd.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

include/config_distro_bootcmd.h +0 -0

index 07a0b3b..ff0e3a8 100644
@@ -32,6 +32,18 @@
 #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
        #devtypel #instance " "
 

Quoted 11 lines

include/config_distro_bootcmd.h line 43

Quoted 9 lines

include/config_distro_bootcmd.h +9 -0

+#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,

Quoted 36 lines · Sjoerd Simons

Quoted 20 lines · Simon Glass

+Stephen who knows more about this stuff

Quoted 16 lines · Sjoerd Simons

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 <[email protected]>

---
  include/config_distro_bootcmd.h | 13 +++++++++++++
  1 file changed, 13 insertions(+)

include/config_distro_bootcmd.h +0 -0

index 07a0b3b..ff0e3a8 100644
@@ -32,6 +32,18 @@
  #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
         #devtypel #instance " "
 

Quoted 11 lines

include/config_distro_bootcmd.h line 43

Quoted 9 lines

include/config_distro_bootcmd.h +9 -0

+#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:

Quoted 6 lines

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

Quoted 52 lines · Simon Glass

+Stephen for real this time

Hi Sjoerd,

Quoted 45 lines · Sjoerd Simons

Quoted 23 lines · Simon Glass

+Stephen who knows more about this stuff

Quoted 18 lines · Sjoerd Simons

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 <[email protected]>

---
  include/config_distro_bootcmd.h | 13 +++++++++++++
  1 file changed, 13 insertions(+)

include/config_distro_bootcmd.h +0 -0

index 07a0b3b..ff0e3a8 100644
@@ -32,6 +32,18 @@
  #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
         #devtypel #instance " "
 

Quoted 12 lines

include/config_distro_bootcmd.h line 43

Quoted 9 lines

include/config_distro_bootcmd.h +9 -0

+#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

Quoted 6 lines

Quoted 3 lines

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.

Properly map memory through map_sysmem so that pxe can be used from the sandbox.

Signed-off-by Sjoerd Simons <[email protected]>

---
 common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 34 deletions(-)

common/cmd_pxe.c +4 -3

index 7e32c95..ec81e70 100644
@@ -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);
 }

common/cmd_pxe.c +15 -12

        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);
 

common/cmd_pxe.c +8 -7

 }
 
 /*
  * 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;
 

common/cmd_pxe.c +6 -3

        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));
 

common/cmd_pxe.c +3 -4

        }

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

Hi Sjoerd,

On 19 February 2015 at 15:41, Sjoerd Simons
<[email protected]> wrote:

Quoted 4 lines

Properly map memory through map_sysmem so that pxe can be used from the sandbox.

Signed-off-by Sjoerd Simons <[email protected]>

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 static int parse_label(char **c, struct pxe_menu *cfg) · line 1292

Quoted 11 lines

---
 common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 34 deletions(-)

common/cmd_pxe.c +2 -1

@@ -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)

Quoted 6 lines

common/cmd_pxe.c +1 -1

 {
        struct token t;
-       char *s, *b, *label_name;
+       char *s, *label_name, *base;
        int err;
 

common/cmd_pxe.c static int parse_label(char **c, struct pxe_menu *cfg) · line 1298

Quoted 2 lines

common/cmd_pxe.c +1 -1

-       b = p;
+       base = p;

This worries me - assigning a pointer to a ulong.

Quoted 4 lines · Simon Glass

Hi Sjoerd,

On 19 February 2015 at 15:41, Sjoerd Simons
<[email protected]> wrote:

Quoted 8 lines

Quoted 4 lines

Properly map memory through map_sysmem so that pxe can be used from the sandbox.

Signed-off-by Sjoerd Simons <[email protected]>

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.

Quoted 19 lines

common/cmd_pxe.c static int parse_label(char **c, struct pxe_menu *cfg) · line 1292

Quoted 11 lines

---
 common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 34 deletions(-)

common/cmd_pxe.c +2 -1

@@ -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)

Quoted 6 lines

common/cmd_pxe.c +1 -1

 {
        struct token t;
-       char *s, *b, *label_name;
+       char *s, *label_name, *base;
        int err;
 

Quoted 4 lines

common/cmd_pxe.c static int parse_label(char **c, struct pxe_menu *cfg) · line 1298

Quoted 2 lines

common/cmd_pxe.c +1 -1

-       b = p;
+       base = p;

This worries me - assigning a pointer to a ulong.

base is a pointer here though. So this comment confuses me.

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 <[email protected]>

---
 include/configs/sandbox.h | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

include/configs/sandbox.h +0 -1

index 5c11650..742bda6 100644
@@ -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

include/configs/sandbox.h +21 -3

+
+#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