From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web11.107886.1597920310895612025 for ; Thu, 20 Aug 2020 03:45:10 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.151, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: hkJw53fcO7XWtD+r6Xd+sTpwlXXzvl3kbNUIeOM9DR9XyrpjVtp9BQl6tUH9SafPKADHlS4jvZ rYJhzRgK0soQ== X-IronPort-AV: E=McAfee;i="6000,8403,9718"; a="135340103" X-IronPort-AV: E=Sophos;i="5.76,332,1592895600"; d="scan'208";a="135340103" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Aug 2020 03:45:10 -0700 IronPort-SDR: 0pszJDk59G16nUSdI1Gbjj7CLL5ZPg3p6o3J37Zba3sW5lOq2SgJXSMWmJ8usMF4h9y4dJf17K YxibHC6wQMRw== X-IronPort-AV: E=Sophos;i="5.76,332,1592895600"; d="scan'208";a="472608852" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.249.158.232]) ([10.249.158.232]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Aug 2020 03:45:08 -0700 Subject: Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow To: devel@edk2.groups.io, lersek@redhat.com Cc: Jiaxin Wu , Siyuan Fu , Seven.ding@lcfuturecenter.com References: <20200819165338.681-1-maciej.rabeda@linux.intel.com> <2dde2087-e291-0232-62e2-a30cdf4e09b2@redhat.com> <590e1de9-9f8c-72df-205e-c767f788ecf3@redhat.com> From: "Maciej Rabeda" Message-ID: Date: Thu, 20 Aug 2020 12:44:58 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <590e1de9-9f8c-72df-205e-c767f788ecf3@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: pl @Laszlo Thanks for regression test! I need to correct the typo in commit msg, (bit 3 -> bit 2). Code logic is not affected. @Michael I am now wondering whether bit 3 is actually relevant to server choice. Bit 3: == 0 -> prompt user to choose a boot file. Which means to me: show minimal menu with prompt (tag 10 - PXE_MENU_PROMPT) and options (tag 9 - PXE_BOOT_MENU). == 1 -> do not prompt user. If boot file name is present (option 67), download that boot file. Bit 3 does not seem to specify/regulate which server to use. Choice of server IP might look like: if (option 43 is present, tag 6 is present, tag_6.bit_2 is set and tag 8 is present and valid)         take server IP from tag 8 (PXE_BOOT_SERVERS) else if (option 66 is present)         take server IP from option 66 (TFTP server name) else if (option 54 is present)         take server IP from option 54 (Server Identifier) else         failure Looking forward to discussion. Thanks, Maciej On 19-Aug-20 21:20, Laszlo Ersek wrote: > On 08/19/20 20:13, Laszlo Ersek wrote: >> On 08/19/20 18:53, Maciej Rabeda wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2876 >>> >>> According to PXE 2.1 spec, DHCP option 43, tag 6 (PXE_DISCOVERY_CONTROL), >>> bit 3 specifies whether PXE client should use/accept TFTP servers defined >>> within PXE_BOOT_SERVERS list (tag 8). This bit was not being taken into >>> account when choosing boot server IP in PxeBcDhcp4BootInfo(). >>> >>> Cc: Jiaxin Wu >>> Cc: Siyuan Fu >>> Signed-off-by: Maciej Rabeda >>> --- >>> NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c >>> index d062a526077b..ed9bca0f7de3 100644 >>> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c >>> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcBoot.c >>> @@ -499,12 +499,16 @@ PxeBcDhcp4BootInfo ( >>> >>> // >>> // Parse the boot server address. >>> - // If prompt/discover is disabled, get the first boot server from the boot servers list. >>> - // Otherwise, parse the boot server Ipv4 address from next server address field in DHCP header. >>> + // If prompt/discover is disabled, server list should be used and is present (DHCP option 43), >>> + // get the first boot server from the boot servers list. >>> + // Otherwise, parse the boot server IPv4 address from next server address field in DHCP header. >>> // If all these fields are not available, use option 54 instead. >>> // >>> VendorOpt = &Cache4->VendorOpt; >>> - if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) && IS_VALID_BOOT_SERVERS (VendorOpt->BitMap)) { >>> + if (IS_DISABLE_PROMPT_MENU (VendorOpt->DiscoverCtrl) && >>> + IS_VALID_BOOT_SERVERS (VendorOpt->BitMap) && >>> + IS_ENABLE_USE_SERVER_LIST (VendorOpt->DiscoverCtrl)) >>> + { >>> Entry = VendorOpt->BootSvr; >>> if (VendorOpt->BootSvrLen >= sizeof (PXEBC_BOOT_SVR_ENTRY) && Entry->IpCnt > 0) { >>> CopyMem ( >>> >> I'm still undecided whether option#43 / tag#6 / bit#3 being clear means >> we should *ignore* PXE_BOOT_SERVERS (tag#8), but I'm willing to defer to >> you on that. So, I can give a cautious >> >> Reviewed-by: Laszlo Ersek >> >> for this patch. >> >> Regarding the feature freeze -- in theory, this is a bugfix. However, >> before we merge it, it would be really nice to get feedback from the >> original reporter (CC'd). >> >> I also intend to regression-test this patch, I'll report back. > I've repeated my usual PXEv4 boot tests (using OVMF IA32X64 and > ArmVirtQemu AARCH64 guests, covering shim + grub + vmlinuz + initrd); > nothing seems to have regressed. > > Regression-tested-by: Laszlo Ersek > > Thanks > Laszlo > > > >