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

[Sean Edmond <seanedmond@linux.microsoft.com>]

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