[U-Boot] [PATCH 2/2] efi_loader: set the dhcp ack received flag

[Heinrich Schuchardt]

On 03/25/2018 07:44 PM, Patrick Wildt wrote:
> On Fri, Mar 23, 2018 at 08:04:27PM +0100, Heinrich Schuchardt wrote:
>> On 03/23/2018 08:01 PM, Heinrich Schuchardt wrote:
>>> >From 689ada7663efae5ef13d021f3266e081d1d53293 Mon Sep 17 00:00:00 2001
>>> From: Patrick Wildt <patrick at blueri.se>
>>> Date: Fri, 23 Mar 2018 15:38:48 +0100
>>> Subject: [PATCH 2/2] efi_loader: set the dhcp ack received flag
>>>
>>> The PXE object contains a flag that specifies whether or not a DHCP
>>> ACK has been received.  This might be used by programs to find out
>>> whether or not it is worth to read the DHCP information ot ouf our
>>> object.
>>>
>>
>> Why should we implement this change now without a consumer for the
>> information?
>>
>>> Signed-off-by: Patrick Wildt <patrick at blueri.se>
>>> ---
>>>  include/efi_api.h        | 4 +++-
>>>  lib/efi_loader/efi_net.c | 4 +++-
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>> index 3ba650e57e..7dfa17f5c6 100644
>>> --- a/include/efi_api.h
>>> +++ b/include/efi_api.h
>>> @@ -756,7 +756,9 @@ struct efi_pxe_packet {
>>>
>>>  struct efi_pxe_mode
>>>  {
>>> -	u8 unused[52];
>>> +	u8 unused1[9];
>>> +	u8 dhcp_ack_received;
>>
>> Why use a byte in the middle of the unused region?
> 
> The EFI Spec defines shared interfaces.  In this case u-boot implements
> the PXE Base Code Protocol, and "struct efi_pxe_mode" is a definition
> for a struct that is shared on the interface.  The struct is defined in
> UEFI Spec 2.6 Chapter 23.3 as EFI_PXE_BASE_CODE_MODE.  In this struct,
> theres a boolean called DhcpAckReceivd, which is the 10th member of the
> struct.  Since booleans in EFI are defined to uint8_t, this means it's
> the 10th byte starting from the beginning of the struct.  Since u-boot
> does not define all of the members of the struct, the first 52 bytes are
> "unused".  Since I am now accessing a field in the interface in the
> middle of those 52 bytes, it is split up into a first set and a second
> set of unused bytes, with the new dhcp_ack_received in the middle.
> Thus, it's not just a simple byte in the middle of an unused region and
> putting it somewhere else would violate the spec.
> 
> The consumer in this case would be the EFI Application being booted
> using the "bootefi" command.
> 
> Please correct me if I'm wrong.
> 
> Best regards,
> Patrick

Thank you for the explanation. I think the right way go ahead is to add
all missing fields and to do away with unused[].

Please, carefully observe the alignment. The spec defines BOOLEAN as
8bit value. ToS is the 19th byte followed by an EFI_IP_ADDRESS which is
a 16-byte buffer aligned on a 4-byte boundary. So after ToS we need one
byte to ensure alignment. We could define a struct efi_ip_address as
u8 a[16] __attribute__((aligned(4))).

Best regards

Heinrich

> 
>>
>> Best regards
>>
>> Heinrich
>>
>>> +	u8 unused2[42];
>>>  	struct efi_pxe_packet dhcp_discover;
>>>  	struct efi_pxe_packet dhcp_ack;
>>>  	struct efi_pxe_packet proxy_offer;
>>> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
>>> index 8c5d5b492c..0b9c7b9345 100644
>>> --- a/lib/efi_loader/efi_net.c
>>> +++ b/lib/efi_loader/efi_net.c
>>> @@ -332,8 +332,10 @@ int efi_net_register(void)
>>>  	netobj->net_mode.max_packet_size = PKTSIZE;
>>>
>>>  	netobj->pxe.mode = &netobj->pxe_mode;
>>> -	if (dhcp_ack)
>>> +	if (dhcp_ack) {
>>>  		netobj->pxe_mode.dhcp_ack = *dhcp_ack;
>>> +		netobj->pxe_mode.dhcp_ack_received = 1;
>>> +	}
>>>
>>>  	/*
>>>  	 * Create WaitForPacket event.
>>>
>>
>