[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