[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

[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] [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] [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

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

[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:
> 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 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 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

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

[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

[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)