[U-Boot] [PATCH 5/6] pxe: Ensure all memory access is to mapped memory
Simon Glass
sjg at chromium.org
Fri Feb 20 20:23:12 CET 2015
Hi Sjoerd,
On 19 February 2015 at 15:41, Sjoerd Simons
<sjoerd.simons at 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 at 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
More information about the U-Boot
mailing list