[U-Boot] [PATCH 2/2] efi_loader: set the dhcp ack received flag
The PXE object contains a flag that specifies whether or not a DHCP ACK has been received. This 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.
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
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
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));
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));
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));
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));
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
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
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");
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");
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?
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);@@ -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
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
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
+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
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/4f977935a9271727bf21889526fdf82f0fc8f...
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/4f977935a9271727bf21889526fdf82f0fc8f...
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
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/4f977935a9271727bf21889526fdf82f0fc8f...
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
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
participants (5)
-
Alexander Graf -
Heinrich Schuchardt -
Leif Lindholm -
Patrick Wildt -
Peter Jones