Re: [U-Boot] [PATCH 5/6] pxe: Ensure all memory access is to mapped memory

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