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

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

[Heinrich Schuchardt]

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?

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

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

[Patrick Wildt]

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

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

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

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

[Patrick Wildt]

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

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;

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

[Heinrich Schuchardt]

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;
> 

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

[Patrick Wildt]

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:

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;

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

[Alexander Graf]

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;
> 

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

[Patrick Wildt]

The PXE object contains a flag that specifies whether or not a DHCP
ACK has been received.  This can be used by EFI Applications to find
out whether or not it is worth to read the DHCP information from our
object.

Signed-off-by: Patrick Wildt <patrick@blueri.se>
---
 lib/efi_loader/efi_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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.
-- 
2.14.3 (Apple Git-98)

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

[Heinrich Schuchardt]

On 03/27/2018 02:24 PM, Patrick Wildt wrote:
> The PXE object contains a flag that specifies whether or not a DHCP
> ACK has been received.  This can be used by EFI Applications to find
> out whether or not it is worth to read the DHCP information from our
> object.
> 
> Signed-off-by: Patrick Wildt <patrick@blueri.se>
> ---
>  lib/efi_loader/efi_net.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 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;
> +	}

We have received a DHCPOFFER and we now send a DHCPREQUEST to the
selected server. This is when efi_net_set_dhcp_ack() is called which
sets the variable dhcp_ack.

If the server sustains its offer it responds with a DHCPACK or with a
DHCPNACK. Shouldn't we ensure a DHCPACK was received (and not a DHCNACK)
before setting dhcp_ack_received?

Best regards

Heinrich

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

[Alexander Graf]

On 27.03.18 18:05, Heinrich Schuchardt wrote:
> On 03/27/2018 02:24 PM, Patrick Wildt wrote:
>> The PXE object contains a flag that specifies whether or not a DHCP
>> ACK has been received.  This can be used by EFI Applications to find
>> out whether or not it is worth to read the DHCP information from our
>> object.
>>
>> Signed-off-by: Patrick Wildt <patrick@blueri.se>
>> ---
>>  lib/efi_loader/efi_net.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> 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;
>> +	}
> 
> We have received a DHCPOFFER and we now send a DHCPREQUEST to the
> selected server. This is when efi_net_set_dhcp_ack() is called which
> sets the variable dhcp_ack.
> 
> If the server sustains its offer it responds with a DHCPACK or with a
> DHCPNACK. Shouldn't we ensure a DHCPACK was received (and not a DHCNACK)
> before setting dhcp_ack_received?

Patrick, ping? :)


Alex

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

[Patrick Wildt]

On Sun, Dec 02, 2018 at 10:21:12PM +0100, Alexander Graf wrote:
> On 27.03.18 18:05, Heinrich Schuchardt wrote:
> > On 03/27/2018 02:24 PM, Patrick Wildt wrote:
> >> The PXE object contains a flag that specifies whether or not a DHCP
> >> ACK has been received.  This can be used by EFI Applications to find
> >> out whether or not it is worth to read the DHCP information from our
> >> object.
> >>
> >> Signed-off-by: Patrick Wildt <patrick@blueri.se>
> >> ---
> >>  lib/efi_loader/efi_net.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> 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;
> >> +	}
> > 
> > We have received a DHCPOFFER and we now send a DHCPREQUEST to the
> > selected server. This is when efi_net_set_dhcp_ack() is called which
> > sets the variable dhcp_ack.
> > 
> > If the server sustains its offer it responds with a DHCPACK or with a
> > DHCPNACK. Shouldn't we ensure a DHCPACK was received (and not a DHCNACK)
> > before setting dhcp_ack_received?
> 
> Patrick, ping? :)
> 
> Alex

Sorry, I had a bit of other stuff to take care of and didn't get to it.
Turns out the change is rather easy to do, since we can just add another
function in the EFI subsystem to call once we receive the actual ACK.
This version works for me.

Patrick

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 53f08161ab..3dcfc4b16f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -285,6 +285,8 @@ efi_fs_from_path(struct efi_device_path *fp);
 
 /* Called by networking code to memorize the dhcp ack package */
 void efi_net_set_dhcp_ack(void *pkt, int len);
+/* Called by networking code to memorize we got an ack */
+void efi_net_set_dhcp_ack_received(void);
 /* Called by efi_set_watchdog_timer to reset the timer */
 efi_status_t efi_set_watchdog(unsigned long timeout);
 
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index c7d9da8521..96610c768e 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -15,6 +15,7 @@ static struct efi_pxe_packet *dhcp_ack;
 static bool new_rx_packet;
 static void *new_tx_packet;
 static void *transmit_buffer;
+static int dhcp_ack_received;
 
 /*
  * The notification function of this event is called in every timer cycle
@@ -536,6 +537,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
 	memcpy(dhcp_ack, pkt, min(len, maxsize));
 }
 
+/**
+ * efi_net_set_dhcp_ack_received() - take note of a received ACK
+ *
+ * This function is called by dhcp_handler().
+ */
+void efi_net_set_dhcp_ack_received(void)
+{
+	dhcp_ack_received = 1;
+}
+
 /**
  * efi_net_push() - callback for received network packet
  *
@@ -645,8 +656,10 @@ efi_status_t efi_net_register(void)
 	netobj->net_mode.if_type = ARP_ETHER;
 
 	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 = dhcp_ack_received;
+	}
 
 	/*
 	 * Create WaitForPacket event.
diff --git a/net/bootp.c b/net/bootp.c
index 9a2b512e4a..b0c26418ca 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -1087,6 +1087,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 			dhcp_packet_process_options(bp);
 			/* Store net params from reply */
 			store_net_params(bp);
+			efi_net_set_dhcp_ack_received();
 			dhcp_state = BOUND;
 			printf("DHCP client bound to address %pI4 (%lu ms)\n",
 			       &net_ip, get_timer(bootp_start));

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

[Alexander Graf]

On 01/31/2019 03:25 PM, Patrick Wildt wrote:
> On Sun, Dec 02, 2018 at 10:21:12PM +0100, Alexander Graf wrote:
>> On 27.03.18 18:05, Heinrich Schuchardt wrote:
>>> On 03/27/2018 02:24 PM, Patrick Wildt wrote:
>>>> The PXE object contains a flag that specifies whether or not a DHCP
>>>> ACK has been received.  This can be used by EFI Applications to find
>>>> out whether or not it is worth to read the DHCP information from our
>>>> object.
>>>>
>>>> Signed-off-by: Patrick Wildt <patrick@blueri.se>
>>>> ---
>>>>   lib/efi_loader/efi_net.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> 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;
>>>> +	}
>>> We have received a DHCPOFFER and we now send a DHCPREQUEST to the
>>> selected server. This is when efi_net_set_dhcp_ack() is called which
>>> sets the variable dhcp_ack.
>>>
>>> If the server sustains its offer it responds with a DHCPACK or with a
>>> DHCPNACK. Shouldn't we ensure a DHCPACK was received (and not a DHCNACK)
>>> before setting dhcp_ack_received?
>> Patrick, ping? :)
>>
>> Alex
> Sorry, I had a bit of other stuff to take care of and didn't get to it.
> Turns out the change is rather easy to do, since we can just add another
> function in the EFI subsystem to call once we receive the actual ACK.
> This version works for me.
>
> Patrick
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 53f08161ab..3dcfc4b16f 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -285,6 +285,8 @@ efi_fs_from_path(struct efi_device_path *fp);
>   
>   /* Called by networking code to memorize the dhcp ack package */
>   void efi_net_set_dhcp_ack(void *pkt, int len);
> +/* Called by networking code to memorize we got an ack */
> +void efi_net_set_dhcp_ack_received(void);
>   /* Called by efi_set_watchdog_timer to reset the timer */
>   efi_status_t efi_set_watchdog(unsigned long timeout);
>   
> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> index c7d9da8521..96610c768e 100644
> --- a/lib/efi_loader/efi_net.c
> +++ b/lib/efi_loader/efi_net.c
> @@ -15,6 +15,7 @@ static struct efi_pxe_packet *dhcp_ack;
>   static bool new_rx_packet;
>   static void *new_tx_packet;
>   static void *transmit_buffer;
> +static int dhcp_ack_received;
>   
>   /*
>    * The notification function of this event is called in every timer cycle
> @@ -536,6 +537,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
>   	memcpy(dhcp_ack, pkt, min(len, maxsize));
>   }
>   
> +/**
> + * efi_net_set_dhcp_ack_received() - take note of a received ACK
> + *
> + * This function is called by dhcp_handler().
> + */
> +void efi_net_set_dhcp_ack_received(void)
> +{
> +	dhcp_ack_received = 1;

Is there any way to pass an object reference in as parameter that allows 
us to find the exact efi net object back again? IIRC the network code 
was pretty much single target, but maybe we don't have to encode that in 
every single place ...

> +}
> +
>   /**
>    * efi_net_push() - callback for received network packet
>    *
> @@ -645,8 +656,10 @@ efi_status_t efi_net_register(void)
>   	netobj->net_mode.if_type = ARP_ETHER;
>   
>   	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 = dhcp_ack_received;
> +	}
>   
>   	/*
>   	 * Create WaitForPacket event.
> diff --git a/net/bootp.c b/net/bootp.c
> index 9a2b512e4a..b0c26418ca 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -1087,6 +1087,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>   			dhcp_packet_process_options(bp);
>   			/* Store net params from reply */
>   			store_net_params(bp);
> +			efi_net_set_dhcp_ack_received();

Won't this fail to compile with !CONFIG_EFI_LOADER?

Alex

>   			dhcp_state = BOUND;
>   			printf("DHCP client bound to address %pI4 (%lu ms)\n",
>   			       &net_ip, get_timer(bootp_start));

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

[Patrick Wildt]

On Thu, Jan 31, 2019 at 03:31:34PM +0100, Alexander Graf wrote:
> On 01/31/2019 03:25 PM, Patrick Wildt wrote:
> > On Sun, Dec 02, 2018 at 10:21:12PM +0100, Alexander Graf wrote:
> > > On 27.03.18 18:05, Heinrich Schuchardt wrote:
> > > > On 03/27/2018 02:24 PM, Patrick Wildt wrote:
> > > > > The PXE object contains a flag that specifies whether or not a DHCP
> > > > > ACK has been received.  This can be used by EFI Applications to find
> > > > > out whether or not it is worth to read the DHCP information from our
> > > > > object.
> > > > > 
> > > > > Signed-off-by: Patrick Wildt <patrick@blueri.se>
> > > > > ---
> > > > >   lib/efi_loader/efi_net.c | 4 +++-
> > > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > 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;
> > > > > +	}
> > > > We have received a DHCPOFFER and we now send a DHCPREQUEST to the
> > > > selected server. This is when efi_net_set_dhcp_ack() is called which
> > > > sets the variable dhcp_ack.
> > > > 
> > > > If the server sustains its offer it responds with a DHCPACK or with a
> > > > DHCPNACK. Shouldn't we ensure a DHCPACK was received (and not a DHCNACK)
> > > > before setting dhcp_ack_received?
> > > Patrick, ping? :)
> > > 
> > > Alex
> > Sorry, I had a bit of other stuff to take care of and didn't get to it.
> > Turns out the change is rather easy to do, since we can just add another
> > function in the EFI subsystem to call once we receive the actual ACK.
> > This version works for me.
> > 
> > Patrick
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 53f08161ab..3dcfc4b16f 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -285,6 +285,8 @@ efi_fs_from_path(struct efi_device_path *fp);
> >   /* Called by networking code to memorize the dhcp ack package */
> >   void efi_net_set_dhcp_ack(void *pkt, int len);
> > +/* Called by networking code to memorize we got an ack */
> > +void efi_net_set_dhcp_ack_received(void);
> >   /* Called by efi_set_watchdog_timer to reset the timer */
> >   efi_status_t efi_set_watchdog(unsigned long timeout);
> > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> > index c7d9da8521..96610c768e 100644
> > --- a/lib/efi_loader/efi_net.c
> > +++ b/lib/efi_loader/efi_net.c
> > @@ -15,6 +15,7 @@ static struct efi_pxe_packet *dhcp_ack;
> >   static bool new_rx_packet;
> >   static void *new_tx_packet;
> >   static void *transmit_buffer;
> > +static int dhcp_ack_received;
> >   /*
> >    * The notification function of this event is called in every timer cycle
> > @@ -536,6 +537,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
> >   	memcpy(dhcp_ack, pkt, min(len, maxsize));
> >   }
> > +/**
> > + * efi_net_set_dhcp_ack_received() - take note of a received ACK
> > + *
> > + * This function is called by dhcp_handler().
> > + */
> > +void efi_net_set_dhcp_ack_received(void)
> > +{
> > +	dhcp_ack_received = 1;
> 
> Is there any way to pass an object reference in as parameter that allows us
> to find the exact efi net object back again? IIRC the network code was
> pretty much single target, but maybe we don't have to encode that in every
> single place ...

I agree, but I have to say that I don't know the u-boot code enough to
know that.

> > +}
> > +
> >   /**
> >    * efi_net_push() - callback for received network packet
> >    *
> > @@ -645,8 +656,10 @@ efi_status_t efi_net_register(void)
> >   	netobj->net_mode.if_type = ARP_ETHER;
> >   	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 = dhcp_ack_received;
> > +	}
> >   	/*
> >   	 * Create WaitForPacket event.
> > diff --git a/net/bootp.c b/net/bootp.c
> > index 9a2b512e4a..b0c26418ca 100644
> > --- a/net/bootp.c
> > +++ b/net/bootp.c
> > @@ -1087,6 +1087,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
> >   			dhcp_packet_process_options(bp);
> >   			/* Store net params from reply */
> >   			store_net_params(bp);
> > +			efi_net_set_dhcp_ack_received();
> 
> Won't this fail to compile with !CONFIG_EFI_LOADER?
> 
> Alex

Turns out I sent an older diff.  I had made that change, but I forgot
to append the right one.  Sorry for the mishap.

Patrick

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 53f08161ab..4f131e52c2 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -285,6 +285,8 @@ efi_fs_from_path(struct efi_device_path *fp);
 
 /* Called by networking code to memorize the dhcp ack package */
 void efi_net_set_dhcp_ack(void *pkt, int len);
+/* Called by networking code to memorize we got an ack */
+void efi_net_set_dhcp_ack_received(void);
 /* Called by efi_set_watchdog_timer to reset the timer */
 efi_status_t efi_set_watchdog(unsigned long timeout);
 
@@ -559,6 +561,7 @@ static inline void efi_restore_gd(void) { }
 static inline void efi_set_bootdev(const char *dev, const char *devnr,
 				   const char *path) { }
 static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
+static inline void efi_net_set_dhcp_ack_received(void) { }
 static inline void efi_print_image_infos(void *pc) { }
 
 #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index c7d9da8521..96610c768e 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -15,6 +15,7 @@ static struct efi_pxe_packet *dhcp_ack;
 static bool new_rx_packet;
 static void *new_tx_packet;
 static void *transmit_buffer;
+static int dhcp_ack_received;
 
 /*
  * The notification function of this event is called in every timer cycle
@@ -536,6 +537,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
 	memcpy(dhcp_ack, pkt, min(len, maxsize));
 }
 
+/**
+ * efi_net_set_dhcp_ack_received() - take note of a received ACK
+ *
+ * This function is called by dhcp_handler().
+ */
+void efi_net_set_dhcp_ack_received(void)
+{
+	dhcp_ack_received = 1;
+}
+
 /**
  * efi_net_push() - callback for received network packet
  *
@@ -645,8 +656,10 @@ efi_status_t efi_net_register(void)
 	netobj->net_mode.if_type = ARP_ETHER;
 
 	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 = dhcp_ack_received;
+	}
 
 	/*
 	 * Create WaitForPacket event.
diff --git a/net/bootp.c b/net/bootp.c
index 9a2b512e4a..b0c26418ca 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -1087,6 +1087,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 			dhcp_packet_process_options(bp);
 			/* Store net params from reply */
 			store_net_params(bp);
+			efi_net_set_dhcp_ack_received();
 			dhcp_state = BOUND;
 			printf("DHCP client bound to address %pI4 (%lu ms)\n",
 			       &net_ip, get_timer(bootp_start));

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

[Heinrich Schuchardt]

On 1/31/19 3:54 PM, Patrick Wildt wrote:
> On Thu, Jan 31, 2019 at 03:31:34PM +0100, Alexander Graf wrote:
>> On 01/31/2019 03:25 PM, Patrick Wildt wrote:
>>> On Sun, Dec 02, 2018 at 10:21:12PM +0100, Alexander Graf wrote:
>>>> On 27.03.18 18:05, Heinrich Schuchardt wrote:
>>>>> On 03/27/2018 02:24 PM, Patrick Wildt wrote:
>>>>>> The PXE object contains a flag that specifies whether or not a DHCP
>>>>>> ACK has been received.  This can be used by EFI Applications to find
>>>>>> out whether or not it is worth to read the DHCP information from our
>>>>>> object.
>>>>>>
>>>>>> Signed-off-by: Patrick Wildt <patrick@blueri.se>
>>>>>> ---
>>>>>>   lib/efi_loader/efi_net.c | 4 +++-
>>>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> 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;
>>>>>> +	}
>>>>> We have received a DHCPOFFER and we now send a DHCPREQUEST to the
>>>>> selected server. This is when efi_net_set_dhcp_ack() is called which
>>>>> sets the variable dhcp_ack.
>>>>>
>>>>> If the server sustains its offer it responds with a DHCPACK or with a
>>>>> DHCPNACK. Shouldn't we ensure a DHCPACK was received (and not a DHCNACK)
>>>>> before setting dhcp_ack_received?
>>>> Patrick, ping? :)
>>>>
>>>> Alex
>>> Sorry, I had a bit of other stuff to take care of and didn't get to it.
>>> Turns out the change is rather easy to do, since we can just add another
>>> function in the EFI subsystem to call once we receive the actual ACK.
>>> This version works for me.
>>>
>>> Patrick
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 53f08161ab..3dcfc4b16f 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -285,6 +285,8 @@ efi_fs_from_path(struct efi_device_path *fp);
>>>   /* Called by networking code to memorize the dhcp ack package */
>>>   void efi_net_set_dhcp_ack(void *pkt, int len);
>>> +/* Called by networking code to memorize we got an ack */
>>> +void efi_net_set_dhcp_ack_received(void);
>>>   /* Called by efi_set_watchdog_timer to reset the timer */
>>>   efi_status_t efi_set_watchdog(unsigned long timeout);
>>> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
>>> index c7d9da8521..96610c768e 100644
>>> --- a/lib/efi_loader/efi_net.c
>>> +++ b/lib/efi_loader/efi_net.c
>>> @@ -15,6 +15,7 @@ static struct efi_pxe_packet *dhcp_ack;
>>>   static bool new_rx_packet;
>>>   static void *new_tx_packet;
>>>   static void *transmit_buffer;
>>> +static int dhcp_ack_received;
>>>   /*
>>>    * The notification function of this event is called in every timer cycle
>>> @@ -536,6 +537,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
>>>   	memcpy(dhcp_ack, pkt, min(len, maxsize));
>>>   }
>>> +/**
>>> + * efi_net_set_dhcp_ack_received() - take note of a received ACK
>>> + *
>>> + * This function is called by dhcp_handler().
>>> + */
>>> +void efi_net_set_dhcp_ack_received(void)

Do we really need multiple functions to update the DHCP status?

I would prefer a single function with a parameter telling which DHCP
status has been reached.

>>> +{
>>> +	dhcp_ack_received = 1;
>>
>> Is there any way to pass an object reference in as parameter that allows us
>> to find the exact efi net object back again? IIRC the network code was
>> pretty much single target, but maybe we don't have to encode that in every
>> single place ...
> 
> I agree, but I have to say that I don't know the u-boot code enough to
> know that.

The current network device can be determined via eth_get_dev().
In function eth_current_changed() this eth_get_dev() function is used to
update the ethact environment variable.

If we have a single update function we can determine the active network
device there.

Best regards

Heinrich

> 
>>> +}
>>> +
>>>   /**
>>>    * efi_net_push() - callback for received network packet
>>>    *
>>> @@ -645,8 +656,10 @@ efi_status_t efi_net_register(void)
>>>   	netobj->net_mode.if_type = ARP_ETHER;
>>>   	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 = dhcp_ack_received;
>>> +	}
>>>   	/*
>>>   	 * Create WaitForPacket event.
>>> diff --git a/net/bootp.c b/net/bootp.c
>>> index 9a2b512e4a..b0c26418ca 100644
>>> --- a/net/bootp.c
>>> +++ b/net/bootp.c
>>> @@ -1087,6 +1087,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>>>   			dhcp_packet_process_options(bp);
>>>   			/* Store net params from reply */
>>>   			store_net_params(bp);
>>> +			efi_net_set_dhcp_ack_received();
>>
>> Won't this fail to compile with !CONFIG_EFI_LOADER?
>>
>> Alex
> 
> Turns out I sent an older diff.  I had made that change, but I forgot
> to append the right one.  Sorry for the mishap.
> 
> Patrick
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 53f08161ab..4f131e52c2 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -285,6 +285,8 @@ efi_fs_from_path(struct efi_device_path *fp);
>  
>  /* Called by networking code to memorize the dhcp ack package */
>  void efi_net_set_dhcp_ack(void *pkt, int len);
> +/* Called by networking code to memorize we got an ack */
> +void efi_net_set_dhcp_ack_received(void);
>  /* Called by efi_set_watchdog_timer to reset the timer */
>  efi_status_t efi_set_watchdog(unsigned long timeout);
>  
> @@ -559,6 +561,7 @@ static inline void efi_restore_gd(void) { }
>  static inline void efi_set_bootdev(const char *dev, const char *devnr,
>  				   const char *path) { }
>  static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
> +static inline void efi_net_set_dhcp_ack_received(void) { }
>  static inline void efi_print_image_infos(void *pc) { }
>  
>  #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> index c7d9da8521..96610c768e 100644
> --- a/lib/efi_loader/efi_net.c
> +++ b/lib/efi_loader/efi_net.c
> @@ -15,6 +15,7 @@ static struct efi_pxe_packet *dhcp_ack;
>  static bool new_rx_packet;
>  static void *new_tx_packet;
>  static void *transmit_buffer;
> +static int dhcp_ack_received;
>  
>  /*
>   * The notification function of this event is called in every timer cycle
> @@ -536,6 +537,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
>  	memcpy(dhcp_ack, pkt, min(len, maxsize));
>  }
>  
> +/**
> + * efi_net_set_dhcp_ack_received() - take note of a received ACK
> + *
> + * This function is called by dhcp_handler().
> + */
> +void efi_net_set_dhcp_ack_received(void)
> +{
> +	dhcp_ack_received = 1;
> +}
> +
>  /**
>   * efi_net_push() - callback for received network packet
>   *
> @@ -645,8 +656,10 @@ efi_status_t efi_net_register(void)
>  	netobj->net_mode.if_type = ARP_ETHER;
>  
>  	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 = dhcp_ack_received;
> +	}
>  
>  	/*
>  	 * Create WaitForPacket event.
> diff --git a/net/bootp.c b/net/bootp.c
> index 9a2b512e4a..b0c26418ca 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -1087,6 +1087,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  			dhcp_packet_process_options(bp);
>  			/* Store net params from reply */
>  			store_net_params(bp);
> +			efi_net_set_dhcp_ack_received();
>  			dhcp_state = BOUND;
>  			printf("DHCP client bound to address %pI4 (%lu ms)\n",
>  			       &net_ip, get_timer(bootp_start));
> 

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

[Patrick Wildt]

On Thu, Jan 31, 2019 at 07:29:04PM +0100, Heinrich Schuchardt wrote:
> On 1/31/19 3:54 PM, Patrick Wildt wrote:
> > On Thu, Jan 31, 2019 at 03:31:34PM +0100, Alexander Graf wrote:
> >> Is there any way to pass an object reference in as parameter that allows us
> >> to find the exact efi net object back again? IIRC the network code was
> >> pretty much single target, but maybe we don't have to encode that in every
> >> single place ...
> > 
> > I agree, but I have to say that I don't know the u-boot code enough to
> > know that.
> 
> The current network device can be determined via eth_get_dev().
> In function eth_current_changed() this eth_get_dev() function is used to
> update the ethact environment variable.
> 
> If we have a single update function we can determine the active network
> device there.
> 
> Best regards
> 
> Heinrich

Looks like you know what to do.

Best regards,
Patrick

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

[Heinrich Schuchardt]

On 2/4/19 5:43 PM, Patrick Wildt wrote:
> On Thu, Jan 31, 2019 at 07:29:04PM +0100, Heinrich Schuchardt wrote:
>> On 1/31/19 3:54 PM, Patrick Wildt wrote:
>>> On Thu, Jan 31, 2019 at 03:31:34PM +0100, Alexander Graf wrote:
>>>> Is there any way to pass an object reference in as parameter that allows us
>>>> to find the exact efi net object back again? IIRC the network code was
>>>> pretty much single target, but maybe we don't have to encode that in every
>>>> single place ...
>>>
>>> I agree, but I have to say that I don't know the u-boot code enough to
>>> know that.
>>
>> The current network device can be determined via eth_get_dev().
>> In function eth_current_changed() this eth_get_dev() function is used to
>> update the ethact environment variable.
>>
>> If we have a single update function we can determine the active network
>> device there.
>>
>> Best regards
>>
>> Heinrich
> 
> Looks like you know what to do.
> 
> Best regards,
> Patrick
> 
Hello Patrick,

will you adjust the patch such that we have only a single update function?

Best regards

Heinrich

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

[Patrick Wildt]

On Thu, Jan 31, 2019 at 07:29:04PM +0100, Heinrich Schuchardt wrote:
> Do we really need multiple functions to update the DHCP status?
> 
> I would prefer a single function with a parameter telling which DHCP
> status has been reached.
> 

I think this diff below might be better.  There we have an update
function that is called after it switch the state, and on REQUESTING
we save the packet information and on BOUND we received the ack.

> 
> The current network device can be determined via eth_get_dev().
> In function eth_current_changed() this eth_get_dev() function is used to
> update the ethact environment variable.
> 
> If we have a single update function we can determine the active network
> device there.

The efi network object is only created on bootefi, so there is no DHCP
going on at that point.  It all happens some time before bootefi is
called.  The only thing that we could do is try to memorize for which
ethernet we received the DHCP packets, and when we create the EFI
network object we can try to see if the DHCP packets match to the
current ethernet we create the object for.

Patrick

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 00b81c6010..7e8f3b04b5 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -290,8 +290,8 @@ efi_status_t efi_smbios_register(void);
 struct efi_simple_file_system_protocol *
 efi_fs_from_path(struct efi_device_path *fp);
 
-/* Called by networking code to memorize the dhcp ack package */
-void efi_net_set_dhcp_ack(void *pkt, int len);
+/* Called by networking code to memorize dhcp information */
+void efi_net_update_dhcp(int state, void *pkt, int len);
 /* Called by efi_set_watchdog_timer to reset the timer */
 efi_status_t efi_set_watchdog(unsigned long timeout);
 
@@ -578,7 +578,7 @@ static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
 static inline void efi_restore_gd(void) { }
 static inline void efi_set_bootdev(const char *dev, const char *devnr,
 				   const char *path) { }
-static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
+static inline void efi_net_update_dhcp(int state, void *pkt, int len) {}
 static inline void efi_print_image_infos(void *pc) { }
 
 #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index c7d9da8521..92e13a7c13 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -8,6 +8,7 @@
 #include <common.h>
 #include <efi_loader.h>
 #include <malloc.h>
+#include "../net/bootp.h"
 
 static const efi_guid_t efi_net_guid = EFI_SIMPLE_NETWORK_GUID;
 static const efi_guid_t efi_pxe_guid = EFI_PXE_GUID;
@@ -15,6 +16,7 @@ static struct efi_pxe_packet *dhcp_ack;
 static bool new_rx_packet;
 static void *new_tx_packet;
 static void *transmit_buffer;
+static int dhcp_ack_received;
 
 /*
  * The notification function of this event is called in every timer cycle
@@ -522,18 +524,22 @@ out:
 }
 
 /**
- * efi_net_set_dhcp_ack() - take note of a selected DHCP IP address
+ * efi_net_update_dhcp() - take note of DHCP information
  *
  * This function is called by dhcp_handler().
  */
-void efi_net_set_dhcp_ack(void *pkt, int len)
+void efi_net_update_dhcp(int state, void *pkt, int len)
 {
 	int maxsize = sizeof(*dhcp_ack);
 
 	if (!dhcp_ack)
 		dhcp_ack = malloc(maxsize);
 
-	memcpy(dhcp_ack, pkt, min(len, maxsize));
+	if (state == REQUESTING)
+		memcpy(dhcp_ack, pkt, min(len, maxsize));
+
+	if (state == BOUND)
+		dhcp_ack_received = 1;
 }
 
 /**
@@ -645,8 +651,10 @@ efi_status_t efi_net_register(void)
 	netobj->net_mode.if_type = ARP_ETHER;
 
 	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 = dhcp_ack_received;
+	}
 
 	/*
 	 * Create WaitForPacket event.
diff --git a/net/bootp.c b/net/bootp.c
index 9a2b512e4a..987fc47d06 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -1067,11 +1067,11 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 			    strlen(CONFIG_SYS_BOOTFILE_PREFIX)) == 0) {
 #endif	/* CONFIG_SYS_BOOTFILE_PREFIX */
 			dhcp_packet_process_options(bp);
-			efi_net_set_dhcp_ack(pkt, len);
 
 			debug("TRANSITIONING TO REQUESTING STATE\n");
 			dhcp_state = REQUESTING;
 
+			efi_net_update_dhcp(dhcp_state, pkt, len);
 			net_set_timeout_handler(5000, bootp_timeout_handler);
 			dhcp_send_request_packet(bp);
 #ifdef CONFIG_SYS_BOOTFILE_PREFIX
@@ -1090,6 +1090,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 			dhcp_state = BOUND;
 			printf("DHCP client bound to address %pI4 (%lu ms)\n",
 			       &net_ip, get_timer(bootp_start));
+			efi_net_update_dhcp(dhcp_state, pkt, len);
 			net_set_timeout_handler(0, (thand_f *)0);
 			bootstage_mark_name(BOOTSTAGE_ID_BOOTP_STOP,
 					    "bootp_stop");

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

[Patrick Wildt]

On Wed, Apr 10, 2019 at 11:20:35AM +0200, Patrick Wildt wrote:
> On Thu, Jan 31, 2019 at 07:29:04PM +0100, Heinrich Schuchardt wrote:
> > Do we really need multiple functions to update the DHCP status?
> > 
> > I would prefer a single function with a parameter telling which DHCP
> > status has been reached.
> > 
> 
> I think this diff below might be better.  There we have an update
> function that is called after it switch the state, and on REQUESTING
> we save the packet information and on BOUND we received the ack.
> 
> > 
> > The current network device can be determined via eth_get_dev().
> > In function eth_current_changed() this eth_get_dev() function is used to
> > update the ethact environment variable.
> > 
> > If we have a single update function we can determine the active network
> > device there.
> 
> The efi network object is only created on bootefi, so there is no DHCP
> going on at that point.  It all happens some time before bootefi is
> called.  The only thing that we could do is try to memorize for which
> ethernet we received the DHCP packets, and when we create the EFI
> network object we can try to see if the DHCP packets match to the
> current ethernet we create the object for.
> 
> Patrick

Updated diff.  We should probably reset the DHCP Ack flag when we
switch to the REQUEST state.  Thus on a second dhcp call, where we
might get a different IP (on a different device) the ACK is properly
reset.

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 00b81c6010..7e8f3b04b5 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -290,8 +290,8 @@ efi_status_t efi_smbios_register(void);
 struct efi_simple_file_system_protocol *
 efi_fs_from_path(struct efi_device_path *fp);
 
-/* Called by networking code to memorize the dhcp ack package */
-void efi_net_set_dhcp_ack(void *pkt, int len);
+/* Called by networking code to memorize dhcp information */
+void efi_net_update_dhcp(int state, void *pkt, int len);
 /* Called by efi_set_watchdog_timer to reset the timer */
 efi_status_t efi_set_watchdog(unsigned long timeout);
 
@@ -578,7 +578,7 @@ static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
 static inline void efi_restore_gd(void) { }
 static inline void efi_set_bootdev(const char *dev, const char *devnr,
 				   const char *path) { }
-static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
+static inline void efi_net_update_dhcp(int state, void *pkt, int len) {}
 static inline void efi_print_image_infos(void *pc) { }
 
 #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index c7d9da8521..192e7f0bb7 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -8,6 +8,7 @@
 #include <common.h>
 #include <efi_loader.h>
 #include <malloc.h>
+#include "../net/bootp.h"
 
 static const efi_guid_t efi_net_guid = EFI_SIMPLE_NETWORK_GUID;
 static const efi_guid_t efi_pxe_guid = EFI_PXE_GUID;
@@ -15,6 +16,7 @@ static struct efi_pxe_packet *dhcp_ack;
 static bool new_rx_packet;
 static void *new_tx_packet;
 static void *transmit_buffer;
+static int dhcp_ack_received;
 
 /*
  * The notification function of this event is called in every timer cycle
@@ -522,18 +524,24 @@ out:
 }
 
 /**
- * efi_net_set_dhcp_ack() - take note of a selected DHCP IP address
+ * efi_net_update_dhcp() - take note of DHCP information
  *
  * This function is called by dhcp_handler().
  */
-void efi_net_set_dhcp_ack(void *pkt, int len)
+void efi_net_update_dhcp(int state, void *pkt, int len)
 {
 	int maxsize = sizeof(*dhcp_ack);
 
 	if (!dhcp_ack)
 		dhcp_ack = malloc(maxsize);
 
-	memcpy(dhcp_ack, pkt, min(len, maxsize));
+	if (state == REQUESTING) {
+		memcpy(dhcp_ack, pkt, min(len, maxsize));
+		dhcp_ack_received = 0;
+	}
+
+	if (state == BOUND)
+		dhcp_ack_received = 1;
 }
 
 /**
@@ -645,8 +653,10 @@ efi_status_t efi_net_register(void)
 	netobj->net_mode.if_type = ARP_ETHER;
 
 	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 = dhcp_ack_received;
+	}
 
 	/*
 	 * Create WaitForPacket event.
diff --git a/net/bootp.c b/net/bootp.c
index 9a2b512e4a..987fc47d06 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -1067,11 +1067,11 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 			    strlen(CONFIG_SYS_BOOTFILE_PREFIX)) == 0) {
 #endif	/* CONFIG_SYS_BOOTFILE_PREFIX */
 			dhcp_packet_process_options(bp);
-			efi_net_set_dhcp_ack(pkt, len);
 
 			debug("TRANSITIONING TO REQUESTING STATE\n");
 			dhcp_state = REQUESTING;
 
+			efi_net_update_dhcp(dhcp_state, pkt, len);
 			net_set_timeout_handler(5000, bootp_timeout_handler);
 			dhcp_send_request_packet(bp);
 #ifdef CONFIG_SYS_BOOTFILE_PREFIX
@@ -1090,6 +1090,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 			dhcp_state = BOUND;
 			printf("DHCP client bound to address %pI4 (%lu ms)\n",
 			       &net_ip, get_timer(bootp_start));
+			efi_net_update_dhcp(dhcp_state, pkt, len);
 			net_set_timeout_handler(0, (thand_f *)0);
 			bootstage_mark_name(BOOTSTAGE_ID_BOOTP_STOP,
 					    "bootp_stop");

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

[Patrick Wildt]

On Wed, Apr 10, 2019 at 11:24:53AM +0200, Patrick Wildt wrote:
> On Wed, Apr 10, 2019 at 11:20:35AM +0200, Patrick Wildt wrote:
> > On Thu, Jan 31, 2019 at 07:29:04PM +0100, Heinrich Schuchardt wrote:
> > > Do we really need multiple functions to update the DHCP status?
> > > 
> > > I would prefer a single function with a parameter telling which DHCP
> > > status has been reached.
> > > 
> > 
> > I think this diff below might be better.  There we have an update
> > function that is called after it switch the state, and on REQUESTING
> > we save the packet information and on BOUND we received the ack.
> > 
> > > 
> > > The current network device can be determined via eth_get_dev().
> > > In function eth_current_changed() this eth_get_dev() function is used to
> > > update the ethact environment variable.
> > > 
> > > If we have a single update function we can determine the active network
> > > device there.
> > 
> > The efi network object is only created on bootefi, so there is no DHCP
> > going on at that point.  It all happens some time before bootefi is
> > called.  The only thing that we could do is try to memorize for which
> > ethernet we received the DHCP packets, and when we create the EFI
> > network object we can try to see if the DHCP packets match to the
> > current ethernet we create the object for.
> > 
> > Patrick
> 
> Updated diff.  We should probably reset the DHCP Ack flag when we
> switch to the REQUEST state.  Thus on a second dhcp call, where we
> might get a different IP (on a different device) the ACK is properly
> reset.
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 00b81c6010..7e8f3b04b5 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -290,8 +290,8 @@ efi_status_t efi_smbios_register(void);
>  struct efi_simple_file_system_protocol *
>  efi_fs_from_path(struct efi_device_path *fp);
>  
> -/* Called by networking code to memorize the dhcp ack package */
> -void efi_net_set_dhcp_ack(void *pkt, int len);
> +/* Called by networking code to memorize dhcp information */
> +void efi_net_update_dhcp(int state, void *pkt, int len);
>  /* Called by efi_set_watchdog_timer to reset the timer */
>  efi_status_t efi_set_watchdog(unsigned long timeout);
>  
> @@ -578,7 +578,7 @@ static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
>  static inline void efi_restore_gd(void) { }
>  static inline void efi_set_bootdev(const char *dev, const char *devnr,
>  				   const char *path) { }
> -static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
> +static inline void efi_net_update_dhcp(int state, void *pkt, int len) {}
>  static inline void efi_print_image_infos(void *pc) { }
>  
>  #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> index c7d9da8521..192e7f0bb7 100644
> --- a/lib/efi_loader/efi_net.c
> +++ b/lib/efi_loader/efi_net.c
> @@ -8,6 +8,7 @@
>  #include <common.h>
>  #include <efi_loader.h>
>  #include <malloc.h>
> +#include "../net/bootp.h"
>  
>  static const efi_guid_t efi_net_guid = EFI_SIMPLE_NETWORK_GUID;
>  static const efi_guid_t efi_pxe_guid = EFI_PXE_GUID;
> @@ -15,6 +16,7 @@ static struct efi_pxe_packet *dhcp_ack;
>  static bool new_rx_packet;
>  static void *new_tx_packet;
>  static void *transmit_buffer;
> +static int dhcp_ack_received;
>  
>  /*
>   * The notification function of this event is called in every timer cycle
> @@ -522,18 +524,24 @@ out:
>  }
>  
>  /**
> - * efi_net_set_dhcp_ack() - take note of a selected DHCP IP address
> + * efi_net_update_dhcp() - take note of DHCP information
>   *
>   * This function is called by dhcp_handler().
>   */
> -void efi_net_set_dhcp_ack(void *pkt, int len)
> +void efi_net_update_dhcp(int state, void *pkt, int len)
>  {
>  	int maxsize = sizeof(*dhcp_ack);
>  
>  	if (!dhcp_ack)
>  		dhcp_ack = malloc(maxsize);
>  
> -	memcpy(dhcp_ack, pkt, min(len, maxsize));
> +	if (state == REQUESTING) {
> +		memcpy(dhcp_ack, pkt, min(len, maxsize));
> +		dhcp_ack_received = 0;
> +	}
> +
> +	if (state == BOUND)
> +		dhcp_ack_received = 1;
>  }
>  
>  /**
> @@ -645,8 +653,10 @@ efi_status_t efi_net_register(void)
>  	netobj->net_mode.if_type = ARP_ETHER;
>  
>  	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 = dhcp_ack_received;
> +	}
>  
>  	/*
>  	 * Create WaitForPacket event.
> diff --git a/net/bootp.c b/net/bootp.c
> index 9a2b512e4a..987fc47d06 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -1067,11 +1067,11 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  			    strlen(CONFIG_SYS_BOOTFILE_PREFIX)) == 0) {
>  #endif	/* CONFIG_SYS_BOOTFILE_PREFIX */
>  			dhcp_packet_process_options(bp);
> -			efi_net_set_dhcp_ack(pkt, len);
>  
>  			debug("TRANSITIONING TO REQUESTING STATE\n");
>  			dhcp_state = REQUESTING;
>  
> +			efi_net_update_dhcp(dhcp_state, pkt, len);
>  			net_set_timeout_handler(5000, bootp_timeout_handler);
>  			dhcp_send_request_packet(bp);
>  #ifdef CONFIG_SYS_BOOTFILE_PREFIX
> @@ -1090,6 +1090,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  			dhcp_state = BOUND;
>  			printf("DHCP client bound to address %pI4 (%lu ms)\n",
>  			       &net_ip, get_timer(bootp_start));
> +			efi_net_update_dhcp(dhcp_state, pkt, len);
>  			net_set_timeout_handler(0, (thand_f *)0);
>  			bootstage_mark_name(BOOTSTAGE_ID_BOOTP_STOP,
>  					    "bootp_stop");

Ping?

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

[Heinrich Schuchardt]

On 8/2/19 9:26 PM, Patrick Wildt wrote:
> On Wed, Apr 10, 2019 at 11:24:53AM +0200, Patrick Wildt wrote:
>> On Wed, Apr 10, 2019 at 11:20:35AM +0200, Patrick Wildt wrote:
>>> On Thu, Jan 31, 2019 at 07:29:04PM +0100, Heinrich Schuchardt wrote:
>>>> Do we really need multiple functions to update the DHCP status?
>>>>
>>>> I would prefer a single function with a parameter telling which DHCP
>>>> status has been reached.
>>>>
>>>
>>> I think this diff below might be better.  There we have an update
>>> function that is called after it switch the state, and on REQUESTING
>>> we save the packet information and on BOUND we received the ack.
>>>
>>>>
>>>> The current network device can be determined via eth_get_dev().
>>>> In function eth_current_changed() this eth_get_dev() function is used to
>>>> update the ethact environment variable.
>>>>
>>>> If we have a single update function we can determine the active network
>>>> device there.
>>>
>>> The efi network object is only created on bootefi, so there is no DHCP
>>> going on at that point.  It all happens some time before bootefi is
>>> called.  The only thing that we could do is try to memorize for which
>>> ethernet we received the DHCP packets, and when we create the EFI
>>> network object we can try to see if the DHCP packets match to the
>>> current ethernet we create the object for.
>>>
>>> Patrick
>>
>> Updated diff.  We should probably reset the DHCP Ack flag when we
>> switch to the REQUEST state.  Thus on a second dhcp call, where we
>> might get a different IP (on a different device) the ACK is properly
>> reset.
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 00b81c6010..7e8f3b04b5 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -290,8 +290,8 @@ efi_status_t efi_smbios_register(void);
>>   struct efi_simple_file_system_protocol *
>>   efi_fs_from_path(struct efi_device_path *fp);
>>
>> -/* Called by networking code to memorize the dhcp ack package */
>> -void efi_net_set_dhcp_ack(void *pkt, int len);
>> +/* Called by networking code to memorize dhcp information */
>> +void efi_net_update_dhcp(int state, void *pkt, int len);
>>   /* Called by efi_set_watchdog_timer to reset the timer */
>>   efi_status_t efi_set_watchdog(unsigned long timeout);
>>
>> @@ -578,7 +578,7 @@ static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
>>   static inline void efi_restore_gd(void) { }
>>   static inline void efi_set_bootdev(const char *dev, const char *devnr,
>>   				   const char *path) { }
>> -static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
>> +static inline void efi_net_update_dhcp(int state, void *pkt, int len) {}
>>   static inline void efi_print_image_infos(void *pc) { }
>>
>>   #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
>> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
>> index c7d9da8521..192e7f0bb7 100644
>> --- a/lib/efi_loader/efi_net.c
>> +++ b/lib/efi_loader/efi_net.c
>> @@ -8,6 +8,7 @@
>>   #include <common.h>
>>   #include <efi_loader.h>
>>   #include <malloc.h>
>> +#include "../net/bootp.h"
>>
>>   static const efi_guid_t efi_net_guid = EFI_SIMPLE_NETWORK_GUID;
>>   static const efi_guid_t efi_pxe_guid = EFI_PXE_GUID;
>> @@ -15,6 +16,7 @@ static struct efi_pxe_packet *dhcp_ack;
>>   static bool new_rx_packet;
>>   static void *new_tx_packet;
>>   static void *transmit_buffer;
>> +static int dhcp_ack_received;
>>
>>   /*
>>    * The notification function of this event is called in every timer cycle
>> @@ -522,18 +524,24 @@ out:
>>   }
>>
>>   /**
>> - * efi_net_set_dhcp_ack() - take note of a selected DHCP IP address
>> + * efi_net_update_dhcp() - take note of DHCP information
>>    *
>>    * This function is called by dhcp_handler().
>>    */
>> -void efi_net_set_dhcp_ack(void *pkt, int len)
>> +void efi_net_update_dhcp(int state, void *pkt, int len)
>>   {
>>   	int maxsize = sizeof(*dhcp_ack);
>>
>>   	if (!dhcp_ack)
>>   		dhcp_ack = malloc(maxsize);
>>
>> -	memcpy(dhcp_ack, pkt, min(len, maxsize));
>> +	if (state == REQUESTING) {
>> +		memcpy(dhcp_ack, pkt, min(len, maxsize));
>> +		dhcp_ack_received = 0;
>> +	}
>> +
>> +	if (state == BOUND)
>> +		dhcp_ack_received = 1;
>>   }
>>
>>   /**
>> @@ -645,8 +653,10 @@ efi_status_t efi_net_register(void)
>>   	netobj->net_mode.if_type = ARP_ETHER;
>>
>>   	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 = dhcp_ack_received;
>> +	}
>>
>>   	/*
>>   	 * Create WaitForPacket event.
>> diff --git a/net/bootp.c b/net/bootp.c
>> index 9a2b512e4a..987fc47d06 100644
>> --- a/net/bootp.c
>> +++ b/net/bootp.c
>> @@ -1067,11 +1067,11 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>>   			    strlen(CONFIG_SYS_BOOTFILE_PREFIX)) == 0) {
>>   #endif	/* CONFIG_SYS_BOOTFILE_PREFIX */
>>   			dhcp_packet_process_options(bp);
>> -			efi_net_set_dhcp_ack(pkt, len);
>>
>>   			debug("TRANSITIONING TO REQUESTING STATE\n");
>>   			dhcp_state = REQUESTING;
>>
>> +			efi_net_update_dhcp(dhcp_state, pkt, len);
>>   			net_set_timeout_handler(5000, bootp_timeout_handler);
>>   			dhcp_send_request_packet(bp);
>>   #ifdef CONFIG_SYS_BOOTFILE_PREFIX
>> @@ -1090,6 +1090,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>>   			dhcp_state = BOUND;
>>   			printf("DHCP client bound to address %pI4 (%lu ms)\n",
>>   			       &net_ip, get_timer(bootp_start));
>> +			efi_net_update_dhcp(dhcp_state, pkt, len);
>>   			net_set_timeout_handler(0, (thand_f *)0);
>>   			bootstage_mark_name(BOOTSTAGE_ID_BOOTP_STOP,
>>   					    "bootp_stop");
>
> Ping?
>

The network address can be set in U-Boot by the 'dhcp' or via 'setaddr
ipaddr'.

U-Boot supports multiple network interfaces. The UEFI sub-system uses
the one that is active when one of the following commands is invoked:

efidebug, env -e, printenv -e, bootefi.

The tftp or dhcp can be invoked before or after any of these.

UEFI payloads may implement a DHCP command themselves.

So you could have:

u-boot> setenv ethact eth0
u-boot> printenv -e
u-boot> setenv ethact eth1
u-boot> dhcp
u-boot> setenv ipaddr 192.168.0.4
u-boot> load mmc 0:1 $fdt_addr_r dtb
u-boot> load mmc 0:1 $kernel_addr_r snp.efi
u-boot> bootefi $kernel_addr_r $fdt_addr
ipxe> dhcp

What do you expect to happen in each of these commands?

With your patch the UEFI sub-system would use eth0 but update the UEFI
network device with the ipaddress assigned by U-Boot's dhcp command for
eth1. That cannot be right.

Best regards

Heinrich

[U-Boot] EFI_PXE_BASE_CODE_PROTOCOL

[Heinrich Schuchardt]

Hello Alex,

lib/efi_loader/efi_net.c contains pieces of the
EFI_PXE_BASE_CODE_PROTOCOL. But it is incompletely implemented: all
function pointers are NULL and so immediate failure is expected when
using the protocol.

Do you remember why you introduced this protocol into U-Boot?
It is not part of the EBBR specification.

It is not needed for booting via GRUB from disk but seems to be used to
configure the network device in GRUB (grub_net_configure_by_dhcp_ack()
seems only to consume pxe_mode->dhcp_ack).

If the UEFI subsystem is initialized before using the 'dhcp' command the
DHCP results are ignored.

@Patrick:
What do you use the protocol for? GRUB?

Best regards

Heinrich

[U-Boot] EFI_PXE_BASE_CODE_PROTOCOL

[Heinrich Schuchardt]

Hello Leif,

iPXE uses the EFI simple network protocol to execute DHCP.

Can GRUB already do the same when the EFI_PXE_BASE_CODE_PROTOCOL is not
present?

What I do not understand about GRUB's grub_net_configure_by_dhcp_ack()
is that it silently assumes IPv4 being used without even checking. This
contradicts the definition of the PXE base code protocol in the UEFI
standard:

EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:

typedef union {
  UINT8                             Raw[1472];
  EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
  EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
} EFI_PXE_BASE_CODE_PACKET;

Should the check be done in grub_efi_net_config_real()?

Best regards

Heinrich

On 8/5/19 10:08 PM, Heinrich Schuchardt wrote:
> Hello Alex,
>
> lib/efi_loader/efi_net.c contains pieces of the
> EFI_PXE_BASE_CODE_PROTOCOL. But it is incompletely implemented: all
> function pointers are NULL and so immediate failure is expected when
> using the protocol.
>
> Do you remember why you introduced this protocol into U-Boot?
> It is not part of the EBBR specification.
>
> It is not needed for booting via GRUB from disk but seems to be used to
> configure the network device in GRUB (grub_net_configure_by_dhcp_ack()
> seems only to consume pxe_mode->dhcp_ack).
>
> If the UEFI subsystem is initialized before using the 'dhcp' command the
> DHCP results are ignored.
>
> @Patrick:
> What do you use the protocol for? GRUB?
>
> Best regards
>
> Heinrich
>

[U-Boot] EFI_PXE_BASE_CODE_PROTOCOL

[Leif Lindholm]

+Peter Jones (sorry Peter)

On Tue, Aug 06, 2019 at 08:34:58AM +0200, Heinrich Schuchardt wrote:
> iPXE uses the EFI simple network protocol to execute DHCP.

OK.

> Can GRUB already do the same when the EFI_PXE_BASE_CODE_PROTOCOL is not
> present?

Yes. As of very recently (proper* DHCP support was only merged in
March 2019, so is included in 2.04 release, prior to that it
technically performed BOOTP).

SNP means you do your own networking - it gives you access to the raw
(usually) Ethernet packets.

* proper as in "it now conceptually does the correct thing", not as in
  "I have extensively tested this".

> What I do not understand about GRUB's grub_net_configure_by_dhcp_ack()
> is that it silently assumes IPv4 being used without even checking. This
> contradicts the definition of the PXE base code protocol in the UEFI
> standard:

Well, it would not surprise me if this function predates GRUB's UEFI
support.

It actually gets even slightly messier when you look at what GRUB does
when netbooting itself; it starts out using MNP (and hence IP
addresses assigned by UEFI) to load its modules, switching to SNP once
it loads efinet.mod.

> EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:
> 
> typedef union {
>  UINT8                             Raw[1472];
>  EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
>  EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
> } EFI_PXE_BASE_CODE_PACKET;
> 
> Should the check be done in grub_efi_net_config_real()?

Possibly. I've cc:d Peter since he's the last person I know who took a
proper look at this.

Certainly, it would be useful if you could raise a bug on Savannah on
the ipv4 assumption.

Best Regards,

Leif

[U-Boot] EFI_PXE_BASE_CODE_PROTOCOL

[Peter Jones]

On Tue, Aug 06, 2019 at 09:44:14AM +0100, Leif Lindholm wrote:
> +Peter Jones (sorry Peter)

+ Javier Martinez Canillas

> On Tue, Aug 06, 2019 at 08:34:58AM +0200, Heinrich Schuchardt wrote:
> > iPXE uses the EFI simple network protocol to execute DHCP.
> 
> OK.
> 
> > Can GRUB already do the same when the EFI_PXE_BASE_CODE_PROTOCOL is not
> > present?
> 
> Yes. As of very recently (proper* DHCP support was only merged in
> March 2019, so is included in 2.04 release, prior to that it
> technically performed BOOTP).
> 
> SNP means you do your own networking - it gives you access to the raw
> (usually) Ethernet packets.
> 
> * proper as in "it now conceptually does the correct thing", not as in
>   "I have extensively tested this".
> 
> > What I do not understand about GRUB's grub_net_configure_by_dhcp_ack()
> > is that it silently assumes IPv4 being used without even checking. This
> > contradicts the definition of the PXE base code protocol in the UEFI
> > standard:
> 
> Well, it would not surprise me if this function predates GRUB's UEFI
> support.
> 
> It actually gets even slightly messier when you look at what GRUB does
> when netbooting itself; it starts out using MNP (and hence IP
> addresses assigned by UEFI) to load its modules, switching to SNP once
> it loads efinet.mod.
> 
> > EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:
> > 
> > typedef union {
> >  UINT8                             Raw[1472];
> >  EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
> >  EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
> > } EFI_PXE_BASE_CODE_PACKET;
> > 
> > Should the check be done in grub_efi_net_config_real()?
> 
> Possibly. I've cc:d Peter since he's the last person I know who took a
> proper look at this.

That's actually what we've got in our current patch set, based on
Michael Chang at SuSE's https work.  Javier Martinez (cc'd) is working
on getting those ready for upstream, but in the mean time, check out the
patches nearby to:

https://github.com/rhboot/grub2/commit/4f977935a9271727bf21889526fdf82f0fc8fed9

-- 
        Peter

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

[Heinrich Schuchardt]

On 8/4/19 1:52 PM, Heinrich Schuchardt wrote:
> On 8/2/19 9:26 PM, Patrick Wildt wrote:
>> On Wed, Apr 10, 2019 at 11:24:53AM +0200, Patrick Wildt wrote:
>>> On Wed, Apr 10, 2019 at 11:20:35AM +0200, Patrick Wildt wrote:
>>>> On Thu, Jan 31, 2019 at 07:29:04PM +0100, Heinrich Schuchardt wrote:
>>>>> Do we really need multiple functions to update the DHCP status?
>>>>>
>>>>> I would prefer a single function with a parameter telling which DHCP
>>>>> status has been reached.
>>>>>
>>>>
>>>> I think this diff below might be better.  There we have an update
>>>> function that is called after it switch the state, and on REQUESTING
>>>> we save the packet information and on BOUND we received the ack.
>>>>
>>>>>
>>>>> The current network device can be determined via eth_get_dev().
>>>>> In function eth_current_changed() this eth_get_dev() function is
>>>>> used to
>>>>> update the ethact environment variable.
>>>>>
>>>>> If we have a single update function we can determine the active
>>>>> network
>>>>> device there.
>>>>
>>>> The efi network object is only created on bootefi, so there is no DHCP
>>>> going on at that point.  It all happens some time before bootefi is
>>>> called.  The only thing that we could do is try to memorize for which
>>>> ethernet we received the DHCP packets, and when we create the EFI
>>>> network object we can try to see if the DHCP packets match to the
>>>> current ethernet we create the object for.
>>>>
>>>> Patrick
>>>
>>> Updated diff.  We should probably reset the DHCP Ack flag when we
>>> switch to the REQUEST state.  Thus on a second dhcp call, where we
>>> might get a different IP (on a different device) the ACK is properly
>>> reset.
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 00b81c6010..7e8f3b04b5 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -290,8 +290,8 @@ efi_status_t efi_smbios_register(void);
>>>   struct efi_simple_file_system_protocol *
>>>   efi_fs_from_path(struct efi_device_path *fp);
>>>
>>> -/* Called by networking code to memorize the dhcp ack package */
>>> -void efi_net_set_dhcp_ack(void *pkt, int len);
>>> +/* Called by networking code to memorize dhcp information */
>>> +void efi_net_update_dhcp(int state, void *pkt, int len);
>>>   /* Called by efi_set_watchdog_timer to reset the timer */
>>>   efi_status_t efi_set_watchdog(unsigned long timeout);
>>>
>>> @@ -578,7 +578,7 @@ static inline efi_status_t
>>> efi_add_runtime_mmio(void *mmio_ptr, u64 len)
>>>   static inline void efi_restore_gd(void) { }
>>>   static inline void efi_set_bootdev(const char *dev, const char *devnr,
>>>                      const char *path) { }
>>> -static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
>>> +static inline void efi_net_update_dhcp(int state, void *pkt, int
>>> len) {}
>>>   static inline void efi_print_image_infos(void *pc) { }
>>>
>>>   #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
>>> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
>>> index c7d9da8521..192e7f0bb7 100644
>>> --- a/lib/efi_loader/efi_net.c
>>> +++ b/lib/efi_loader/efi_net.c
>>> @@ -8,6 +8,7 @@
>>>   #include <common.h>
>>>   #include <efi_loader.h>
>>>   #include <malloc.h>
>>> +#include "../net/bootp.h"
>>>
>>>   static const efi_guid_t efi_net_guid = EFI_SIMPLE_NETWORK_GUID;
>>>   static const efi_guid_t efi_pxe_guid = EFI_PXE_GUID;
>>> @@ -15,6 +16,7 @@ static struct efi_pxe_packet *dhcp_ack;
>>>   static bool new_rx_packet;
>>>   static void *new_tx_packet;
>>>   static void *transmit_buffer;
>>> +static int dhcp_ack_received;
>>>
>>>   /*
>>>    * The notification function of this event is called in every timer
>>> cycle
>>> @@ -522,18 +524,24 @@ out:
>>>   }
>>>
>>>   /**
>>> - * efi_net_set_dhcp_ack() - take note of a selected DHCP IP address
>>> + * efi_net_update_dhcp() - take note of DHCP information
>>>    *
>>>    * This function is called by dhcp_handler().
>>>    */
>>> -void efi_net_set_dhcp_ack(void *pkt, int len)
>>> +void efi_net_update_dhcp(int state, void *pkt, int len)
>>>   {
>>>       int maxsize = sizeof(*dhcp_ack);
>>>
>>>       if (!dhcp_ack)
>>>           dhcp_ack = malloc(maxsize);
>>>
>>> -    memcpy(dhcp_ack, pkt, min(len, maxsize));
>>> +    if (state == REQUESTING) {
>>> +        memcpy(dhcp_ack, pkt, min(len, maxsize));
>>> +        dhcp_ack_received = 0;
>>> +    }
>>> +
>>> +    if (state == BOUND)
>>> +        dhcp_ack_received = 1;
>>>   }
>>>
>>>   /**
>>> @@ -645,8 +653,10 @@ efi_status_t efi_net_register(void)
>>>       netobj->net_mode.if_type = ARP_ETHER;
>>>
>>>       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 = dhcp_ack_received;
>>> +    }
>>>
>>>       /*
>>>        * Create WaitForPacket event.
>>> diff --git a/net/bootp.c b/net/bootp.c
>>> index 9a2b512e4a..987fc47d06 100644
>>> --- a/net/bootp.c
>>> +++ b/net/bootp.c
>>> @@ -1067,11 +1067,11 @@ static void dhcp_handler(uchar *pkt, unsigned
>>> dest, struct in_addr sip,
>>>                   strlen(CONFIG_SYS_BOOTFILE_PREFIX)) == 0) {
>>>   #endif    /* CONFIG_SYS_BOOTFILE_PREFIX */
>>>               dhcp_packet_process_options(bp);
>>> -            efi_net_set_dhcp_ack(pkt, len);
>>>
>>>               debug("TRANSITIONING TO REQUESTING STATE\n");
>>>               dhcp_state = REQUESTING;
>>>
>>> +            efi_net_update_dhcp(dhcp_state, pkt, len);
>>>               net_set_timeout_handler(5000, bootp_timeout_handler);
>>>               dhcp_send_request_packet(bp);
>>>   #ifdef CONFIG_SYS_BOOTFILE_PREFIX
>>> @@ -1090,6 +1090,7 @@ static void dhcp_handler(uchar *pkt, unsigned
>>> dest, struct in_addr sip,
>>>               dhcp_state = BOUND;
>>>               printf("DHCP client bound to address %pI4 (%lu ms)\n",
>>>                      &net_ip, get_timer(bootp_start));
>>> +            efi_net_update_dhcp(dhcp_state, pkt, len);
>>>               net_set_timeout_handler(0, (thand_f *)0);
>>>               bootstage_mark_name(BOOTSTAGE_ID_BOOTP_STOP,
>>>                           "bootp_stop");
>>
>> Ping?
>>
>
> The network address can be set in U-Boot by the 'dhcp' or via 'setaddr
> ipaddr'.
>
> U-Boot supports multiple network interfaces. The UEFI sub-system uses
> the one that is active when one of the following commands is invoked:
>
> efidebug, env -e, printenv -e, bootefi.
>
> The tftp or dhcp can be invoked before or after any of these.
>
> UEFI payloads may implement a DHCP command themselves.
>
> So you could have:
>
> u-boot> setenv ethact eth0
> u-boot> printenv -e
> u-boot> setenv ethact eth1
> u-boot> dhcp
> u-boot> setenv ipaddr 192.168.0.4
> u-boot> load mmc 0:1 $fdt_addr_r dtb
> u-boot> load mmc 0:1 $kernel_addr_r snp.efi
> u-boot> bootefi $kernel_addr_r $fdt_addr
> ipxe> dhcp
>
> What do you expect to happen in each of these commands?
>
> With your patch the UEFI sub-system would use eth0 but update the UEFI
> network device with the ipaddress assigned by U-Boot's dhcp command for
> eth1. That cannot be right.
>
> Best regards
>
> Heinrich

The UEFI compliant way to set dhcp_ack is calling
EFI_PXE_BASE_CODE_PROTOCOL.SetPackets() which we yet have to implement.

I am currently preparing a patch to replace the NULL pointers in the
EFI_PXE_BASE_CODE_PROTOCOL by empty functions return EFI_UNSUPPORTED.

Best regards

Heinrich

[U-Boot] EFI_PXE_BASE_CODE_PROTOCOL

[Heinrich Schuchardt]

On 8/6/19 7:02 PM, Peter Jones wrote:
> On Tue, Aug 06, 2019 at 09:44:14AM +0100, Leif Lindholm wrote:
>> +Peter Jones (sorry Peter)
>
> + Javier Martinez Canillas
>
>> On Tue, Aug 06, 2019 at 08:34:58AM +0200, Heinrich Schuchardt wrote:
>>> iPXE uses the EFI simple network protocol to execute DHCP.
>>
>> OK.
>>
>>> Can GRUB already do the same when the EFI_PXE_BASE_CODE_PROTOCOL is not
>>> present?
>>
>> Yes. As of very recently (proper* DHCP support was only merged in
>> March 2019, so is included in 2.04 release, prior to that it
>> technically performed BOOTP).
>>
>> SNP means you do your own networking - it gives you access to the raw
>> (usually) Ethernet packets.
>>
>> * proper as in "it now conceptually does the correct thing", not as in
>>    "I have extensively tested this".
>>
>>> What I do not understand about GRUB's grub_net_configure_by_dhcp_ack()
>>> is that it silently assumes IPv4 being used without even checking. This
>>> contradicts the definition of the PXE base code protocol in the UEFI
>>> standard:
>>
>> Well, it would not surprise me if this function predates GRUB's UEFI
>> support.
>>
>> It actually gets even slightly messier when you look at what GRUB does
>> when netbooting itself; it starts out using MNP (and hence IP
>> addresses assigned by UEFI) to load its modules, switching to SNP once
>> it loads efinet.mod.
>>
>>> EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:
>>>
>>> typedef union {
>>>   UINT8                             Raw[1472];
>>>   EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
>>>   EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
>>> } EFI_PXE_BASE_CODE_PACKET;
>>>
>>> Should the check be done in grub_efi_net_config_real()?
>>
>> Possibly. I've cc:d Peter since he's the last person I know who took a
>> proper look at this.
>
> That's actually what we've got in our current patch set, based on
> Michael Chang at SuSE's https work.  Javier Martinez (cc'd) is working
> on getting those ready for upstream, but in the mean time, check out the
> patches nearby to:
>
> https://github.com/rhboot/grub2/commit/4f977935a9271727bf21889526fdf82f0fc8fed9
>

Thank you for the link.

There are currently no plans to implement these device paths in U-Boot:

* IPv4 Device Path
* IPv6 Device Path
* Uniform Resource Identifiers (URI) Device Path

I assume that these device paths would only be installed on handles
implementing the EFI DHCPv4 Protocol or the EFI DHCPv6 Protocol but
could not identify the relevant information in the specification.

Best regards

Heinrich

[U-Boot] EFI_PXE_BASE_CODE_PROTOCOL

[Leif Lindholm]

On Tue, Aug 06, 2019 at 08:52:09PM +0200, Heinrich Schuchardt wrote:
> > > > EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:
> > > > 
> > > > typedef union {
> > > >   UINT8                             Raw[1472];
> > > >   EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
> > > >   EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
> > > > } EFI_PXE_BASE_CODE_PACKET;
> > > > 
> > > > Should the check be done in grub_efi_net_config_real()?
> > > 
> > > Possibly. I've cc:d Peter since he's the last person I know who took a
> > > proper look at this.
> > 
> > That's actually what we've got in our current patch set, based on
> > Michael Chang at SuSE's https work.  Javier Martinez (cc'd) is working
> > on getting those ready for upstream, but in the mean time, check out the
> > patches nearby to:
> > 
> > https://github.com/rhboot/grub2/commit/4f977935a9271727bf21889526fdf82f0fc8fed9
> 
> Thank you for the link.
> 
> There are currently no plans to implement these device paths in U-Boot:
> 
> * IPv4 Device Path
> * IPv6 Device Path
> * Uniform Resource Identifiers (URI) Device Path
> 
> I assume that these device paths would only be installed on handles
> implementing the EFI DHCPv4 Protocol or the EFI DHCPv6 Protocol but
> could not identify the relevant information in the specification.

10.3.4.12/13 (UEFI 2.8 spec) for IPv4/IPv6.
10.3.4.23 for URI.
(And 10.3.4.21 for iSCSI.)

But if you are only asking because you found the (NULLed-out) PXE
protocol implemented, then I would suggest we can ignore this for now.

I guess it could be useful for netbooting GRUB (the device paths, and
passing DHCP information retrieved by U-Boot into GRUB), not the
EFI_PXE_BASE_CODE_PROTOCOL).

/
    Leif