From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3ABEBC76196 for ; Tue, 11 Apr 2023 00:03:40 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C4FE7859AD; Tue, 11 Apr 2023 02:03:37 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=linux.microsoft.com header.i=@linux.microsoft.com header.b="Z7kw17dr"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id A815E85D37; Tue, 11 Apr 2023 02:03:33 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by phobos.denx.de (Postfix) with ESMTP id 1802D85D37 for ; Tue, 11 Apr 2023 02:03:28 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanedmond@linux.microsoft.com Received: from [192.168.1.68] (d172-218-241-181.bchsia.telus.net [172.218.241.181]) by linux.microsoft.com (Postfix) with ESMTPSA id BD6A82174E2F for ; Mon, 10 Apr 2023 17:03:26 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com BD6A82174E2F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1681171406; bh=2VLP7SlBKR6vQOf/xNLTXoJNXtA4SL2t7EX0qzUqW08=; h=Date:Subject:To:References:From:In-Reply-To:From; b=Z7kw17drN6YyOdFdiII1SbNtwS7V7zgF9cIHpmII3Hlh6cMvwMWuDLammz+J/NdPz s5a/CsSH2E7d7cst5h4gAScjWR6gCEGzNJqkO46+gMp19kjL9UEzq1OCg9VLrZ04Rp 8i1T8hgFThXoj1i6V0SRqtKnCYvlw1mly8rRZdwQ= Message-ID: Date: Mon, 10 Apr 2023 17:03:26 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH v2 2/3] net: dhcp6: pxe: Add DHCP/PXE commands for IPv6 To: u-boot@lists.denx.de References: <20230407065619.15908-1-seanedmond@linux.microsoft.com> <20230407065619.15908-3-seanedmond@linux.microsoft.com> From: Sean Edmond In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On 2023-04-07 11:55 a.m., Simon Glass wrote: > Hi Sean, > > On Fri, 7 Apr 2023 at 18:56, wrote: >> From: Sean Edmond >> >> 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 >> --- >> 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 > > [..] > >> +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 >> #include >> #include >> +#include >> >> #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