[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