[U-Boot] [PATCH v2 00/17] efi_loader: Simple Network Protocol

This patch series focuses on * correction of parameter types for boot services * fixes for the SetMem and CopyMem boot services * fixes for the simple network protocol implementation (SNP) * a unit test for SetMem, CopyMem and the simple network protocol
The unit test broadcasts a DHCPDISCOVER messager over the network and receives the reply.
This patch series is based on * efi-next tree https://github.com/agraf/u-boot/tree/efi-next * [PATCH 1/1] efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST https://patchwork.ozlabs.org/patch/816412/ This patch enables the unit test on qemu-x86_defconfig * [PATCH 1/1] efi_loader: provide function comments for boot services https://patchwork.ozlabs.org/patch/817010/
--- v2 Move efi_st_memcmp to a new module. We can reuse it in future tests. Use constants as return values. Reflect renaming of signaled to is_signaled for events. --- Heinrich Schuchardt (17): efi_loader: call EFI_EXIT in efi_copy_mem, efi_set_mem efi_loader: parameters of CopyMem and SetMem efi_loader: pass GUIDs as const efi_guid_t * efi_loader: wrong type in wait_for_event efi_loader: incorrect definition of EFI_SIMPLE_NETWORK_PROTOCOL efi_loader: correct bits of receive_filters bit mask efi_loader: fill simple network protocol revision efi_loader: efi_net: hwaddr_size = 6 efi_net: return EFI_UNSUPPORTED where appropriate efi_loader: use events for efi_net_receive efi_loader: implement WaitForPacket event efi_loader: fix efi_net_get_status efi_loader: size fields in SimpleNetworkProtocol efi_loader: fill return values in SimpleNetworkProtocol efi_selftest: correct definition of efi_st_error efi_selftest: allow printing MAC addresses efi_loader: supply EFI network test
include/efi_api.h | 62 +++-- include/efi_selftest.h | 15 +- lib/efi_loader/efi_boottime.c | 54 ++-- lib/efi_loader/efi_net.c | 144 +++++++++-- lib/efi_selftest/Makefile | 8 +- lib/efi_selftest/efi_selftest_console.c | 41 ++- lib/efi_selftest/efi_selftest_events.c | 2 +- lib/efi_selftest/efi_selftest_snp.c | 424 ++++++++++++++++++++++++++++++++ lib/efi_selftest/efi_selftest_tpl.c | 2 +- lib/efi_selftest/efi_selftest_util.c | 25 ++ 10 files changed, 702 insertions(+), 75 deletions(-) create mode 100644 lib/efi_selftest/efi_selftest_snp.c create mode 100644 lib/efi_selftest/efi_selftest_util.c

EFI_ENTRY and EFI_EXIT calls must match.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- lib/efi_loader/efi_boottime.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 66ce92f654..b8b98f2c4a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1868,6 +1868,7 @@ static void EFIAPI efi_copy_mem(void *destination, void *source, { EFI_ENTRY("%p, %p, %ld", destination, source, length); memcpy(destination, source, length); + EFI_EXIT(EFI_SUCCESS); }
/* @@ -1885,6 +1886,7 @@ static void EFIAPI efi_set_mem(void *buffer, unsigned long size, uint8_t value) { EFI_ENTRY("%p, %ld, 0x%x", buffer, size, value); memset(buffer, value, size); + EFI_EXIT(EFI_SUCCESS); }
/*

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
EFI_ENTRY and EFI_EXIT calls must match.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
lib/efi_loader/efi_boottime.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org

The UEFI spec defines the length parameters of CopyMem and SetMem as UINTN. We should size_t here.
The source buffer of CopyMem should be marked as const.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_api.h | 7 +++---- lib/efi_loader/efi_boottime.c | 10 +++++----- 2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index c3b9032a48..0b1a383e61 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -156,10 +156,9 @@ struct efi_boot_services { void *handle, ...); efi_status_t (EFIAPI *calculate_crc32)(void *data, unsigned long data_size, uint32_t *crc32); - void (EFIAPI *copy_mem)(void *destination, void *source, - unsigned long length); - void (EFIAPI *set_mem)(void *buffer, unsigned long size, - uint8_t value); + void (EFIAPI *copy_mem)(void *destination, const void *source, + size_t length); + void (EFIAPI *set_mem)(void *buffer, size_t size, uint8_t value); void *create_event_ex; };
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index b8b98f2c4a..c48ff2cd2a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1863,10 +1863,10 @@ static efi_status_t EFIAPI efi_calculate_crc32(void *data, * @source source of the copy operation * @length number of bytes to copy */ -static void EFIAPI efi_copy_mem(void *destination, void *source, - unsigned long length) +static void EFIAPI efi_copy_mem(void *destination, const void *source, + size_t length) { - EFI_ENTRY("%p, %p, %ld", destination, source, length); + EFI_ENTRY("%p, %p, %ld", destination, source, (unsigned long)length); memcpy(destination, source, length); EFI_EXIT(EFI_SUCCESS); } @@ -1882,9 +1882,9 @@ static void EFIAPI efi_copy_mem(void *destination, void *source, * @size size of buffer in bytes * @value byte to copy to the buffer */ -static void EFIAPI efi_set_mem(void *buffer, unsigned long size, uint8_t value) +static void EFIAPI efi_set_mem(void *buffer, size_t size, uint8_t value) { - EFI_ENTRY("%p, %ld, 0x%x", buffer, size, value); + EFI_ENTRY("%p, %ld, 0x%x", buffer, (unsigned long)size, value); memset(buffer, value, size); EFI_EXIT(EFI_SUCCESS); }

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
The UEFI spec defines the length parameters of CopyMem and SetMem as UINTN. We should size_t here.
The source buffer of CopyMem should be marked as const.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_api.h | 7 +++---- lib/efi_loader/efi_boottime.c | 10 +++++----- 2 files changed, 8 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

We need to call some boottime services internally. Our GUIDs are stored as const efi_guid_t *.
The boottime services never change GUIDs. So we can define the parameters as const efi_guid_t *.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_api.h | 27 ++++++++++++++------------- lib/efi_loader/efi_boottime.c | 40 +++++++++++++++++++++------------------- 2 files changed, 35 insertions(+), 32 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 0b1a383e61..aa4306aac9 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -77,24 +77,25 @@ struct efi_boot_services { efi_status_t (EFIAPI *check_event)(struct efi_event *event); #define EFI_NATIVE_INTERFACE 0x00000000 efi_status_t (EFIAPI *install_protocol_interface)( - void **handle, efi_guid_t *protocol, + void **handle, const efi_guid_t *protocol, int protocol_interface_type, void *protocol_interface); efi_status_t (EFIAPI *reinstall_protocol_interface)( - void *handle, efi_guid_t *protocol, + void *handle, const efi_guid_t *protocol, void *old_interface, void *new_interface); efi_status_t (EFIAPI *uninstall_protocol_interface)(void *handle, - efi_guid_t *protocol, void *protocol_interface); - efi_status_t (EFIAPI *handle_protocol)(efi_handle_t, efi_guid_t *, - void **); + const efi_guid_t *protocol, void *protocol_interface); + efi_status_t (EFIAPI *handle_protocol)(efi_handle_t, + const efi_guid_t *protocol, + void **protocol_interface); void *reserved; efi_status_t (EFIAPI *register_protocol_notify)( - efi_guid_t *protocol, struct efi_event *event, + const efi_guid_t *protocol, struct efi_event *event, void **registration); efi_status_t (EFIAPI *locate_handle)( enum efi_locate_search_type search_type, - efi_guid_t *protocol, void *search_key, + const efi_guid_t *protocol, void *search_key, unsigned long *buffer_size, efi_handle_t *buffer); - efi_status_t (EFIAPI *locate_device_path)(efi_guid_t *protocol, + efi_status_t (EFIAPI *locate_device_path)(const efi_guid_t *protocol, struct efi_device_path **device_path, efi_handle_t *device); efi_status_t (EFIAPI *install_configuration_table)( @@ -131,14 +132,14 @@ struct efi_boot_services { #define EFI_OPEN_PROTOCOL_BY_DRIVER 0x00000010 #define EFI_OPEN_PROTOCOL_EXCLUSIVE 0x00000020 efi_status_t (EFIAPI *open_protocol)(efi_handle_t handle, - efi_guid_t *protocol, void **interface, + const efi_guid_t *protocol, void **interface, efi_handle_t agent_handle, efi_handle_t controller_handle, u32 attributes); efi_status_t (EFIAPI *close_protocol)(void *handle, - efi_guid_t *protocol, void *agent_handle, + const efi_guid_t *protocol, void *agent_handle, void *controller_handle); efi_status_t(EFIAPI *open_protocol_information)(efi_handle_t handle, - efi_guid_t *protocol, + const efi_guid_t *protocol, struct efi_open_protocol_info_entry **entry_buffer, unsigned long *entry_count); efi_status_t (EFIAPI *protocols_per_handle)(efi_handle_t handle, @@ -146,9 +147,9 @@ struct efi_boot_services { unsigned long *protocols_buffer_count); efi_status_t (EFIAPI *locate_handle_buffer) ( enum efi_locate_search_type search_type, - efi_guid_t *protocol, void *search_key, + const efi_guid_t *protocol, void *search_key, unsigned long *no_handles, efi_handle_t **buffer); - efi_status_t (EFIAPI *locate_protocol)(efi_guid_t *protocol, + efi_status_t (EFIAPI *locate_protocol)(const efi_guid_t *protocol, void *registration, void **protocol_interface); efi_status_t (EFIAPI *install_multiple_protocol_interfaces)( void **handle, ...); diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index c48ff2cd2a..e5adc17fab 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -704,7 +704,7 @@ static efi_status_t EFIAPI efi_check_event(struct efi_event *event) * @return status code */ static efi_status_t EFIAPI efi_install_protocol_interface(void **handle, - efi_guid_t *protocol, int protocol_interface_type, + const efi_guid_t *protocol, int protocol_interface_type, void *protocol_interface) { struct list_head *lhandle; @@ -776,7 +776,7 @@ out: * @return status code */ static efi_status_t EFIAPI efi_install_protocol_interface_ext(void **handle, - efi_guid_t *protocol, int protocol_interface_type, + const efi_guid_t *protocol, int protocol_interface_type, void *protocol_interface) { EFI_ENTRY("%p, %pUl, %d, %p", handle, protocol, protocol_interface_type, @@ -802,7 +802,7 @@ static efi_status_t EFIAPI efi_install_protocol_interface_ext(void **handle, * @return status code */ static efi_status_t EFIAPI efi_reinstall_protocol_interface(void *handle, - efi_guid_t *protocol, void *old_interface, + const efi_guid_t *protocol, void *old_interface, void *new_interface) { EFI_ENTRY("%p, %pUl, %p, %p", handle, protocol, old_interface, @@ -823,7 +823,7 @@ static efi_status_t EFIAPI efi_reinstall_protocol_interface(void *handle, * @return status code */ static efi_status_t EFIAPI efi_uninstall_protocol_interface(void *handle, - efi_guid_t *protocol, void *protocol_interface) + const efi_guid_t *protocol, void *protocol_interface) { struct list_head *lhandle; int i; @@ -876,7 +876,7 @@ out: * @return status code */ static efi_status_t EFIAPI efi_uninstall_protocol_interface_ext(void *handle, - efi_guid_t *protocol, void *protocol_interface) + const efi_guid_t *protocol, void *protocol_interface) { EFI_ENTRY("%p, %pUl, %p", handle, protocol, protocol_interface);
@@ -897,9 +897,10 @@ static efi_status_t EFIAPI efi_uninstall_protocol_interface_ext(void *handle, * @registration key for retrieving the registration information * @return status code */ -static efi_status_t EFIAPI efi_register_protocol_notify(efi_guid_t *protocol, - struct efi_event *event, - void **registration) +static efi_status_t EFIAPI efi_register_protocol_notify( + const efi_guid_t *protocol, + struct efi_event *event, + void **registration) { EFI_ENTRY("%pUl, %p, %p", protocol, event, registration); return EFI_EXIT(EFI_OUT_OF_RESOURCES); @@ -917,7 +918,7 @@ static efi_status_t EFIAPI efi_register_protocol_notify(efi_guid_t *protocol, * @return 0 if the handle implements the protocol */ static int efi_search(enum efi_locate_search_type search_type, - efi_guid_t *protocol, void *search_key, + const efi_guid_t *protocol, void *search_key, struct efi_object *efiobj) { int i; @@ -954,7 +955,7 @@ static int efi_search(enum efi_locate_search_type search_type, */ static efi_status_t efi_locate_handle( enum efi_locate_search_type search_type, - efi_guid_t *protocol, void *search_key, + const efi_guid_t *protocol, void *search_key, unsigned long *buffer_size, efi_handle_t *buffer) { struct list_head *lhandle; @@ -1006,7 +1007,7 @@ static efi_status_t efi_locate_handle( */ static efi_status_t EFIAPI efi_locate_handle_ext( enum efi_locate_search_type search_type, - efi_guid_t *protocol, void *search_key, + const efi_guid_t *protocol, void *search_key, unsigned long *buffer_size, efi_handle_t *buffer) { EFI_ENTRY("%d, %pUl, %p, %p, %p", search_type, protocol, search_key, @@ -1028,7 +1029,8 @@ static efi_status_t EFIAPI efi_locate_handle_ext( * @device handle of the device * @return status code */ -static efi_status_t EFIAPI efi_locate_device_path(efi_guid_t *protocol, +static efi_status_t EFIAPI efi_locate_device_path( + const efi_guid_t *protocol, struct efi_device_path **device_path, efi_handle_t *device) { @@ -1567,7 +1569,7 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle, * @return status code */ static efi_status_t EFIAPI efi_close_protocol(void *handle, - efi_guid_t *protocol, + const efi_guid_t *protocol, void *agent_handle, void *controller_handle) { @@ -1590,7 +1592,7 @@ static efi_status_t EFIAPI efi_close_protocol(void *handle, * @return status code */ static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle, - efi_guid_t *protocol, + const efi_guid_t *protocol, struct efi_open_protocol_info_entry **entry_buffer, unsigned long *entry_count) { @@ -1679,7 +1681,7 @@ static efi_status_t EFIAPI efi_protocols_per_handle(void *handle, */ static efi_status_t EFIAPI efi_locate_handle_buffer( enum efi_locate_search_type search_type, - efi_guid_t *protocol, void *search_key, + const efi_guid_t *protocol, void *search_key, unsigned long *no_handles, efi_handle_t **buffer) { efi_status_t r; @@ -1721,7 +1723,7 @@ out: * @registration registration key passed to the notification function * @protocol_interface interface implementing the protocol */ -static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol, +static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol, void *registration, void **protocol_interface) { @@ -1774,7 +1776,7 @@ static efi_status_t EFIAPI efi_install_multiple_protocol_interfaces( EFI_ENTRY("%p", handle);
va_list argptr; - efi_guid_t *protocol; + const efi_guid_t *protocol; void *protocol_interface; efi_status_t r = EFI_SUCCESS; int i = 0; @@ -1905,7 +1907,7 @@ static void EFIAPI efi_set_mem(void *buffer, size_t size, uint8_t value) * @return status code */ static efi_status_t EFIAPI efi_open_protocol( - void *handle, efi_guid_t *protocol, + void *handle, const efi_guid_t *protocol, void **protocol_interface, void *agent_handle, void *controller_handle, uint32_t attributes) { @@ -1989,7 +1991,7 @@ out: * @return status code */ static efi_status_t EFIAPI efi_handle_protocol(void *handle, - efi_guid_t *protocol, + const efi_guid_t *protocol, void **protocol_interface) { return efi_open_protocol(handle, protocol, protocol_interface, NULL,

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
We need to call some boottime services internally. Our GUIDs are stored as const efi_guid_t *.
The boottime services never change GUIDs. So we can define the parameters as const efi_guid_t *.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_api.h | 27 ++++++++++++++------------- lib/efi_loader/efi_boottime.c | 40 +++++++++++++++++++++------------------- 2 files changed, 35 insertions(+), 32 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

The UEFI spec defines parameter index of WaitForEvent as UINTN*. So we should use size_t here.
I deliberately do not use UINTN because I hold a following patch that will eliminate UINTN because uppercase types to not match the U-Boot coding style.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_api.h | 2 +- lib/efi_loader/efi_boottime.c | 2 +- lib/efi_selftest/efi_selftest_events.c | 2 +- lib/efi_selftest/efi_selftest_tpl.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index aa4306aac9..c44dc9d0cb 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -71,7 +71,7 @@ struct efi_boot_services { enum efi_timer_delay type, uint64_t trigger_time); efi_status_t (EFIAPI *wait_for_event)(unsigned long number_of_events, - struct efi_event **event, unsigned long *index); + struct efi_event **event, size_t *index); efi_status_t (EFIAPI *signal_event)(struct efi_event *event); efi_status_t (EFIAPI *close_event)(struct efi_event *event); efi_status_t (EFIAPI *check_event)(struct efi_event *event); diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index e5adc17fab..976d5822f7 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -553,7 +553,7 @@ static efi_status_t EFIAPI efi_set_timer_ext(struct efi_event *event, */ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events, struct efi_event **event, - unsigned long *index) + size_t *index) { int i, j;
diff --git a/lib/efi_selftest/efi_selftest_events.c b/lib/efi_selftest/efi_selftest_events.c index 532f165d43..b2cdc150da 100644 --- a/lib/efi_selftest/efi_selftest_events.c +++ b/lib/efi_selftest/efi_selftest_events.c @@ -108,7 +108,7 @@ static int teardown(void) */ static int execute(void) { - unsigned long index; + size_t index; efi_status_t ret;
/* Set 10 ms timer */ diff --git a/lib/efi_selftest/efi_selftest_tpl.c b/lib/efi_selftest/efi_selftest_tpl.c index 5d13f3b52d..0b78ee7595 100644 --- a/lib/efi_selftest/efi_selftest_tpl.c +++ b/lib/efi_selftest/efi_selftest_tpl.c @@ -110,7 +110,7 @@ static int teardown(void) */ static int execute(void) { - unsigned long index; + size_t index; efi_status_t ret; UINTN old_tpl;

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
The UEFI spec defines parameter index of WaitForEvent as UINTN*. So we should use size_t here.
I deliberately do not use UINTN because I hold a following patch that will eliminate UINTN because uppercase types to not match the U-Boot coding style.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_api.h | 2 +- lib/efi_loader/efi_boottime.c | 2 +- lib/efi_selftest/efi_selftest_events.c | 2 +- lib/efi_selftest/efi_selftest_tpl.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

WaitForPacket is an event and not a function pointer.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_api.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/efi_api.h b/include/efi_api.h index c44dc9d0cb..308baeec49 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -633,7 +633,7 @@ struct efi_simple_network ulong *header_size, ulong *buffer_size, void *buffer, struct efi_mac_address *src_addr, struct efi_mac_address *dest_addr, u16 *protocol); - void (EFIAPI *waitforpacket)(void); + struct efi_event *wait_for_packet; struct efi_simple_network_mode *mode; };

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
WaitForPacket is an event and not a function pointer.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_api.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org

Remove extraneous commas. Add comment.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_api.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 308baeec49..8c227ce703 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -593,11 +593,12 @@ struct efi_simple_network_mode { u8 media_present; };
-#define EFI_SIMPLE_NETWORK_RECEIVE_UNICAST 0x01, -#define EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST 0x02, -#define EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST 0x04, -#define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS 0x08, -#define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST 0x10, +/* receive_filters bit mask */ +#define EFI_SIMPLE_NETWORK_RECEIVE_UNICAST 0x01 +#define EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST 0x02 +#define EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST 0x04 +#define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS 0x08 +#define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST 0x10
struct efi_simple_network {

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Remove extraneous commas. Add comment.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_api.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

Provide the simple network protocol revision. This revision number could be used to identify backwards compatible enhancements of the protocol.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_api.h | 3 +++ lib/efi_loader/efi_net.c | 1 + 2 files changed, 4 insertions(+)
diff --git a/include/efi_api.h b/include/efi_api.h index 8c227ce703..2f31464cb3 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -600,6 +600,9 @@ struct efi_simple_network_mode { #define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS 0x08 #define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST 0x10
+/* revision of the simple network protocol */ +#define EFI_SIMPLE_NETWORK_PROTOCOL_REVISION 0x00010000 + struct efi_simple_network { u64 revision; diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 91f1e4a69e..fb23bcf633 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -228,6 +228,7 @@ int efi_net_register(void) netobj->parent.protocols[2].guid = &efi_pxe_guid; netobj->parent.protocols[2].protocol_interface = &netobj->pxe; netobj->parent.handle = &netobj->net; + netobj->net.revision = EFI_SIMPLE_NETWORK_PROTOCOL_REVISION; netobj->net.start = efi_net_start; netobj->net.stop = efi_net_stop; netobj->net.initialize = efi_net_initialize;

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Provide the simple network protocol revision. This revision number could be used to identify backwards compatible enhancements of the protocol.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_api.h | 3 +++ lib/efi_loader/efi_net.c | 1 + 2 files changed, 4 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org

The length of a MAC address is 6. We have to set this length in the EFI_SIMPLE_NETWORK_MODE structure of the EFI_SIMPLE_NETWORK_PROTOCOL.
Without this patch iPXE fails to initialize the network with error message SNP MAC(001e0633bcbf,0x0) has invalid hardware address length 0
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- lib/efi_loader/efi_net.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index fb23bcf633..9def34cf93 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -245,6 +245,7 @@ int efi_net_register(void) netobj->net.mode = &netobj->net_mode; netobj->net_mode.state = EFI_NETWORK_STARTED; memcpy(netobj->net_mode.current_address.mac_addr, eth_get_ethaddr(), 6); + netobj->net_mode.hwaddr_size = ARP_HLEN; netobj->net_mode.max_packet_size = PKTSIZE;
netobj->pxe.mode = &netobj->pxe_mode;

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
The length of a MAC address is 6. We have to set this length in the EFI_SIMPLE_NETWORK_MODE structure of the EFI_SIMPLE_NETWORK_PROTOCOL.
Without this patch iPXE fails to initialize the network with error message SNP MAC(001e0633bcbf,0x0) has invalid hardware address length 0
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
lib/efi_loader/efi_net.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org

U-Boot does not implement all functions of the simple network protocol. The unimplemented functions return either of EFI_SUCCESS and EFI_INVALID_PARAMETER.
The UEFI spec foresees to return EFI_UNSUPPORTED in these cases.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- lib/efi_loader/efi_net.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 9def34cf93..569bf367a8 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -78,9 +78,7 @@ static efi_status_t EFIAPI efi_net_receive_filters( EFI_ENTRY("%p, %x, %x, %x, %lx, %p", this, enable, disable, reset_mcast_filter, mcast_filter_count, mcast_filter);
- /* XXX Do we care? */ - - return EFI_EXIT(EFI_SUCCESS); + return EFI_EXIT(EFI_UNSUPPORTED); }
static efi_status_t EFIAPI efi_net_station_address( @@ -89,7 +87,7 @@ static efi_status_t EFIAPI efi_net_station_address( { EFI_ENTRY("%p, %x, %p", this, reset, new_mac);
- return EFI_EXIT(EFI_INVALID_PARAMETER); + return EFI_EXIT(EFI_UNSUPPORTED); }
static efi_status_t EFIAPI efi_net_statistics(struct efi_simple_network *this, @@ -98,7 +96,7 @@ static efi_status_t EFIAPI efi_net_statistics(struct efi_simple_network *this, { EFI_ENTRY("%p, %x, %p, %p", this, reset, stat_size, stat_table);
- return EFI_EXIT(EFI_INVALID_PARAMETER); + return EFI_EXIT(EFI_UNSUPPORTED); }
static efi_status_t EFIAPI efi_net_mcastiptomac(struct efi_simple_network *this, @@ -118,7 +116,7 @@ static efi_status_t EFIAPI efi_net_nvdata(struct efi_simple_network *this, EFI_ENTRY("%p, %x, %lx, %lx, %p", this, read_write, offset, buffer_size, buffer);
- return EFI_EXIT(EFI_INVALID_PARAMETER); + return EFI_EXIT(EFI_UNSUPPORTED); }
static efi_status_t EFIAPI efi_net_get_status(struct efi_simple_network *this,

On 5 October 2017 at 08:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
U-Boot does not implement all functions of the simple network protocol. The unimplemented functions return either of EFI_SUCCESS and EFI_INVALID_PARAMETER.
The UEFI spec foresees to return EFI_UNSUPPORTED in these cases.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
lib/efi_loader/efi_net.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

A timer event is defined. The timer handler cares for receiving new packets.
efi_timer_check is called both in efi_net_transmit and efi_net_receive to enable events during network communication.
Calling efi_timer_check in efi_net_get_status is implemented in a separate patch.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- lib/efi_loader/efi_net.c | 53 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 569bf367a8..3498b96f7a 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -19,6 +19,11 @@ static const efi_guid_t efi_pxe_guid = EFI_PXE_GUID; static struct efi_pxe_packet *dhcp_ack; static bool new_rx_packet; static void *new_tx_packet; +/* + * The notification function of this event is called in every timer cycle + * to check if a new network packet has been received. + */ +static struct efi_event *network_timer_event;
struct efi_net_obj { /* Generic EFI object parent class data */ @@ -143,6 +148,8 @@ static efi_status_t EFIAPI efi_net_transmit(struct efi_simple_network *this, EFI_ENTRY("%p, %lx, %lx, %p, %p, %p, %p", this, header_size, buffer_size, buffer, src_addr, dest_addr, protocol);
+ efi_timer_check(); + if (header_size) { /* We would need to create the header if header_size != 0 */ return EFI_EXIT(EFI_INVALID_PARAMETER); @@ -174,9 +181,7 @@ static efi_status_t EFIAPI efi_net_receive(struct efi_simple_network *this, EFI_ENTRY("%p, %p, %p, %p, %p, %p, %p", this, header_size, buffer_size, buffer, src_addr, dest_addr, protocol);
- push_packet = efi_net_push; - eth_rx(); - push_packet = NULL; + efi_timer_check();
if (!new_rx_packet) return EFI_EXIT(EFI_NOT_READY); @@ -204,10 +209,32 @@ void efi_net_set_dhcp_ack(void *pkt, int len) memcpy(dhcp_ack, pkt, min(len, maxsize)); }
+/* + * Check if a new network packet has been received. + * + * This notification function is called in every timer cycle. + * + * @event the event for which this notification function is registered + * @context event context - not used in this function + */ +static void EFIAPI efi_network_timer_notify(struct efi_event *event, + void *context) +{ + EFI_ENTRY("%p, %p", event, context); + + if (!new_rx_packet) { + push_packet = efi_net_push; + eth_rx(); + push_packet = NULL; + } + EFI_EXIT(EFI_SUCCESS); +} + /* This gets called from do_bootefi_exec(). */ int efi_net_register(void) { struct efi_net_obj *netobj; + efi_status_t r;
if (!eth_get_dev()) { /* No eth device active, don't expose any */ @@ -253,5 +280,25 @@ int efi_net_register(void) /* Hook net up to the device list */ list_add_tail(&netobj->parent.link, &efi_obj_list);
+ /* + * Create a timer event. + * + * The notification function is used to check if an new networkd packet + * has been received. + */ + r = efi_create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL, TPL_CALLBACK, + efi_network_timer_notify, NULL, + &network_timer_event); + if (r != EFI_SUCCESS) { + printf("ERROR: Failed to register network event\n"); + return r; + } + /* Network is time critical, create event in every timer cyle */ + r = efi_set_timer(network_timer_event, EFI_TIMER_PERIODIC, 0); + if (r != EFI_SUCCESS) { + printf("ERROR: Failed to set network timer\n"); + return r; + } + return 0; }

On 5 October 2017 at 08:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
A timer event is defined. The timer handler cares for receiving new packets.
efi_timer_check is called both in efi_net_transmit and efi_net_receive to enable events during network communication.
Calling efi_timer_check in efi_net_get_status is implemented in a separate patch.
Can you explain in the commit message why this patch is needed?
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
lib/efi_loader/efi_net.c | 53 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

The WaitForPacket event informs that a network package has been received by the SimpleNetworkProtocol.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 signaled has been renamed to is_signaled and is bool --- lib/efi_loader/efi_net.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 3498b96f7a..f63db686af 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -24,6 +24,10 @@ static void *new_tx_packet; * to check if a new network packet has been received. */ static struct efi_event *network_timer_event; +/* + * This event is signaled when a packet has been received. + */ +static struct efi_event *wait_for_packet;
struct efi_net_obj { /* Generic EFI object parent class data */ @@ -227,6 +231,8 @@ static void EFIAPI efi_network_timer_notify(struct efi_event *event, eth_rx(); push_packet = NULL; } + if (!new_rx_packet) + wait_for_packet->is_signaled = true; EFI_EXIT(EFI_SUCCESS); }
@@ -280,6 +286,17 @@ int efi_net_register(void) /* Hook net up to the device list */ list_add_tail(&netobj->parent.link, &efi_obj_list);
+ /* + * Create WaitForPacket event. + */ + r = efi_create_event(EVT_NOTIFY_WAIT, TPL_CALLBACK, + efi_network_timer_notify, NULL, + &wait_for_packet); + if (r != EFI_SUCCESS) { + printf("ERROR: Failed to register network event\n"); + return r; + } + netobj->net.wait_for_packet = wait_for_packet; /* * Create a timer event. *

On 05.10.17 16:36, Heinrich Schuchardt wrote:
The WaitForPacket event informs that a network package has been received by the SimpleNetworkProtocol.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 signaled has been renamed to is_signaled and is bool
lib/efi_loader/efi_net.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 3498b96f7a..f63db686af 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -24,6 +24,10 @@ static void *new_tx_packet;
- to check if a new network packet has been received.
*/ static struct efi_event *network_timer_event; +/*
- This event is signaled when a packet has been received.
- */
+static struct efi_event *wait_for_packet;
struct efi_net_obj { /* Generic EFI object parent class data */ @@ -227,6 +231,8 @@ static void EFIAPI efi_network_timer_notify(struct efi_event *event, eth_rx(); push_packet = NULL; }
- if (!new_rx_packet)
wait_for_packet->is_signaled = true;
Shouldn't that be if (new_rx_packet)? You want to know whether eth_rx() received something, no?
In fact, why don't you just move this whole line into efi_net_push()?
Alex
EFI_EXIT(EFI_SUCCESS); }
@@ -280,6 +286,17 @@ int efi_net_register(void) /* Hook net up to the device list */ list_add_tail(&netobj->parent.link, &efi_obj_list);
- /*
* Create WaitForPacket event.
*/
- r = efi_create_event(EVT_NOTIFY_WAIT, TPL_CALLBACK,
efi_network_timer_notify, NULL,
&wait_for_packet);
- if (r != EFI_SUCCESS) {
printf("ERROR: Failed to register network event\n");
return r;
- }
- netobj->net.wait_for_packet = wait_for_packet; /*
- Create a timer event.

The returned interrupt status was wrong.
As out transmit buffer is empty we need to always set EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT.
When we have received a packet we need to set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT.
Furthermore we should call efi_timer_check() to handle events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_api.h | 6 ++++++ lib/efi_loader/efi_net.c | 11 ++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 2f31464cb3..1f349db246 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -600,6 +600,12 @@ struct efi_simple_network_mode { #define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS 0x08 #define EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST 0x10
+/* interrupt status bit mask */ +#define EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT 0x01 +#define EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT 0x02 +#define EFI_SIMPLE_NETWORK_COMMAND_INTERRUPT 0x04 +#define EFI_SIMPLE_NETWORK_SOFTWARE_INTERRUPT 0x08 + /* revision of the simple network protocol */ #define EFI_SIMPLE_NETWORK_PROTOCOL_REVISION 0x00010000
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index f63db686af..5866119817 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -133,9 +133,14 @@ static efi_status_t EFIAPI efi_net_get_status(struct efi_simple_network *this, { EFI_ENTRY("%p, %p, %p", this, int_status, txbuf);
- /* We send packets synchronously, so nothing is outstanding */ - if (int_status) - *int_status = 0; + efi_timer_check(); + + if (int_status) { + /* We send packets synchronously, so nothing is outstanding */ + *int_status = EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; + if (new_rx_packet) + *int_status |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; + } if (txbuf) *txbuf = new_tx_packet;

On 5 October 2017 at 08:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
The returned interrupt status was wrong.
As out transmit buffer is empty we need to always set EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT.
When we have received a packet we need to set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT.
Furthermore we should call efi_timer_check() to handle events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_api.h | 6 ++++++ lib/efi_loader/efi_net.c | 11 ++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

The size fields in the Simple Network Protocol are all UINTN in the UEFI spec. So use size_t.
Provide a function description of the receive function.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_api.h | 4 ++-- lib/efi_loader/efi_net.c | 24 ++++++++++++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 1f349db246..a9a6494afe 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -636,11 +636,11 @@ struct efi_simple_network efi_status_t (EFIAPI *get_status)(struct efi_simple_network *this, u32 *int_status, void **txbuf); efi_status_t (EFIAPI *transmit)(struct efi_simple_network *this, - ulong header_size, ulong buffer_size, void *buffer, + size_t header_size, size_t buffer_size, void *buffer, struct efi_mac_address *src_addr, struct efi_mac_address *dest_addr, u16 *protocol); efi_status_t (EFIAPI *receive)(struct efi_simple_network *this, - ulong *header_size, ulong *buffer_size, void *buffer, + size_t *header_size, size_t *buffer_size, void *buffer, struct efi_mac_address *src_addr, struct efi_mac_address *dest_addr, u16 *protocol); struct efi_event *wait_for_packet; diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 5866119817..f8ae5578ba 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -150,12 +150,13 @@ static efi_status_t EFIAPI efi_net_get_status(struct efi_simple_network *this, }
static efi_status_t EFIAPI efi_net_transmit(struct efi_simple_network *this, - ulong header_size, ulong buffer_size, void *buffer, + size_t header_size, size_t buffer_size, void *buffer, struct efi_mac_address *src_addr, struct efi_mac_address *dest_addr, u16 *protocol) { - EFI_ENTRY("%p, %lx, %lx, %p, %p, %p, %p", this, header_size, - buffer_size, buffer, src_addr, dest_addr, protocol); + EFI_ENTRY("%p, %lu, %lu, %p, %p, %p, %p", this, + (unsigned long)header_size, (unsigned long)buffer_size, + buffer, src_addr, dest_addr, protocol);
efi_timer_check();
@@ -182,8 +183,23 @@ static void efi_net_push(void *pkt, int len) new_rx_packet = true; }
+/* + * Receive a packet from a network interface. + * + * This function implements the Receive service of the Simple Network Protocol. + * See the UEFI spec for details. + * + * @this the instance of the Simple Network Protocol + * @header_size size of the media header + * @buffer_size size of the buffer to receive the packet + * @buffer buffer to receive the packet + * @src_addr source MAC address + * @dest_addr destination MAC address + * @protocol protocol + * @return status code + */ static efi_status_t EFIAPI efi_net_receive(struct efi_simple_network *this, - ulong *header_size, ulong *buffer_size, void *buffer, + size_t *header_size, size_t *buffer_size, void *buffer, struct efi_mac_address *src_addr, struct efi_mac_address *dest_addr, u16 *protocol) {

On 5 October 2017 at 08:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
The size fields in the Simple Network Protocol are all UINTN in the UEFI spec. So use size_t.
Provide a function description of the receive function.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_api.h | 4 ++-- lib/efi_loader/efi_net.c | 24 ++++++++++++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

In the receive function all return values should be filled.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- lib/efi_loader/efi_net.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index f8ae5578ba..9c85e1b72f 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -203,6 +203,10 @@ static efi_status_t EFIAPI efi_net_receive(struct efi_simple_network *this, struct efi_mac_address *src_addr, struct efi_mac_address *dest_addr, u16 *protocol) { + struct ethernet_hdr *eth_hdr; + size_t hdr_size = sizeof(struct ethernet_hdr); + u16 protlen; + EFI_ENTRY("%p, %p, %p, %p, %p, %p, %p", this, header_size, buffer_size, buffer, src_addr, dest_addr, protocol);
@@ -210,13 +214,32 @@ static efi_status_t EFIAPI efi_net_receive(struct efi_simple_network *this,
if (!new_rx_packet) return EFI_EXIT(EFI_NOT_READY); - + /* Check that we at least received an Ethernet header */ + if (net_rx_packet_len < sizeof(struct ethernet_hdr)) { + new_rx_packet = false; + return EFI_EXIT(EFI_NOT_READY); + } + /* Fill export parameters */ + eth_hdr = (struct ethernet_hdr *)net_rx_packet; + protlen = ntohs(eth_hdr->et_protlen); + if (protlen == 0x8100) { + hdr_size += 4; + protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]); + } + if (header_size) + *header_size = hdr_size; + if (dest_addr) + memcpy(dest_addr, eth_hdr->et_dest, ARP_HLEN); + if (src_addr) + memcpy(src_addr, eth_hdr->et_src, ARP_HLEN); + if (protocol) + *protocol = protlen; if (*buffer_size < net_rx_packet_len) { /* Packet doesn't fit, try again with bigger buf */ *buffer_size = net_rx_packet_len; return EFI_EXIT(EFI_BUFFER_TOO_SMALL); } - + /* Copy packet */ memcpy(buffer, net_rx_packet, net_rx_packet_len); *buffer_size = net_rx_packet_len; new_rx_packet = false;

On 5 October 2017 at 08:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
In the receive function all return values should be filled.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
lib/efi_loader/efi_net.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
(in so far as I understand it)

Enclose definition in parantheses to allow using efi_st_error like a void function.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- include/efi_selftest.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/efi_selftest.h b/include/efi_selftest.h index beb662d4e1..2f0992f06e 100644 --- a/include/efi_selftest.h +++ b/include/efi_selftest.h @@ -23,8 +23,8 @@ * @... format string followed by fields to print */ #define efi_st_error(...) \ - efi_st_printf("%s(%u):\nERROR: ", __FILE__, __LINE__); \ - efi_st_printf(__VA_ARGS__) \ + (efi_st_printf("%s(%u):\nERROR: ", __FILE__, __LINE__), \ + efi_st_printf(__VA_ARGS__)) \
/* * A test may be setup and executed at boottime,

On 5 October 2017 at 08:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Enclose definition in parantheses to allow using efi_st_error like a void function.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
include/efi_selftest.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

Add %pm as format string to print a MAC address. This is helpful when analyzing network problems.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 no change --- lib/efi_selftest/efi_selftest_console.c | 41 ++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/lib/efi_selftest/efi_selftest_console.c b/lib/efi_selftest/efi_selftest_console.c index 7b5b724a61..840e2290c6 100644 --- a/lib/efi_selftest/efi_selftest_console.c +++ b/lib/efi_selftest/efi_selftest_console.c @@ -12,6 +12,37 @@ struct efi_simple_text_output_protocol *con_out; struct efi_simple_input_interface *con_in;
+/* + * Print a MAC address to an u16 string + * + * @pointer: mac address + * @buf: pointer to buffer address + * on return position of terminating zero word + */ +static void mac(void *pointer, u16 **buf) +{ + int i, j; + u16 c; + u8 *p = (u8 *)pointer; + u8 byte; + u16 *pos = *buf; + + for (i = 0; i < ARP_HLEN; ++i) { + if (i) + *pos++ = ':'; + byte = p[i]; + for (j = 4; j >= 0; j -= 4) { + c = (byte >> j) & 0x0f; + c += '0'; + if (c > '9') + c += 'a' - '9' - 1; + *pos++ = c; + } + } + *pos = 0; + *buf = pos; +} + /* * Print a pointer to an u16 string * @@ -146,7 +177,15 @@ void efi_st_printf(const char *fmt, ...) int2dec(va_arg(args, s32), &pos); break; case 'p': - pointer(va_arg(args, void*), &pos); + ++c; + switch (*c) { + case 'm': + mac(va_arg(args, void*), &pos); + break; + default: + --c; + pointer(va_arg(args, void*), &pos); + } break; case 's': s = va_arg(args, const char *);

On 5 October 2017 at 08:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Add %pm as format string to print a MAC address. This is helpful when analyzing network problems.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 no change
lib/efi_selftest/efi_selftest_console.c | 41 ++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
I wonder if we could use U-Boot's existing printf() code?

This patch provices an EFI application to check the correct function of the Simple Network Protocol implementation.
It sends a DHCP request and analyzes the DHCP offer.
Different error conditions including a 10s timeout are checked.
A successful execution will look like this:
=> bootefi nettest Scanning disk ide.blk#0... Found 1 disks WARNING: Invalid device tree, expect boot to fail Network test DHCP Discover DHCP reply received from 192.168.76.2 (52:55:c0:a8:4c:02) as broadcast message. OK. The test was completed successfully.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v2 move efi_st_memcmp to efi_selftest_util.c The function can be reused for other tests.
Use constants for return values. --- include/efi_selftest.h | 11 + lib/efi_selftest/Makefile | 8 +- lib/efi_selftest/efi_selftest_snp.c | 424 +++++++++++++++++++++++++++++++++++ lib/efi_selftest/efi_selftest_util.c | 25 +++ 4 files changed, 467 insertions(+), 1 deletion(-) create mode 100644 lib/efi_selftest/efi_selftest_snp.c create mode 100644 lib/efi_selftest/efi_selftest_util.c
diff --git a/include/efi_selftest.h b/include/efi_selftest.h index 2f0992f06e..7ec42a0406 100644 --- a/include/efi_selftest.h +++ b/include/efi_selftest.h @@ -60,6 +60,17 @@ void efi_st_exit_boot_services(void); void efi_st_printf(const char *fmt, ...) __attribute__ ((format (__printf__, 1, 2)));
+/* + * Compare memory. + * We cannot use lib/string.c due to different CFLAGS values. + * + * @buf1: first buffer + * @buf2: second buffer + * @length: number of bytes to compare + * @return: 0 if both buffers contain the same bytes + */ +int efi_st_memcmp(const void *buf1, const void *buf2, size_t length); + /* * Reads an Unicode character from the input device. * diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile index 30f1960933..e446046e02 100644 --- a/lib/efi_selftest/Makefile +++ b/lib/efi_selftest/Makefile @@ -15,12 +15,18 @@ CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI) CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI) CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI) CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI) +CFLAGS_efi_selftest_snp.o := $(CFLAGS_EFI) +CFLAGS_REMOVE_efi_selftest_snp.o := $(CFLAGS_NON_EFI) CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI) CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI) +CFLAGS_efi_selftest_util.o := $(CFLAGS_EFI) +CFLAGS_REMOVE_efi_selftest_util.o := $(CFLAGS_NON_EFI)
obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \ efi_selftest.o \ efi_selftest_console.o \ efi_selftest_events.o \ efi_selftest_exitbootservices.o \ -efi_selftest_tpl.o +efi_selftest_snp.o \ +efi_selftest_tpl.o \ +efi_selftest_util.o diff --git a/lib/efi_selftest/efi_selftest_snp.c b/lib/efi_selftest/efi_selftest_snp.c new file mode 100644 index 0000000000..638be0147d --- /dev/null +++ b/lib/efi_selftest/efi_selftest_snp.c @@ -0,0 +1,424 @@ +/* + * efi_selftest_snp + * + * Copyright (c) 2017 Heinrich Schuchardt xypron.glpk@gmx.de + * + * SPDX-License-Identifier: GPL-2.0+ + * + * This unit test covers the Simple Network Protocol as well as + * the CopyMem and SetMem boottime services. + * + * A DHCP discover message is sent. The test is successful if a + * DHCP reply is received. + * + * TODO: Once ConnectController and DisconnectController are implemented + * we should connect our code as controller. + */ + +#include <efi_selftest.h> + +/* + * MAC address for broadcasts + */ +static const u8 BROADCAST_MAC[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}; + +struct dhcp_hdr { + u8 op; +#define BOOTREQUEST 1 +#define BOOTREPLY 2 + u8 htype; +# define HWT_ETHER 1 + u8 hlen; +# define HWL_ETHER 6 + u8 hops; + u32 xid; + u16 secs; + u16 flags; +#define DHCP_FLAGS_UNICAST 0x0000 +#define DHCP_FLAGS_BROADCAST 0x0080 + u32 ciaddr; + u32 yiaddr; + u32 siaddr; + u32 giaddr; + u8 chaddr[16]; + u8 sname[64]; + u8 file[128]; +}; + +/* + * Message type option. + */ +#define DHCP_MESSAGE_TYPE 0x35 +#define DHCPDISCOVER 1 +#define DHCPOFFER 2 +#define DHCPREQUEST 3 +#define DHCPDECLINE 4 +#define DHCPACK 5 +#define DHCPNAK 6 +#define DHCPRELEASE 7 + +struct dhcp { + struct ethernet_hdr eth_hdr; + struct ip_udp_hdr ip_udp; + struct dhcp_hdr dhcp_hdr; + u8 opt[128]; +} __packed; + +static struct efi_boot_services *boottime; +static struct efi_simple_network *net; +static struct efi_event *timer; +static const efi_guid_t efi_net_guid = EFI_SIMPLE_NETWORK_GUID; +/* IP packet ID */ +static unsigned int net_ip_id; + +/* + * Compute the checksum of the IP header. We cover even values of length only. + * We cannot use net/checksum.c due to different CFLAGS values. + * + * @buf: IP header + * @len: length of header in bytes + * @return: checksum + */ +static unsigned int efi_ip_checksum(const void *buf, size_t len) +{ + size_t i; + u32 sum = 0; + const u16 *pos = buf; + + for (i = 0; i < len; i += 2) + sum += *pos++; + + sum = (sum >> 16) + (sum & 0xffff); + sum += sum >> 16; + sum = ~sum & 0xffff; + + return sum; +} + +/* + * Transmit a DHCPDISCOVER message. + */ +static efi_status_t send_dhcp_discover(void) +{ + efi_status_t ret; + struct dhcp p = {}; + + /* + * Fill ethernet header + */ + boottime->copy_mem(p.eth_hdr.et_dest, (void *)BROADCAST_MAC, ARP_HLEN); + boottime->copy_mem(p.eth_hdr.et_src, &net->mode->current_address, + ARP_HLEN); + p.eth_hdr.et_protlen = htons(PROT_IP); + /* + * Fill IP header + */ + p.ip_udp.ip_hl_v = 0x45; + p.ip_udp.ip_len = htons(sizeof(struct dhcp) - + sizeof(struct ethernet_hdr)); + p.ip_udp.ip_id = htons(++net_ip_id); + p.ip_udp.ip_off = htons(IP_FLAGS_DFRAG); + p.ip_udp.ip_ttl = 0xff; /* time to live */ + p.ip_udp.ip_p = IPPROTO_UDP; + boottime->set_mem(&p.ip_udp.ip_dst, 4, 0xff); + p.ip_udp.ip_sum = efi_ip_checksum(&p.ip_udp, IP_HDR_SIZE); + + /* + * Fill UDP header + */ + p.ip_udp.udp_src = htons(68); + p.ip_udp.udp_dst = htons(67); + p.ip_udp.udp_len = htons(sizeof(struct dhcp) - + sizeof(struct ethernet_hdr) - + sizeof(struct ip_hdr)); + /* + * Fill DHCP header + */ + p.dhcp_hdr.op = BOOTREQUEST; + p.dhcp_hdr.htype = HWT_ETHER; + p.dhcp_hdr.hlen = HWL_ETHER; + p.dhcp_hdr.flags = htons(DHCP_FLAGS_UNICAST); + boottime->copy_mem(&p.dhcp_hdr.chaddr, + &net->mode->current_address, ARP_HLEN); + /* + * Fill options + */ + p.opt[0] = 0x63; /* DHCP magic cookie */ + p.opt[1] = 0x82; + p.opt[2] = 0x53; + p.opt[3] = 0x63; + p.opt[4] = DHCP_MESSAGE_TYPE; + p.opt[5] = 0x01; /* length */ + p.opt[6] = DHCPDISCOVER; + p.opt[7] = 0x39; /* maximum message size */ + p.opt[8] = 0x02; /* length */ + p.opt[9] = 0x02; /* 576 bytes */ + p.opt[10] = 0x40; + p.opt[11] = 0xff; /* end of options */ + + /* + * Transmit DHCPDISCOVER message. + */ + ret = net->transmit(net, 0, sizeof(struct dhcp), &p, NULL, NULL, 0); + if (ret != EFI_SUCCESS) + efi_st_error("Sending a DHCP request failed\n"); + else + efi_st_printf("DHCP Discover\n"); + return ret; +} + +/* + * Setup unit test. + * + * Create a 1 s periodic timer. + * Start the network driver. + * + * @handle: handle of the loaded image + * @systable: system table + * @return: EFI_ST_SUCCESS for success + */ +static int setup(const efi_handle_t handle, + const struct efi_system_table *systable) +{ + efi_status_t ret; + + boottime = systable->boottime; + + /* + * Create a timer event. + */ + ret = boottime->create_event(EVT_TIMER, TPL_CALLBACK, NULL, NULL, + &timer); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to create event\n"); + return EFI_ST_FAILURE; + } + /* + * Set timer period to 1s. + */ + ret = boottime->set_timer(timer, EFI_TIMER_PERIODIC, 10000000); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to locate simple network protocol\n"); + return EFI_ST_FAILURE; + } + /* + * Find an interface implementing the SNP protocol. + */ + ret = boottime->locate_protocol(&efi_net_guid, NULL, (void **)&net); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to locate simple network protocol\n"); + return EFI_ST_FAILURE; + } + /* + * Check hardware address size. + */ + if (!net->mode) { + efi_st_error("Mode not provided\n"); + return EFI_ST_FAILURE; + } + if (net->mode->hwaddr_size != ARP_HLEN) { + efi_st_error("HwAddressSize = %u, expected %u\n", + net->mode->hwaddr_size, ARP_HLEN); + return EFI_ST_FAILURE; + } + /* + * Check that WaitForPacket event exists. + */ + if (!net->wait_for_packet) { + efi_st_error("WaitForPacket event missing\n"); + return EFI_ST_FAILURE; + } + /* + * Initialize network adapter. + */ + ret = net->initialize(net, 0, 0); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to initialize network adapter\n"); + return EFI_ST_FAILURE; + } + /* + * Start network adapter. + */ + ret = net->start(net); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to start network adapter\n"); + return EFI_ST_FAILURE; + } + return EFI_ST_SUCCESS; +} + +/* + * Execute unit test. + * + * A DHCP discover message is sent. The test is successful if a + * DHCP reply is received within 10 seconds. + * + * @return: EFI_ST_SUCCESS for success + */ +static int execute(void) +{ + efi_status_t ret; + struct efi_event *events[2]; + size_t index; + union { + struct dhcp p; + u8 b[PKTSIZE]; + } buffer; + struct efi_mac_address srcaddr; + struct efi_mac_address destaddr; + size_t buffer_size; + u8 *addr; + /* + * The timeout is to occur after 10 s. + */ + unsigned int timeout = 10; + + /* + * Send DHCP discover message + */ + ret = send_dhcp_discover(); + if (ret != EFI_SUCCESS) + return EFI_ST_FAILURE; + + /* + * If we would call WaitForEvent only with the WaitForPacket event, + * our code would block until a packet is received which might never + * occur. By calling WaitFor event with both a timer event and the + * WaitForPacket event we can escape this blocking situation. + * + * If the timer event occurs before we have received a DHCP reply + * a further DHCP discover message is sent. + */ + events[0] = timer; + events[1] = net->wait_for_packet; + for (;;) { + /* + * Wait for packet to be received or timer event. + */ + boottime->wait_for_event(2, events, &index); + if (index == 0) { + /* + * The timer event occurred. Check for timeout. + */ + --timeout; + if (!timeout) { + efi_st_error("Timeout occurred\n"); + return EFI_ST_FAILURE; + } + /* + * Send further DHCP discover message + */ + ret = send_dhcp_discover(); + if (ret != EFI_SUCCESS) + return EFI_ST_FAILURE; + continue; + } + /* + * Receive packet + */ + buffer_size = sizeof(buffer); + net->receive(net, NULL, &buffer_size, &buffer, + &srcaddr, &destaddr, NULL); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to receive packet"); + return EFI_ST_FAILURE; + } + /* + * Check the packet is meant for this system. + * Unfortunately QEMU ignores the broadcast flag. + * So we have to check for broadcasts too. + */ + if (efi_st_memcmp(&destaddr, &net->mode->current_address, + ARP_HLEN) && + efi_st_memcmp(&destaddr, BROADCAST_MAC, ARP_HLEN)) + continue; + /* + * Check this is a DHCP reply + */ + if (buffer.p.eth_hdr.et_protlen != ntohs(PROT_IP) || + buffer.p.ip_udp.ip_hl_v != 0x45 || + buffer.p.ip_udp.ip_p != IPPROTO_UDP || + buffer.p.ip_udp.udp_src != ntohs(67) || + buffer.p.ip_udp.udp_dst != ntohs(68) || + buffer.p.dhcp_hdr.op != BOOTREPLY) + continue; + /* + * We successfully received a DHCP reply. + */ + break; + } + + /* + * Write a log message. + */ + addr = (u8 *)&buffer.p.ip_udp.ip_src; + efi_st_printf("DHCP reply received from %u.%u.%u.%u (%pm) ", + addr[0], addr[1], addr[2], addr[3], &srcaddr); + if (!efi_st_memcmp(&destaddr, BROADCAST_MAC, ARP_HLEN)) + efi_st_printf("as broadcast message.\n"); + else + efi_st_printf("as unicast message.\n"); + + return EFI_ST_SUCCESS; +} + +/* + * Tear down unit test. + * + * Close the timer event created in setup. + * Shut down the network adapter. + * + * @return: EFI_ST_SUCCESS for success + */ +static int teardown(void) +{ + efi_status_t ret; + int exit_status = EFI_ST_SUCCESS; + + if (timer) { + /* + * Stop timer. + */ + ret = boottime->set_timer(timer, EFI_TIMER_STOP, 0); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to stop timer"); + exit_status = EFI_ST_FAILURE; + } + /* + * Close timer event. + */ + ret = boottime->close_event(timer); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to close event"); + exit_status = EFI_ST_FAILURE; + } + } + if (net) { + /* + * Stop network adapter. + */ + ret = net->stop(net); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to stop network adapter\n"); + exit_status = EFI_ST_FAILURE; + } + /* + * Shut down network adapter. + */ + ret = net->shutdown(net); + if (ret != EFI_SUCCESS) { + efi_st_error("Failed to shut down network adapter\n"); + exit_status = EFI_ST_FAILURE; + } + } + + return exit_status; +} + +EFI_UNIT_TEST(snp) = { + .name = "simple network protocol", + .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT, + .setup = setup, + .execute = execute, + .teardown = teardown, +}; diff --git a/lib/efi_selftest/efi_selftest_util.c b/lib/efi_selftest/efi_selftest_util.c new file mode 100644 index 0000000000..c9c295e2fb --- /dev/null +++ b/lib/efi_selftest/efi_selftest_util.c @@ -0,0 +1,25 @@ +/* + * efi_selftest_util + * + * Copyright (c) 2017 Heinrich Schuchardt xypron.glpk@gmx.de + * + * SPDX-License-Identifier: GPL-2.0+ + * + * Utility functions + */ + +#include <efi_selftest.h> + +int efi_st_memcmp(const void *buf1, const void *buf2, size_t length) +{ + const u8 *pos1 = buf1; + const u8 *pos2 = buf2; + + for (; length; --length) { + if (*pos1 != *pos2) + return pos1 - pos2; + ++pos1; + ++pos2; + } + return EFI_ST_SUCCESS; +}

On 5 October 2017 at 08:36, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
This patch provices an EFI application to check the correct function of the Simple Network Protocol implementation.
It sends a DHCP request and analyzes the DHCP offer.
Different error conditions including a 10s timeout are checked.
A successful execution will look like this:
=> bootefi nettest Scanning disk ide.blk#0... Found 1 disks WARNING: Invalid device tree, expect boot to fail Network test DHCP Discover DHCP reply received from 192.168.76.2 (52:55:c0:a8:4c:02) as broadcast message. OK. The test was completed successfully.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v2 move efi_st_memcmp to efi_selftest_util.c The function can be reused for other tests.
Use constants for return values.
include/efi_selftest.h | 11 + lib/efi_selftest/Makefile | 8 +- lib/efi_selftest/efi_selftest_snp.c | 424 +++++++++++++++++++++++++++++++++++ lib/efi_selftest/efi_selftest_util.c | 25 +++ 4 files changed, 467 insertions(+), 1 deletion(-) create mode 100644 lib/efi_selftest/efi_selftest_snp.c create mode 100644 lib/efi_selftest/efi_selftest_util.c
Reviewed-by: Simon Glass sjg@chromium.org

On 05.10.17 16:35, Heinrich Schuchardt wrote:
This patch series focuses on
- correction of parameter types for boot services
- fixes for the SetMem and CopyMem boot services
- fixes for the simple network protocol implementation (SNP)
- a unit test for SetMem, CopyMem and the simple network protocol
The unit test broadcasts a DHCPDISCOVER messager over the network and receives the reply.
This patch series is based on
- efi-next tree https://github.com/agraf/u-boot/tree/efi-next
- [PATCH 1/1] efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST https://patchwork.ozlabs.org/patch/816412/ This patch enables the unit test on qemu-x86_defconfig
- [PATCH 1/1] efi_loader: provide function comments for boot services https://patchwork.ozlabs.org/patch/817010/
Looks quite good to me. I've applied them (with the modification I suggested), running through tests and will push them to master if nobody (human or machine) objects within the next 1-2 days.
Alex

On 10/06/2017 02:08 PM, Alexander Graf wrote:
On 05.10.17 16:35, Heinrich Schuchardt wrote:
This patch series focuses on
- correction of parameter types for boot services
- fixes for the SetMem and CopyMem boot services
- fixes for the simple network protocol implementation (SNP)
- a unit test for SetMem, CopyMem and the simple network protocol
The unit test broadcasts a DHCPDISCOVER messager over the network and receives the reply.
This patch series is based on
- efi-next tree https://github.com/agraf/u-boot/tree/efi-next
- [PATCH 1/1] efi_selftest: enable CONFIG_CMD_BOOTEFI_SELFTEST https://patchwork.ozlabs.org/patch/816412/ This patch enables the unit test on qemu-x86_defconfig
- [PATCH 1/1] efi_loader: provide function comments for boot services https://patchwork.ozlabs.org/patch/817010/
Looks quite good to me. I've applied them (with the modification I suggested), running through tests and will push them to master if nobody (human or machine) objects within the next 1-2 days.
Alex
You moved setting is_signaled to efi_net_push.
181 static void efi_net_push(void *pkt, int len) 182 { 183 new_rx_packet = true; 184 wait_for_packet->is_signaled = true; 185 }
The code looks fine to me.
The selftest for the Simple Network Protocol correctly receives a DHCP packet.
Thank you.
I just sent you a fix for efi_st_memcmp: [PATCH 1/1] efi_selftest: efi_st_memcmp return difference of bytes
Best regards
Heinrich
participants (3)
-
Alexander Graf
-
Heinrich Schuchardt
-
Simon Glass