[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

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

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

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

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

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

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