Re: [PATCH v2 2/3] net: dhcp6: pxe: Add DHCP/PXE commands for IPv6

On 2023-04-07 11:55 a.m., Simon Glass wrote:

Hi Sean,

On Fri, 7 Apr 2023 at 18:56, seanedmond@linux.microsoft.com wrote:

...

From: Sean Edmond seanedmond@microsoft.com

Adds commands to support DHCP and PXE with IPv6.

New configs added:

• CMD_DHCP6
• DHCP6_PXE_CLIENTARCH
• DHCP6_PXE_DHCP_OPTION
• DHCP6_ENTERPRISE_ID

New commands added (when IPv6 is enabled):

• dhcp6
• pxe get -ipv6
• pxe boot -ipv6

Signed-off-by: Sean Edmond seanedmond@microsoft.com

boot/bootmeth_distro.c | 2 +- boot/bootmeth_pxe.c | 4 +- boot/pxe_utils.c | 3 +- cmd/Kconfig | 26 +++++++++++++ cmd/net.c | 23 +++++++++++ cmd/pxe.c | 86 +++++++++++++++++++++++++++++++++++++----- cmd/sysboot.c | 2 +- include/pxe_utils.h | 10 ++++- 8 files changed, 140 insertions(+), 16 deletions(-)

With nits below:

Reviewed-by: Simon Glass sjg@chromium.org

[..]

...

+if CMD_DHCP6

+config DHCP6_PXE_CLIENTARCH

•   hex
  

•   default 0x16 if ARM64
  

•   default 0x15 if ARM
  

•   default 0xFF
  

Do we need a separate option or could we use BOOTP_PXE_CLIENTARCH ?

I created a new option because I wanted to change the default to 0xFF ("undefined" Processor Architecture Types according to https://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml). I wanted to do this without changing BOOTP_PXE_CLIENTARCH , and potentially disrupting exisiting DHCPv4 implementations.

...

+config DHCP6_PXE_DHCP_OPTION

•   bool "Request & store 'pxe_configfile' from DHCP6 server"
  

+config DHCP6_ENTERPRISE_ID

•   int "Enterprise ID to send in DHCPv6 Vendor Class Option"
  

•   default 0
  

+endif

• config CMD_TFTPBOOT bool "tftpboot" default y

diff --git a/cmd/net.c b/cmd/net.c index d5e20843dd..95529a9d12 100644 --- a/cmd/net.c +++ b/cmd/net.c @@ -111,6 +111,29 @@ U_BOOT_CMD( ); #endif

+#if defined(CONFIG_CMD_DHCP6) +static int do_dhcp6(struct cmd_tbl *cmdtp, int flag, int argc,

•               char *const argv[])
  

+{

•   int i;
  

•   int dhcp_argc;
  

•   char *dhcp_argv[] = {NULL, NULL, NULL, NULL};
  

•   /* Add -ipv6 flag for autoload */
  

•   for (i = 0; i < argc; i++)
  

•           dhcp_argv[i] = argv[i];
  

•   dhcp_argc = argc + 1;
  

•   dhcp_argv[dhcp_argc - 1] =  USE_IP6_CMD_PARAM;
  

•   return netboot_common(DHCP6, cmdtp, dhcp_argc, dhcp_argv);
  

+}

+U_BOOT_CMD(dhcp6, 3, 1, do_dhcp6,

•      "boot image via network using DHCPv6/TFTP protocol. \n"
  

•      "Use IPv6 hostIPaddr framed with [] brackets",
  

•      "[loadAddress] [[hostIPaddr:]bootfilename]");
  

+#endif

• #if defined(CONFIG_CMD_DHCP) static int do_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])

diff --git a/cmd/pxe.c b/cmd/pxe.c index db8e4697f2..71e6fd9633 100644 --- a/cmd/pxe.c +++ b/cmd/pxe.c @@ -8,6 +8,7 @@ #include <command.h> #include <fs.h> #include <net.h> +#include <net6.h>

#include "pxe_utils.h"

@@ -29,12 +30,20 @@ static int do_get_tftp(struct pxe_context *ctx, const char *file_path, { char *tftp_argv[] = {"tftp", NULL, NULL, NULL}; int ret;

•   int num_args;
  
     tftp_argv[1] = file_addr;
     tftp_argv[2] = (void *)file_path;
  

•   if (ctx->use_ipv6) {
  

•           tftp_argv[3] = USE_IP6_CMD_PARAM;
  

•           num_args = 4;
  

•   } else {
  

•           num_args = 3;
  

•   }
  
  

•   if (do_tftpb(ctx->cmdtp, 0, 3, tftp_argv))
  

•   if (do_tftpb(ctx->cmdtp, 0, num_args, tftp_argv))
             return -ENOENT;
  

•    ret = pxe_get_file_size(sizep);
     if (ret)
             return log_msg_ret("tftp", ret);
  

@@ -43,6 +52,23 @@ static int do_get_tftp(struct pxe_context *ctx, const char *file_path, return 1; }

+#if defined(CONFIG_DHCP6_PXE_DHCP_OPTION) +/*

• • Looks for a pxe file with specified config file name,
• • which is received from DHCPv4 option 209 or
• • DHCPv6 option 60.
• • Returns 1 on success or < 0 on error.
• */

+static inline int pxe_dhcp_option_path(struct pxe_context *ctx, unsigned long pxefile_addr_r)

Please drop the inline as the compiler can handle that.

...

+{

•   int ret = get_pxe_file(ctx, pxelinux_configfile, pxefile_addr_r);
  

•   free(pxelinux_configfile);
  

•   return ret;
  

+} +#endif /*

• Looks for a pxe file with a name based on the pxeuuid environment variable.

@@ -105,15 +131,25 @@ static int pxe_ipaddr_paths(struct pxe_context *ctx, unsigned long pxefile_addr_ return -ENOENT; }

-int pxe_get(ulong pxefile_addr_r, char **bootdirp, ulong *sizep) +int pxe_get(ulong pxefile_addr_r, char **bootdirp, ulong *sizep, bool use_ipv6) { struct cmd_tbl cmdtp[] = {}; /* dummy */ struct pxe_context ctx; int i;

     if (pxe_setup_ctx(&ctx, cmdtp, do_get_tftp, NULL, false,

•                     env_get("bootfile")))
  

•                     env_get("bootfile"), use_ipv6))
             return -ENOMEM;
  

+#if defined(CONFIG_DHCP6_PXE_DHCP_OPTION)

It's a bit annoying that pxelinux_configfile causes these #ifdefs. How about always having pxelinux_configfile and then you don't need to worry. It can just be NULL when not used. It is only BSS space, after all...

...

•   if (pxelinux_configfile && use_ipv6) {
  

•           if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
  

•                   goto done;
  

•           goto error_exit;
  

•   }
  

+#endif

•    /*
      * Keep trying paths until we successfully get a file we're looking
      * for.
  

@@ -131,9 +167,12 @@ int pxe_get(ulong pxefile_addr_r, char **bootdirp, ulong *sizep) i++; }

+#if defined(CONFIG_DHCP6_PXE_DHCP_OPTION)

...then you can use if (IS_ENABLED(... here

...

+error_exit: pxe_destroy_ctx(&ctx);

     return -ENOENT;

+#endif done: *bootdirp = env_get("bootfile");

@@ -169,9 +208,17 @@ do_pxe_get(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) char *fname; ulong size; int ret;

•   bool use_ipv6 = false;
  
  

•   if (argc != 1)
  

•           return CMD_RET_USAGE;
  

•   if (IS_ENABLED(CONFIG_IPV6)) {
  

•           if (argc != 1 && argc != 2)
  

•                   return CMD_RET_USAGE;
  

•           if (!strcmp(argv[argc - 1], USE_IP6_CMD_PARAM))
  

•                   use_ipv6 = true;
  

•   } else {
  

•           if (argc != 1)
  

•                   return CMD_RET_USAGE;
  

•   }
  
     pxefile_addr_str = from_env("pxefile_addr_r");
  
  

@@ -183,7 +230,7 @@ do_pxe_get(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (ret < 0) return 1;

•   ret = pxe_get(pxefile_addr_r, &fname, &size);
  

•   ret = pxe_get(pxefile_addr_r, &fname, &size, use_ipv6);
     switch (ret) {
     case 0:
             printf("Config file '%s' found\n", fname);
  

@@ -211,13 +258,19 @@ do_pxe_boot(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) char *pxefile_addr_str; struct pxe_context ctx; int ret;

•   bool use_ipv6 = false;
  

•   if (IS_ENABLED(CONFIG_IPV6)) {
  

•           if (!strcmp(argv[argc - 1], USE_IP6_CMD_PARAM))
  

•                   use_ipv6 = true;
  

•   }
  
  

•   if (argc == 1) {
  

•   if (argc == 1 || (argc == 2 && use_ipv6)) {
             pxefile_addr_str = from_env("pxefile_addr_r");
             if (!pxefile_addr_str)
                     return 1;
  
  

•   } else if (argc == 2) {
  

•   } else if (argc == 2 || (argc == 3 && use_ipv6)) {
             pxefile_addr_str = argv[1];
     } else {
             return CMD_RET_USAGE;
  

@@ -229,7 +282,7 @@ do_pxe_boot(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) }

     if (pxe_setup_ctx(&ctx, cmdtp, do_get_tftp, NULL, false,

•                     env_get("bootfile"))) {
  

•                     env_get("bootfile"), use_ipv6)) {
             printf("Out of memory\n");
             return CMD_RET_FAILURE;
     }
  

@@ -244,8 +297,13 @@ do_pxe_boot(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) }

static struct cmd_tbl cmd_pxe_sub[] = { +#if IS_ENABLED(CONFIG_IPV6)

•   U_BOOT_CMD_MKENT(get, 2, 1, do_pxe_get, "", ""),
  

•   U_BOOT_CMD_MKENT(boot, 3, 1, do_pxe_boot, "", "")
  

+#else U_BOOT_CMD_MKENT(get, 1, 1, do_pxe_get, "", ""), U_BOOT_CMD_MKENT(boot, 2, 1, do_pxe_boot, "", "") +#endif

That's a bit ugly. How about using the max and then checking it in the C code?

...

};

static void __maybe_unused pxe_reloc(void) @@ -281,9 +339,19 @@ static int do_pxe(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_USAGE; }

+#if IS_ENABLED(CONFIG_IPV6) +U_BOOT_CMD(pxe, 4, 1, do_pxe,

•      "commands to get and boot from pxe files\n"
  

•      "To use IPv6 add -ipv6 parameter",
  

•      "get [" USE_IP6_CMD_PARAM "] - try to retrieve a pxe file using tftp\n"
  

•      "pxe boot [pxefile_addr_r] [-ipv6] - boot from the pxe file at pxefile_addr_r\n"
  

+); +#else U_BOOT_CMD(pxe, 3, 1, do_pxe, "commands to get and boot from pxe files", "get - try to retrieve a pxe file using tftp\n" "pxe boot [pxefile_addr_r] - boot from the pxe file at pxefile_addr_r\n" ); #endif

same here

...

+#endif /* CONFIG_CMD_NET */

[..]

Regards, Simon