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@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@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.

On 03/25/2018 10:23 PM, Patrick Wildt wrote:

On Sun, Mar 25, 2018 at 10:04:55PM +0200, Heinrich Schuchardt wrote:

...

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

I have noticed that, yes. I think explicitly padding the struct gives better visibility of the issue, instead of relying on an implicit alignment. Two other structures in u-boot EFI headers contain explicit "pad" members. I'd feel safer to go that route. What do you think about the following?

Best regards, Patrick

I think it is fine to use a padding byte. But still the alignment should be specified for efi_ip_address. Otherwise we might pass data with wrong alignment.

Best regards

Heinrich

diff --git a/include/efi_api.h b/include/efi_api.h index 3ba650e57e..489ff476a4 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -756,7 +756,28 @@ struct efi_pxe_packet {

struct efi_pxe_mode {

• u8 unused[52];

• u8 started;
• u8 ipv6_available;
• u8 ipv6_supported;
• u8 using_ipv6;
• u8 bis_supported;
• u8 bis_detected;
• u8 auto_arp;
• u8 send_guid;
• u8 dhcp_discover_valid;
• u8 dhcp_ack_received;
• u8 proxy_offer_received;
• u8 pxe_discovervalid;
• u8 pxe_reply_received;
• u8 pxe_bis_reply_received;
• u8 icmp_error_received;
• u8 tftp_error_received;
• u8 make_callbacks;
• u8 ttl;
• u8 tos;
• u8 pad;
• struct efi_ip_address station_ip;
• struct efi_ip_address subnet_mask; struct efi_pxe_packet dhcp_discover; struct efi_pxe_packet dhcp_ack; struct efi_pxe_packet proxy_offer;

On 26.03.18 15:28, Patrick Wildt wrote:

On Mon, Mar 26, 2018 at 06:39:06AM +0200, Heinrich Schuchardt wrote:

...

On 03/25/2018 10:23 PM, Patrick Wildt wrote:

...

On Sun, Mar 25, 2018 at 10:04:55PM +0200, Heinrich Schuchardt wrote:

...

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

I have noticed that, yes. I think explicitly padding the struct gives better visibility of the issue, instead of relying on an implicit alignment. Two other structures in u-boot EFI headers contain explicit "pad" members. I'd feel safer to go that route. What do you think about the following?

Best regards, Patrick

I think it is fine to use a padding byte. But still the alignment should be specified for efi_ip_address. Otherwise we might pass data with wrong alignment.

Best regards

Heinrich

EDK2 does this by defining EFI_IP_ADDRESS to a union which includes a uint32_t addr[4], but I guess the attribute makes it more explicit. This should do:

Looks good to me. Can you please resubmit as real patch with proper patch description, SoB line and CC everyone on this thread?

Thanks!

Alex

Best regards, Patrick

diff --git a/include/efi_api.h b/include/efi_api.h index 3ba650e57e..06789acdd1 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -662,7 +662,7 @@ struct efi_mac_address {

struct efi_ip_address { u8 ip_addr[16]; -}; +} __attribute__((aligned(4)));

enum efi_simple_network_state { EFI_NETWORK_STOPPED, @@ -756,7 +756,28 @@ struct efi_pxe_packet {

struct efi_pxe_mode {

• u8 unused[52];

• u8 started;
• u8 ipv6_available;
• u8 ipv6_supported;
• u8 using_ipv6;
• u8 bis_supported;
• u8 bis_detected;
• u8 auto_arp;
• u8 send_guid;
• u8 dhcp_discover_valid;
• u8 dhcp_ack_received;
• u8 proxy_offer_received;
• u8 pxe_discovervalid;
• u8 pxe_reply_received;
• u8 pxe_bis_reply_received;
• u8 icmp_error_received;
• u8 tftp_error_received;
• u8 make_callbacks;
• u8 ttl;
• u8 tos;
• u8 pad;
• struct efi_ip_address station_ip;
• struct efi_ip_address subnet_mask; struct efi_pxe_packet dhcp_discover; struct efi_pxe_packet dhcp_ack; struct efi_pxe_packet proxy_offer;