Re: [edk2-devel] [PATCH v1] NetworkPkg/UefiPxeBcDxe: Fix PXE_BOOT_SERVERS usage in boot info parse flow

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

@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 <jiaxin.wu@intel.com>
>>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>>> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>>> ---
>>>   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 <lersek@redhat.com>
>>
>> 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 <lersek@redhat.com>
>
> Thanks
> Laszlo
>
>
> 
>