[U-Boot] [PATCH 0/3] efi_loader: event services
Currently U-Boot only allows a single event. This is sufficient to run grub but not for other workloads as iPXE.
The EFI_SIMPLE_TEXT_INPUT_PROTOCOL requires an event WaitForKey. This is used for instance by the iPXE shell.
With the 1st patch events are stored in an array. Four events seems to be enough for now.
The 2nd patch implements the WaitForKey event for the EFI_SIMPLE_TEXT_INPUT_PROTOCOL.
The 3rd patch disables debug output for the WaitForKey event in function efi_check_event.
Heinrich Schuchardt (3): efi_loader: implement multiple event support efi_loader: implement WaitForKey efi_loader: no debug message on wait for key
include/efi_api.h | 8 +- include/efi_loader.h | 26 +++++++ lib/efi_loader/efi_boottime.c | 177 ++++++++++++++++++++++++++++-------------- lib/efi_loader/efi_console.c | 2 +- 4 files changed, 151 insertions(+), 62 deletions(-)
Up to now the boot time supported only a single event. This patch now allows four events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- include/efi_api.h | 8 ++- include/efi_loader.h | 23 ++++++ lib/efi_loader/efi_boottime.c | 161 ++++++++++++++++++++++++++---------------- 3 files changed, 131 insertions(+), 61 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 42cd47ff08..18bef722c6 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -28,8 +28,12 @@ enum efi_event_type { EFI_TIMER_RELATIVE = 2 };
-#define EVT_NOTIFY_WAIT 0x00000100 -#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_TIMER 0x80000000 +#define EVT_RUNTIME 0x40000000 +#define EVT_NOTIFY_WAIT 0x00000100 +#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201 +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202
/* EFI Boot Services table */ struct efi_boot_services { diff --git a/include/efi_loader.h b/include/efi_loader.h index c620652307..a35b971f7e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -71,6 +71,29 @@ struct efi_object { void *handle; };
+/** + * struct efi_event + * + * @type: Type of event + * @trigger_type: Type of timer + * @trigger_time: Period of the timer + * @trigger_next: Next time to trigger the timer + * @nofify_function: Function to call when the event is triggered + * @notify_context: Data to be passed to the notify function + * @signaled: The notify function was already called + */ +struct efi_event { + u32 type; + unsigned long notify_tpl; + void (EFIAPI *notify_function)(void *event, void *context); + void *notify_context; + u64 trigger_next; + u32 trigger_time; + enum efi_event_type trigger_type; + int signaled; +}; + + /* This list contains all UEFI objects we know of */ extern struct list_head efi_obj_list;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3060c25a2a..ef3e7d9d52 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -162,21 +162,10 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer) }
/* - * Our event capabilities are very limited. Only support a single - * event to exist, so we don't need to maintain lists. + * Our event capabilities are very limited. Only a small limited + * number of events is allowed to coexist. */ -static struct { - enum efi_event_type type; - u32 trigger_type; - u32 trigger_time; - u64 trigger_next; - unsigned long notify_tpl; - void (EFIAPI *notify_function) (void *event, void *context); - void *notify_context; -} efi_event = { - /* Disable timers on bootup */ - .trigger_next = -1ULL, -}; +static struct efi_event efi_events[16];
static efi_status_t EFIAPI efi_create_event( enum efi_event_type type, ulong notify_tpl, @@ -184,13 +173,10 @@ static efi_status_t EFIAPI efi_create_event( void *context), void *notify_context, void **event) { + int i; + EFI_ENTRY("%d, 0x%lx, %p, %p", type, notify_tpl, notify_function, notify_context); - if (efi_event.notify_function) { - /* We only support one event at a time */ - return EFI_EXIT(EFI_OUT_OF_RESOURCES); - } - if (event == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
@@ -201,13 +187,20 @@ static efi_status_t EFIAPI efi_create_event( notify_function == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
- efi_event.type = type; - efi_event.notify_tpl = notify_tpl; - efi_event.notify_function = notify_function; - efi_event.notify_context = notify_context; - *event = &efi_event; - - return EFI_EXIT(EFI_SUCCESS); + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + if (efi_events[i].type) + continue; + efi_events[i].type = type; + efi_events[i].notify_tpl = notify_tpl; + efi_events[i].notify_function = notify_function; + efi_events[i].notify_context = notify_context; + /* Disable timers on bootup */ + efi_events[i].trigger_next = -1ULL; + efi_events[i].signaled = 0; + *event = &efi_events[i]; + return EFI_EXIT(EFI_SUCCESS); + } + return EFI_EXIT(EFI_OUT_OF_RESOURCES); }
/* @@ -216,17 +209,25 @@ static efi_status_t EFIAPI efi_create_event( */ void efi_timer_check(void) { + int i; u64 now = timer_get_us();
- if (now >= efi_event.trigger_next) { - /* Triggering! */ - if (efi_event.trigger_type == EFI_TIMER_PERIODIC) - efi_event.trigger_next += efi_event.trigger_time / 10; - if (efi_event.type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL)) - efi_event.notify_function(&efi_event, - efi_event.notify_context); + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + if (!(efi_events[i].type & EVT_TIMER) || + efi_events[i].trigger_type == EFI_TIMER_STOP || + now < efi_events[i].trigger_next) + continue; + if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC) + efi_events[i].trigger_next += + efi_events[i].trigger_time / 10; + efi_events[i].signaled = 1; + if (efi_events[i].type & EVT_NOTIFY_SIGNAL) { + EFI_EXIT(EFI_SUCCESS); + efi_events[i].notify_function(&efi_events[i], + efi_events[i].notify_context); + efi_restore_gd(); + } } - WATCHDOG_RESET(); }
@@ -236,67 +237,109 @@ static efi_status_t EFIAPI efi_set_timer(void *event, int type, /* We don't have 64bit division available everywhere, so limit timer * distances to 32bit bits. */ u32 trigger32 = trigger_time; + int i;
EFI_ENTRY("%p, %d, %"PRIx64, event, type, trigger_time);
if (trigger32 < trigger_time) { + trigger32 = 0xffffffff; printf("WARNING: Truncating timer from %"PRIx64" to %x\n", trigger_time, trigger32); }
- if (event != &efi_event) { - /* We only support one event at a time */ - return EFI_EXIT(EFI_INVALID_PARAMETER); - } + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + if (event != &efi_events[i]) + continue;
- switch (type) { - case EFI_TIMER_STOP: - efi_event.trigger_next = -1ULL; - break; - case EFI_TIMER_PERIODIC: - case EFI_TIMER_RELATIVE: - efi_event.trigger_next = timer_get_us() + (trigger32 / 10); - break; - default: - return EFI_EXIT(EFI_INVALID_PARAMETER); + switch (type) { + case EFI_TIMER_STOP: + efi_events[i].trigger_next = -1ULL; + break; + case EFI_TIMER_PERIODIC: + case EFI_TIMER_RELATIVE: + efi_events[i].trigger_next = + timer_get_us() + (trigger32 / 10); + break; + default: + return EFI_EXIT(EFI_INVALID_PARAMETER); + } + efi_events[i].trigger_type = type; + efi_events[i].trigger_time = trigger_time; + return EFI_EXIT(EFI_SUCCESS); } - efi_event.trigger_type = type; - efi_event.trigger_time = trigger_time; - - return EFI_EXIT(EFI_SUCCESS); + return EFI_EXIT(EFI_INVALID_PARAMETER); }
static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events, void *event, unsigned long *index) { - u64 now; + int i;
EFI_ENTRY("%ld, %p, %p", num_events, event, index);
- now = timer_get_us(); - while (now < efi_event.trigger_next) { } - efi_timer_check(); - + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + if (event != &efi_events[i]) + continue; + while (!efi_events[i].signaled) + efi_timer_check(); + efi_events[i].signaled = 0; + break; + } return EFI_EXIT(EFI_SUCCESS); }
static efi_status_t EFIAPI efi_signal_event(void *event) { + int i; + EFI_ENTRY("%p", event); + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + if (event != &efi_events[i]) + continue; + if (efi_events[i].signaled) + break; + efi_events[i].signaled = 1; + if (efi_events[i].type & EVT_NOTIFY_SIGNAL) { + EFI_EXIT(EFI_SUCCESS); + efi_events[i].notify_function(&efi_events[i], + efi_events[i].notify_context); + efi_restore_gd(); + } + break; + } return EFI_EXIT(EFI_SUCCESS); }
static efi_status_t EFIAPI efi_close_event(void *event) { + int i; + EFI_ENTRY("%p", event); - efi_event.trigger_next = -1ULL; + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + if (event != &efi_events[i]) + continue; + efi_events[i].type = 0; + efi_events[i].trigger_next = -1ULL; + efi_events[i].signaled = 0; + break; + } return EFI_EXIT(EFI_SUCCESS); }
static efi_status_t EFIAPI efi_check_event(void *event) { + int i; + EFI_ENTRY("%p", event); - return EFI_EXIT(EFI_NOT_READY); + efi_timer_check(); + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + if (event != &efi_events[i]) + continue; + if (efi_events[i].signaled) + return EFI_EXIT(EFI_SUCCESS); + return EFI_EXIT(EFI_NOT_READY); + } + return EFI_EXIT(EFI_INVALID_PARAMETER); }
static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
On 05.07.17 19:47, Heinrich Schuchardt wrote:
Up to now the boot time supported only a single event. This patch now allows four events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_api.h | 8 ++- include/efi_loader.h | 23 ++++++ lib/efi_loader/efi_boottime.c | 161 ++++++++++++++++++++++++++---------------- 3 files changed, 131 insertions(+), 61 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 42cd47ff08..18bef722c6 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -28,8 +28,12 @@ enum efi_event_type { EFI_TIMER_RELATIVE = 2 };
-#define EVT_NOTIFY_WAIT 0x00000100 -#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_TIMER 0x80000000 +#define EVT_RUNTIME 0x40000000 +#define EVT_NOTIFY_WAIT 0x00000100 +#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201 +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202
/* EFI Boot Services table */ struct efi_boot_services { diff --git a/include/efi_loader.h b/include/efi_loader.h index c620652307..a35b971f7e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -71,6 +71,29 @@ struct efi_object { void *handle; };
+/**
- struct efi_event
- @type: Type of event
- @trigger_type: Type of timer
- @trigger_time: Period of the timer
- @trigger_next: Next time to trigger the timer
- @nofify_function: Function to call when the event is triggered
- @notify_context: Data to be passed to the notify function
- @signaled: The notify function was already called
- */
+struct efi_event {
- u32 type;
- unsigned long notify_tpl;
- void (EFIAPI *notify_function)(void *event, void *context);
- void *notify_context;
- u64 trigger_next;
- u32 trigger_time;
- enum efi_event_type trigger_type;
- int signaled;
+};
- /* This list contains all UEFI objects we know of */ extern struct list_head efi_obj_list;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3060c25a2a..ef3e7d9d52 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -162,21 +162,10 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer) }
/*
- Our event capabilities are very limited. Only support a single
- event to exist, so we don't need to maintain lists.
- Our event capabilities are very limited. Only a small limited
*/
- number of events is allowed to coexist.
-static struct {
- enum efi_event_type type;
- u32 trigger_type;
- u32 trigger_time;
- u64 trigger_next;
- unsigned long notify_tpl;
- void (EFIAPI *notify_function) (void *event, void *context);
- void *notify_context;
-} efi_event = {
- /* Disable timers on bootup */
- .trigger_next = -1ULL,
-}; +static struct efi_event efi_events[16];
static efi_status_t EFIAPI efi_create_event( enum efi_event_type type, ulong notify_tpl, @@ -184,13 +173,10 @@ static efi_status_t EFIAPI efi_create_event( void *context), void *notify_context, void **event) {
- int i;
- EFI_ENTRY("%d, 0x%lx, %p, %p", type, notify_tpl, notify_function, notify_context);
- if (efi_event.notify_function) {
/* We only support one event at a time */return EFI_EXIT(EFI_OUT_OF_RESOURCES);- }
- if (event == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
@@ -201,13 +187,20 @@ static efi_status_t EFIAPI efi_create_event( notify_function == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
- efi_event.type = type;
- efi_event.notify_tpl = notify_tpl;
- efi_event.notify_function = notify_function;
- efi_event.notify_context = notify_context;
- *event = &efi_event;
- return EFI_EXIT(EFI_SUCCESS);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (efi_events[i].type)
Please explicitly check for EFI_TIMER_STOP here.
continue;efi_events[i].type = type;efi_events[i].notify_tpl = notify_tpl;efi_events[i].notify_function = notify_function;efi_events[i].notify_context = notify_context;/* Disable timers on bootup */efi_events[i].trigger_next = -1ULL;efi_events[i].signaled = 0;*event = &efi_events[i];return EFI_EXIT(EFI_SUCCESS);}
return EFI_EXIT(EFI_OUT_OF_RESOURCES); }
/*
@@ -216,17 +209,25 @@ static efi_status_t EFIAPI efi_create_event( */ void efi_timer_check(void) {
- int i; u64 now = timer_get_us();
- if (now >= efi_event.trigger_next) {
/* Triggering! */if (efi_event.trigger_type == EFI_TIMER_PERIODIC)efi_event.trigger_next += efi_event.trigger_time / 10;if (efi_event.type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL))efi_event.notify_function(&efi_event,efi_event.notify_context);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (!(efi_events[i].type & EVT_TIMER) ||efi_events[i].trigger_type == EFI_TIMER_STOP ||now < efi_events[i].trigger_next)continue;
This if clause is quite hard to read. I don't think it would hurt to just unfold it into individual cases? You can then also document for each why they should get ignored.
if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC)efi_events[i].trigger_next +=efi_events[i].trigger_time / 10;
I stumbled over my own code here - awesome :). Can you please put the division into a static function that tells the reader from which number space into which number space we're converting (100ns)?
efi_events[i].signaled = 1;if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {EFI_EXIT(EFI_SUCCESS);
This is quite dangerous...
efi_events[i].notify_function(&efi_events[i],efi_events[i].notify_context);efi_restore_gd();
... I see that you do restore gd but please document this heavily with a big, obvious comment and ideally even use EFI_ENTRY() again here instead of hard coding efi_restore_gd().
}
wrong indentation
}
- WATCHDOG_RESET(); }
@@ -236,67 +237,109 @@ static efi_status_t EFIAPI efi_set_timer(void *event, int type, /* We don't have 64bit division available everywhere, so limit timer * distances to 32bit bits. */ u32 trigger32 = trigger_time;
int i;
EFI_ENTRY("%p, %d, %"PRIx64, event, type, trigger_time);
if (trigger32 < trigger_time) {
trigger32 = 0xffffffff;
This change should be a separate patch I suppose. Makes bisecting things easier.
printf("WARNING: Truncating timer from %"PRIx64" to %x\n", trigger_time, trigger32);}
- if (event != &efi_event) {
/* We only support one event at a time */return EFI_EXIT(EFI_INVALID_PARAMETER);- }
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;
- switch (type) {
- case EFI_TIMER_STOP:
efi_event.trigger_next = -1ULL;break;- case EFI_TIMER_PERIODIC:
- case EFI_TIMER_RELATIVE:
efi_event.trigger_next = timer_get_us() + (trigger32 / 10);break;- default:
return EFI_EXIT(EFI_INVALID_PARAMETER);
switch (type) {case EFI_TIMER_STOP:efi_events[i].trigger_next = -1ULL;break;case EFI_TIMER_PERIODIC:case EFI_TIMER_RELATIVE:efi_events[i].trigger_next =timer_get_us() + (trigger32 / 10);break;default:return EFI_EXIT(EFI_INVALID_PARAMETER);}efi_events[i].trigger_type = type;efi_events[i].trigger_time = trigger_time;return EFI_EXIT(EFI_SUCCESS);
- efi_event.trigger_type = type;
- efi_event.trigger_time = trigger_time;
- return EFI_EXIT(EFI_SUCCESS);
return EFI_EXIT(EFI_INVALID_PARAMETER); }
static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events, void *event, unsigned long *index) {
- u64 now;
int i;
EFI_ENTRY("%ld, %p, %p", num_events, event, index);
- now = timer_get_us();
- while (now < efi_event.trigger_next) { }
- efi_timer_check();
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;
We also need to check if EVT_NOTIFY_SIGNAL is set and error out in that case, no?
while (!efi_events[i].signaled)efi_timer_check();
Ah, nice, so your new code also resets the watchdog. Very good :).
efi_events[i].signaled = 0;
Please document that line with reference to the spec.
break;} return EFI_EXIT(EFI_SUCCESS); }
static efi_status_t EFIAPI efi_signal_event(void *event) {
int i;
EFI_ENTRY("%p", event);
for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;if (efi_events[i].signaled)break;efi_events[i].signaled = 1;if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {EFI_EXIT(EFI_SUCCESS);efi_events[i].notify_function(&efi_events[i],efi_events[i].notify_context);efi_restore_gd();
This looks like code repetition. Can you fold it in with the same code above?
}break;} return EFI_EXIT(EFI_SUCCESS); }
static efi_status_t EFIAPI efi_close_event(void *event) {
int i;
EFI_ENTRY("%p", event);
- efi_event.trigger_next = -1ULL;
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;efi_events[i].type = 0;
Use STOPPED here.
efi_events[i].trigger_next = -1ULL;efi_events[i].signaled = 0;break;} return EFI_EXIT(EFI_SUCCESS); }
static efi_status_t EFIAPI efi_check_event(void *event) {
int i;
EFI_ENTRY("%p", event);
- return EFI_EXIT(EFI_NOT_READY);
- efi_timer_check();
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
Here to we need to check for EVT_NOTIFY_SIGNAL and error out, no?
Alex
if (event != &efi_events[i])continue;if (efi_events[i].signaled)return EFI_EXIT(EFI_SUCCESS);return EFI_EXIT(EFI_NOT_READY);}
return EFI_EXIT(EFI_INVALID_PARAMETER); }
static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
On 07/12/2017 12:55 PM, Alexander Graf wrote:
On 05.07.17 19:47, Heinrich Schuchardt wrote:
Up to now the boot time supported only a single event. This patch now allows four events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_api.h | 8 ++- include/efi_loader.h | 23 ++++++ lib/efi_loader/efi_boottime.c | 161 ++++++++++++++++++++++++++---------------- 3 files changed, 131 insertions(+), 61 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 42cd47ff08..18bef722c6 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -28,8 +28,12 @@ enum efi_event_type { EFI_TIMER_RELATIVE = 2 }; -#define EVT_NOTIFY_WAIT 0x00000100 -#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_TIMER 0x80000000 +#define EVT_RUNTIME 0x40000000 +#define EVT_NOTIFY_WAIT 0x00000100 +#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201 +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202 /* EFI Boot Services table */ struct efi_boot_services { diff --git a/include/efi_loader.h b/include/efi_loader.h index c620652307..a35b971f7e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -71,6 +71,29 @@ struct efi_object { void *handle; }; +/**
- struct efi_event
- @type: Type of event
- @trigger_type: Type of timer
- @trigger_time: Period of the timer
- @trigger_next: Next time to trigger the timer
- @nofify_function: Function to call when the event is triggered
- @notify_context: Data to be passed to the notify function
- @signaled: The notify function was already called
- */
+struct efi_event {
- u32 type;
- unsigned long notify_tpl;
- void (EFIAPI *notify_function)(void *event, void *context);
- void *notify_context;
- u64 trigger_next;
- u32 trigger_time;
- enum efi_event_type trigger_type;
- int signaled;
+};
- /* This list contains all UEFI objects we know of */ extern struct list_head efi_obj_list; diff --git a/lib/efi_loader/efi_boottime.c
b/lib/efi_loader/efi_boottime.c index 3060c25a2a..ef3e7d9d52 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -162,21 +162,10 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer) } /*
- Our event capabilities are very limited. Only support a single
- event to exist, so we don't need to maintain lists.
- Our event capabilities are very limited. Only a small limited
*/
- number of events is allowed to coexist.
-static struct {
- enum efi_event_type type;
- u32 trigger_type;
- u32 trigger_time;
- u64 trigger_next;
- unsigned long notify_tpl;
- void (EFIAPI *notify_function) (void *event, void *context);
- void *notify_context;
-} efi_event = {
- /* Disable timers on bootup */
- .trigger_next = -1ULL,
-}; +static struct efi_event efi_events[16]; static efi_status_t EFIAPI efi_create_event( enum efi_event_type type, ulong notify_tpl, @@ -184,13 +173,10 @@ static efi_status_t EFIAPI efi_create_event( void *context), void *notify_context, void **event) {
- int i;
EFI_ENTRY("%d, 0x%lx, %p, %p", type, notify_tpl, notify_function, notify_context);
- if (efi_event.notify_function) {
/* We only support one event at a time */return EFI_EXIT(EFI_OUT_OF_RESOURCES);- }
if (event == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
- efi_event.type = type;
- efi_event.notify_tpl = notify_tpl;
- efi_event.notify_function = notify_function;
- efi_event.notify_context = notify_context;
- *event = &efi_event;
- return EFI_EXIT(EFI_SUCCESS);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (efi_events[i].type)Please explicitly check for EFI_TIMER_STOP here.
TimerCancel (alias EFI_TIMER_STOP) is set in TriggerTimer. If the status of a timer is set to TimerCancel this does not imply that the event can be deleted (i.e. the slot can be reused).
The owner of the event might decide to call TriggerTimer for the same event at a later time with TimerPeriodic or TimerRelative.
So I do not understand why I should check for EFI_TIMER_STOP here.
continue;efi_events[i].type = type;efi_events[i].notify_tpl = notify_tpl;efi_events[i].notify_function = notify_function;efi_events[i].notify_context = notify_context;/* Disable timers on bootup */efi_events[i].trigger_next = -1ULL;efi_events[i].signaled = 0;*event = &efi_events[i];return EFI_EXIT(EFI_SUCCESS);- }
- return EFI_EXIT(EFI_OUT_OF_RESOURCES); } /*
@@ -216,17 +209,25 @@ static efi_status_t EFIAPI efi_create_event( */ void efi_timer_check(void) {
- int i; u64 now = timer_get_us();
- if (now >= efi_event.trigger_next) {
/* Triggering! */if (efi_event.trigger_type == EFI_TIMER_PERIODIC)efi_event.trigger_next += efi_event.trigger_time / 10;if (efi_event.type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL))efi_event.notify_function(&efi_event,efi_event.notify_context);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (!(efi_events[i].type & EVT_TIMER) ||efi_events[i].trigger_type == EFI_TIMER_STOP ||now < efi_events[i].trigger_next)continue;This if clause is quite hard to read. I don't think it would hurt to just unfold it into individual cases? You can then also document for each why they should get ignored.
if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC)efi_events[i].trigger_next +=efi_events[i].trigger_time / 10;I stumbled over my own code here - awesome :). Can you please put the division into a static function that tells the reader from which number space into which number space we're converting (100ns)?
efi_events[i].signaled = 1;if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {EFI_EXIT(EFI_SUCCESS);This is quite dangerous...
What do you mean by "quite dangerous"?
We have to call the EFI application here. Thus we have to switch the gd context.
efi_events[i].notify_function(&efi_events[i],efi_events[i].notify_context);efi_restore_gd();... I see that you do restore gd but please document this heavily with a big, obvious comment and ideally even use EFI_ENTRY() again here instead of hard coding efi_restore_gd().
I used efi_restore_gd() to see less messages in debug mode.
In the reply to another patch you suggested to provide a configurable verbosity level for EFI_ENTRY. So we can use EFI_ENTRY here.
}wrong indentation
ok
}
WATCHDOG_RESET();*event, int type, /* We don't have 64bit division available everywhere, so limit timer * distances to 32bit bits. */ u32 trigger32 = trigger_time;
- int i; EFI_ENTRY("%p, %d, %"PRIx64, event, type, trigger_time); if (trigger32 < trigger_time) {
trigger32 = 0xffffffff;This change should be a separate patch I suppose. Makes bisecting things easier.
ok
printf("WARNING: Truncating timer from %"PRIx64" to %x\n", trigger_time, trigger32); }
- if (event != &efi_event) {
/* We only support one event at a time */return EFI_EXIT(EFI_INVALID_PARAMETER);- }
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;
- switch (type) {
- case EFI_TIMER_STOP:
efi_event.trigger_next = -1ULL;break;- case EFI_TIMER_PERIODIC:
- case EFI_TIMER_RELATIVE:
efi_event.trigger_next = timer_get_us() + (trigger32 / 10);break;- default:
return EFI_EXIT(EFI_INVALID_PARAMETER);
switch (type) {case EFI_TIMER_STOP:efi_events[i].trigger_next = -1ULL;break;case EFI_TIMER_PERIODIC:case EFI_TIMER_RELATIVE:efi_events[i].trigger_next =timer_get_us() + (trigger32 / 10);break;default:return EFI_EXIT(EFI_INVALID_PARAMETER);}efi_events[i].trigger_type = type;efi_events[i].trigger_time = trigger_time;return EFI_EXIT(EFI_SUCCESS); }
- efi_event.trigger_type = type;
- efi_event.trigger_time = trigger_time;
- return EFI_EXIT(EFI_SUCCESS);
- return EFI_EXIT(EFI_INVALID_PARAMETER); } static efi_status_t EFIAPI efi_wait_for_event(unsigned long
num_events, void *event, unsigned long *index) {
- u64 now;
- int i; EFI_ENTRY("%ld, %p, %p", num_events, event, index);
- now = timer_get_us();
- while (now < efi_event.trigger_next) { }
- efi_timer_check();
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;We also need to check if EVT_NOTIFY_SIGNAL is set and error out in that case, no?
Yes according to the UEFI spec.
while (!efi_events[i].signaled)efi_timer_check();Ah, nice, so your new code also resets the watchdog. Very good :).
efi_events[i].signaled = 0;Please document that line with reference to the spec.
ok
break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_signal_event(void *event) {
- int i;
EFI_ENTRY("%p", event);- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;if (efi_events[i].signaled)break;efi_events[i].signaled = 1;if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {EFI_EXIT(EFI_SUCCESS);efi_events[i].notify_function(&efi_events[i],efi_events[i].notify_context);efi_restore_gd();This looks like code repetition. Can you fold it in with the same code above?
ok
}break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_close_event(void *event) {
- int i;
EFI_ENTRY("%p", event);
- efi_event.trigger_next = -1ULL;
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;efi_events[i].type = 0;Use STOPPED here.
This is not the trigger_type field (where we could set EFI_TIMER_STOP).
efi_events[i].trigger_next = -1ULL;efi_events[i].signaled = 0;break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_check_event(void *event) {
- int i;
EFI_ENTRY("%p", event);
- return EFI_EXIT(EFI_NOT_READY);
- efi_timer_check();
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
Here to we need to check for EVT_NOTIFY_SIGNAL and error out, no?
No. You may want to check if the event available even if you could wait for it.
E.g. the application sets up an event for console input. If there is some other work to do, just poll the event with CheckEvent. If all work is done, use WaitForEvent.
Thanks for reviewing
Regards
Heinrich
if (event != &efi_events[i])continue;if (efi_events[i].signaled)return EFI_EXIT(EFI_SUCCESS);return EFI_EXIT(EFI_NOT_READY);- }
- return EFI_EXIT(EFI_INVALID_PARAMETER); } static efi_status_t EFIAPI efi_install_protocol_interface(void
**handle,
On 15.07.17 13:43, Heinrich Schuchardt wrote:
On 07/12/2017 12:55 PM, Alexander Graf wrote:
On 05.07.17 19:47, Heinrich Schuchardt wrote:
Up to now the boot time supported only a single event. This patch now allows four events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_api.h | 8 ++- include/efi_loader.h | 23 ++++++ lib/efi_loader/efi_boottime.c | 161 ++++++++++++++++++++++++++---------------- 3 files changed, 131 insertions(+), 61 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 42cd47ff08..18bef722c6 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -28,8 +28,12 @@ enum efi_event_type { EFI_TIMER_RELATIVE = 2 }; -#define EVT_NOTIFY_WAIT 0x00000100 -#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_TIMER 0x80000000 +#define EVT_RUNTIME 0x40000000 +#define EVT_NOTIFY_WAIT 0x00000100 +#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201 +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202 /* EFI Boot Services table */ struct efi_boot_services { diff --git a/include/efi_loader.h b/include/efi_loader.h index c620652307..a35b971f7e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -71,6 +71,29 @@ struct efi_object { void *handle; }; +/**
- struct efi_event
- @type: Type of event
- @trigger_type: Type of timer
- @trigger_time: Period of the timer
- @trigger_next: Next time to trigger the timer
- @nofify_function: Function to call when the event is triggered
- @notify_context: Data to be passed to the notify function
- @signaled: The notify function was already called
- */
+struct efi_event {
- u32 type;
- unsigned long notify_tpl;
- void (EFIAPI *notify_function)(void *event, void *context);
- void *notify_context;
- u64 trigger_next;
- u32 trigger_time;
- enum efi_event_type trigger_type;
- int signaled;
+};
- /* This list contains all UEFI objects we know of */ extern struct list_head efi_obj_list; diff --git a/lib/efi_loader/efi_boottime.c
b/lib/efi_loader/efi_boottime.c index 3060c25a2a..ef3e7d9d52 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -162,21 +162,10 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer) } /*
- Our event capabilities are very limited. Only support a single
- event to exist, so we don't need to maintain lists.
- Our event capabilities are very limited. Only a small limited
- number of events is allowed to coexist. */
-static struct {
- enum efi_event_type type;
- u32 trigger_type;
- u32 trigger_time;
- u64 trigger_next;
- unsigned long notify_tpl;
- void (EFIAPI *notify_function) (void *event, void *context);
- void *notify_context;
-} efi_event = {
- /* Disable timers on bootup */
- .trigger_next = -1ULL,
-}; +static struct efi_event efi_events[16]; static efi_status_t EFIAPI efi_create_event( enum efi_event_type type, ulong notify_tpl, @@ -184,13 +173,10 @@ static efi_status_t EFIAPI efi_create_event( void *context), void *notify_context, void **event) {
- int i;
EFI_ENTRY("%d, 0x%lx, %p, %p", type, notify_tpl, notify_function, notify_context);
- if (efi_event.notify_function) {
/* We only support one event at a time */return EFI_EXIT(EFI_OUT_OF_RESOURCES);- }
if (event == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
- efi_event.type = type;
- efi_event.notify_tpl = notify_tpl;
- efi_event.notify_function = notify_function;
- efi_event.notify_context = notify_context;
- *event = &efi_event;
- return EFI_EXIT(EFI_SUCCESS);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (efi_events[i].type)Please explicitly check for EFI_TIMER_STOP here.
TimerCancel (alias EFI_TIMER_STOP) is set in TriggerTimer. If the status of a timer is set to TimerCancel this does not imply that the event can be deleted (i.e. the slot can be reused).
The owner of the event might decide to call TriggerTimer for the same event at a later time with TimerPeriodic or TimerRelative.
So I do not understand why I should check for EFI_TIMER_STOP here.
Simply because you're checking for it already, but implicitly:
enum efi_event_type { EFI_TIMER_STOP = 0, EFI_TIMER_PERIODIC = 1, EFI_TIMER_RELATIVE = 2 };
and to avoid confusion, I prefer to call out enums when we match them. Otherwise someone will think that this check will include EFI_TIMER_STOP one day ;).
continue;efi_events[i].type = type;efi_events[i].notify_tpl = notify_tpl;efi_events[i].notify_function = notify_function;efi_events[i].notify_context = notify_context;/* Disable timers on bootup */efi_events[i].trigger_next = -1ULL;efi_events[i].signaled = 0;*event = &efi_events[i];return EFI_EXIT(EFI_SUCCESS);- }
- return EFI_EXIT(EFI_OUT_OF_RESOURCES); } /*
@@ -216,17 +209,25 @@ static efi_status_t EFIAPI efi_create_event( */ void efi_timer_check(void) {
- int i; u64 now = timer_get_us();
- if (now >= efi_event.trigger_next) {
/* Triggering! */if (efi_event.trigger_type == EFI_TIMER_PERIODIC)efi_event.trigger_next += efi_event.trigger_time / 10;if (efi_event.type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL))efi_event.notify_function(&efi_event,efi_event.notify_context);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (!(efi_events[i].type & EVT_TIMER) ||efi_events[i].trigger_type == EFI_TIMER_STOP ||now < efi_events[i].trigger_next)continue;This if clause is quite hard to read. I don't think it would hurt to just unfold it into individual cases? You can then also document for each why they should get ignored.
if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC)efi_events[i].trigger_next +=efi_events[i].trigger_time / 10;I stumbled over my own code here - awesome :). Can you please put the division into a static function that tells the reader from which number space into which number space we're converting (100ns)?
efi_events[i].signaled = 1;if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {EFI_EXIT(EFI_SUCCESS);This is quite dangerous...
What do you mean by "quite dangerous"?
We have to call the EFI application here. Thus we have to switch the gd context.
Yes, but switching context is always tricky :).
efi_events[i].notify_function(&efi_events[i],efi_events[i].notify_context);efi_restore_gd();... I see that you do restore gd but please document this heavily with a big, obvious comment and ideally even use EFI_ENTRY() again here instead of hard coding efi_restore_gd().
I used efi_restore_gd() to see less messages in debug mode.
In the reply to another patch you suggested to provide a configurable verbosity level for EFI_ENTRY. So we can use EFI_ENTRY here.
That works too, but for clarity it's usually much nicer to have a full function start with ENTRY and end with EXIT. That way things become more obvious. I've wasted way too many hours on debugging ENTRY/EXIT misplacement.
One thing that would already help readability would be to just move the whole exit/entry dance into a separate function. The compiler will inline it, so the compiled code should be the same. But at least for the reader it's obvious what happens:
static void efi_call_notify(...) { /* * Description about the necessity of this function */
EFI_EXIT();
notifier(...);
/* Go back to U-Boot space */ EFI_ENTRY(); }
}wrong indentation
ok
}
WATCHDOG_RESET();*event, int type, /* We don't have 64bit division available everywhere, so limit timer * distances to 32bit bits. */ u32 trigger32 = trigger_time;
- int i; EFI_ENTRY("%p, %d, %"PRIx64, event, type, trigger_time); if (trigger32 < trigger_time) {
trigger32 = 0xffffffff;This change should be a separate patch I suppose. Makes bisecting things easier.
ok
printf("WARNING: Truncating timer from %"PRIx64" to %x\n", trigger_time, trigger32); }
- if (event != &efi_event) {
/* We only support one event at a time */return EFI_EXIT(EFI_INVALID_PARAMETER);- }
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;
- switch (type) {
- case EFI_TIMER_STOP:
efi_event.trigger_next = -1ULL;break;- case EFI_TIMER_PERIODIC:
- case EFI_TIMER_RELATIVE:
efi_event.trigger_next = timer_get_us() + (trigger32 / 10);break;- default:
return EFI_EXIT(EFI_INVALID_PARAMETER);
switch (type) {case EFI_TIMER_STOP:efi_events[i].trigger_next = -1ULL;break;case EFI_TIMER_PERIODIC:case EFI_TIMER_RELATIVE:efi_events[i].trigger_next =timer_get_us() + (trigger32 / 10);break;default:return EFI_EXIT(EFI_INVALID_PARAMETER);}efi_events[i].trigger_type = type;efi_events[i].trigger_time = trigger_time;return EFI_EXIT(EFI_SUCCESS); }
- efi_event.trigger_type = type;
- efi_event.trigger_time = trigger_time;
- return EFI_EXIT(EFI_SUCCESS);
- return EFI_EXIT(EFI_INVALID_PARAMETER); } static efi_status_t EFIAPI efi_wait_for_event(unsigned long
num_events, void *event, unsigned long *index) {
- u64 now;
- int i; EFI_ENTRY("%ld, %p, %p", num_events, event, index);
- now = timer_get_us();
- while (now < efi_event.trigger_next) { }
- efi_timer_check();
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;We also need to check if EVT_NOTIFY_SIGNAL is set and error out in that case, no?
Yes according to the UEFI spec.
while (!efi_events[i].signaled)efi_timer_check();Ah, nice, so your new code also resets the watchdog. Very good :).
efi_events[i].signaled = 0;Please document that line with reference to the spec.
ok
break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_signal_event(void *event) {
- int i;
EFI_ENTRY("%p", event);- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;if (efi_events[i].signaled)break;efi_events[i].signaled = 1;if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {EFI_EXIT(EFI_SUCCESS);efi_events[i].notify_function(&efi_events[i],efi_events[i].notify_context);efi_restore_gd();This looks like code repetition. Can you fold it in with the same code above?
ok
}break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_close_event(void *event) {
- int i;
EFI_ENTRY("%p", event);
- efi_event.trigger_next = -1ULL;
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;efi_events[i].type = 0;Use STOPPED here.
This is not the trigger_type field (where we could set EFI_TIMER_STOP).
Ok, I'm confused. In efi_create_event() you set efi_events[i] to the parameter "type" which is enum efi_event_type, no?
So what are the semantics of the "type" field vs "trigger_type"?
To clarify that, please make the "type" field its own enum and double check that it only gets assigned with values that match that enum.
efi_events[i].trigger_next = -1ULL;efi_events[i].signaled = 0;break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_check_event(void *event) {
- int i;
EFI_ENTRY("%p", event);
- return EFI_EXIT(EFI_NOT_READY);
- efi_timer_check();
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
Here to we need to check for EVT_NOTIFY_SIGNAL and error out, no?
No. You may want to check if the event available even if you could wait for it.
E.g. the application sets up an event for console input. If there is some other work to do, just poll the event with CheckEvent. If all work is done, use WaitForEvent.
Well, I agree, but the spec explicitly states that CheckEvent() should error out if it sees EVT_NOTIFY_SIGNAL ;).
Alex
On 07/16/2017 09:25 AM, Alexander Graf wrote:
On 15.07.17 13:43, Heinrich Schuchardt wrote:
On 07/12/2017 12:55 PM, Alexander Graf wrote:
On 05.07.17 19:47, Heinrich Schuchardt wrote:
Up to now the boot time supported only a single event. This patch now allows four events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_api.h | 8 ++- include/efi_loader.h | 23 ++++++ lib/efi_loader/efi_boottime.c | 161 ++++++++++++++++++++++++++---------------- 3 files changed, 131 insertions(+), 61 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 42cd47ff08..18bef722c6 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -28,8 +28,12 @@ enum efi_event_type { EFI_TIMER_RELATIVE = 2 }; -#define EVT_NOTIFY_WAIT 0x00000100 -#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_TIMER 0x80000000 +#define EVT_RUNTIME 0x40000000 +#define EVT_NOTIFY_WAIT 0x00000100 +#define EVT_NOTIFY_SIGNAL 0x00000200 +#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201 +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202 /* EFI Boot Services table */ struct efi_boot_services { diff --git a/include/efi_loader.h b/include/efi_loader.h index c620652307..a35b971f7e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -71,6 +71,29 @@ struct efi_object { void *handle; }; +/**
- struct efi_event
- @type: Type of event
- @trigger_type: Type of timer
- @trigger_time: Period of the timer
- @trigger_next: Next time to trigger the timer
- @nofify_function: Function to call when the event is triggered
- @notify_context: Data to be passed to the notify function
- @signaled: The notify function was already called
- */
+struct efi_event {
- u32 type;
- unsigned long notify_tpl;
- void (EFIAPI *notify_function)(void *event, void *context);
- void *notify_context;
- u64 trigger_next;
- u32 trigger_time;
- enum efi_event_type trigger_type;
- int signaled;
+};
- /* This list contains all UEFI objects we know of */ extern struct list_head efi_obj_list; diff --git a/lib/efi_loader/efi_boottime.c
b/lib/efi_loader/efi_boottime.c index 3060c25a2a..ef3e7d9d52 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -162,21 +162,10 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer) } /*
- Our event capabilities are very limited. Only support a single
- event to exist, so we don't need to maintain lists.
- Our event capabilities are very limited. Only a small limited
- number of events is allowed to coexist. */
-static struct {
- enum efi_event_type type;
- u32 trigger_type;
- u32 trigger_time;
- u64 trigger_next;
- unsigned long notify_tpl;
- void (EFIAPI *notify_function) (void *event, void *context);
- void *notify_context;
-} efi_event = {
- /* Disable timers on bootup */
- .trigger_next = -1ULL,
-}; +static struct efi_event efi_events[16]; static efi_status_t EFIAPI efi_create_event( enum efi_event_type type, ulong notify_tpl, @@ -184,13 +173,10 @@ static efi_status_t EFIAPI efi_create_event( void *context), void *notify_context, void **event) {
- int i;
EFI_ENTRY("%d, 0x%lx, %p, %p", type, notify_tpl,notify_function, notify_context);
- if (efi_event.notify_function) {
/* We only support one event at a time */return EFI_EXIT(EFI_OUT_OF_RESOURCES);- }
if (event == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
- efi_event.type = type;
- efi_event.notify_tpl = notify_tpl;
- efi_event.notify_function = notify_function;
- efi_event.notify_context = notify_context;
- *event = &efi_event;
- return EFI_EXIT(EFI_SUCCESS);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (efi_events[i].type)Please explicitly check for EFI_TIMER_STOP here.
TimerCancel (alias EFI_TIMER_STOP) is set in TriggerTimer. If the status of a timer is set to TimerCancel this does not imply that the event can be deleted (i.e. the slot can be reused).
The owner of the event might decide to call TriggerTimer for the same event at a later time with TimerPeriodic or TimerRelative.
So I do not understand why I should check for EFI_TIMER_STOP here.
Simply because you're checking for it already, but implicitly:
enum efi_event_type { EFI_TIMER_STOP = 0, EFI_TIMER_PERIODIC = 1, EFI_TIMER_RELATIVE = 2 };
and to avoid confusion, I prefer to call out enums when we match them. Otherwise someone will think that this check will include EFI_TIMER_STOP one day ;).
We are talking about this structure after applying upconfing v2 of the patch:
/** * struct efi_event * * @type: Type of event provided by CreateEvent * @notify_tpl: Task priority level of notifications * @trigger_time: Period of the timer * @trigger_next: Next time to trigger the timer * @nofify_function: Function to call when the event is triggered * @notify_context: Data to be passed to the notify function * @trigger_type: Type of timer provided by SetTimer * @signaled: The notify function was already called */ struct efi_event { u32 type; unsigned long notify_tpl; void (EFIAPI *notify_function)(struct efi_event *event, void *context); void *notify_context; u64 trigger_next; u32 trigger_time; enum efi_event_type trigger_type; int signaled; };
Field type is a bitmask containing: EVT_TIMER, EVT_RUNTIME, EVT_NOTIFY_WAIT, EVT_NOTIFY_SIGNAL, EVT_SIGNAL_EXIT_BOOT_SERVICES, EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE
Type is set in efi_create_event(). Type is zero if the event slot is empty. Type is non-zero if the event slot is used.
Field trigger_type contains either of EFI_TIMER_STOP, EFI_TIMER_PERIODIC, or EFI_TIMER_RELATIVE.
Trigger_type is set in efi_set_timer().
continue;efi_events[i].type = type;efi_events[i].notify_tpl = notify_tpl;efi_events[i].notify_function = notify_function;efi_events[i].notify_context = notify_context;/* Disable timers on bootup */efi_events[i].trigger_next = -1ULL;efi_events[i].signaled = 0;*event = &efi_events[i];return EFI_EXIT(EFI_SUCCESS);- }
- return EFI_EXIT(EFI_OUT_OF_RESOURCES); } /*
@@ -216,17 +209,25 @@ static efi_status_t EFIAPI efi_create_event( */ void efi_timer_check(void) {
- int i; u64 now = timer_get_us();
- if (now >= efi_event.trigger_next) {
/* Triggering! */if (efi_event.trigger_type == EFI_TIMER_PERIODIC)efi_event.trigger_next += efi_event.trigger_time / 10;if (efi_event.type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL))efi_event.notify_function(&efi_event,efi_event.notify_context);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (!(efi_events[i].type & EVT_TIMER) ||efi_events[i].trigger_type == EFI_TIMER_STOP ||now < efi_events[i].trigger_next)continue;This if clause is quite hard to read. I don't think it would hurt to just unfold it into individual cases? You can then also document for each why they should get ignored.
if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC)efi_events[i].trigger_next +=efi_events[i].trigger_time / 10;I stumbled over my own code here - awesome :). Can you please put the division into a static function that tells the reader from which number space into which number space we're converting (100ns)?
efi_events[i].signaled = 1;if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {EFI_EXIT(EFI_SUCCESS);This is quite dangerous...
What do you mean by "quite dangerous"?
We have to call the EFI application here. Thus we have to switch the gd context.
Yes, but switching context is always tricky :).
efi_events[i].notify_function(&efi_events[i],efi_events[i].notify_context);efi_restore_gd();... I see that you do restore gd but please document this heavily with a big, obvious comment and ideally even use EFI_ENTRY() again here instead of hard coding efi_restore_gd().
I used efi_restore_gd() to see less messages in debug mode.
In the reply to another patch you suggested to provide a configurable verbosity level for EFI_ENTRY. So we can use EFI_ENTRY here.
That works too, but for clarity it's usually much nicer to have a full function start with ENTRY and end with EXIT. That way things become more obvious. I've wasted way too many hours on debugging ENTRY/EXIT misplacement.
One thing that would already help readability would be to just move the whole exit/entry dance into a separate function. The compiler will inline it, so the compiled code should be the same. But at least for the reader it's obvious what happens:
static void efi_call_notify(...) { /* * Description about the necessity of this function */
EFI_EXIT(); notifier(...); /* Go back to U-Boot space */ EFI_ENTRY();}
Will be done in v2 of the patch.
}wrong indentation
ok
}
WATCHDOG_RESET();*event, int type, /* We don't have 64bit division available everywhere, so limit timer * distances to 32bit bits. */ u32 trigger32 = trigger_time;
- int i; EFI_ENTRY("%p, %d, %"PRIx64, event, type, trigger_time); if (trigger32 < trigger_time) {
trigger32 = 0xffffffff;This change should be a separate patch I suppose. Makes bisecting things easier.
ok
printf("WARNING: Truncating timer from %"PRIx64" to %x\n", trigger_time, trigger32); }
- if (event != &efi_event) {
/* We only support one event at a time */return EFI_EXIT(EFI_INVALID_PARAMETER);- }
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;
- switch (type) {
- case EFI_TIMER_STOP:
efi_event.trigger_next = -1ULL;break;- case EFI_TIMER_PERIODIC:
- case EFI_TIMER_RELATIVE:
efi_event.trigger_next = timer_get_us() + (trigger32 / 10);break;- default:
return EFI_EXIT(EFI_INVALID_PARAMETER);
switch (type) {case EFI_TIMER_STOP:efi_events[i].trigger_next = -1ULL;break;case EFI_TIMER_PERIODIC:case EFI_TIMER_RELATIVE:efi_events[i].trigger_next =timer_get_us() + (trigger32 / 10);break;default:return EFI_EXIT(EFI_INVALID_PARAMETER);}efi_events[i].trigger_type = type;efi_events[i].trigger_time = trigger_time;return EFI_EXIT(EFI_SUCCESS); }
- efi_event.trigger_type = type;
- efi_event.trigger_time = trigger_time;
- return EFI_EXIT(EFI_SUCCESS);
- return EFI_EXIT(EFI_INVALID_PARAMETER); } static efi_status_t EFIAPI efi_wait_for_event(unsigned long
num_events, void *event, unsigned long *index) {
- u64 now;
- int i; EFI_ENTRY("%ld, %p, %p", num_events, event, index);
- now = timer_get_us();
- while (now < efi_event.trigger_next) { }
- efi_timer_check();
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;We also need to check if EVT_NOTIFY_SIGNAL is set and error out in that case, no?
Yes according to the UEFI spec.
while (!efi_events[i].signaled)efi_timer_check();Ah, nice, so your new code also resets the watchdog. Very good :).
efi_events[i].signaled = 0;Please document that line with reference to the spec.
ok
break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_signal_event(void *event) {
- int i;
EFI_ENTRY("%p", event);- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;if (efi_events[i].signaled)break;efi_events[i].signaled = 1;if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {EFI_EXIT(EFI_SUCCESS);efi_events[i].notify_function(&efi_events[i],efi_events[i].notify_context);efi_restore_gd();This looks like code repetition. Can you fold it in with the same code above?
ok
}break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_close_event(void *event) {
- int i;
EFI_ENTRY("%p", event);
- efi_event.trigger_next = -1ULL;
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
if (event != &efi_events[i])continue;efi_events[i].type = 0;Use STOPPED here.
This is not the trigger_type field (where we could set EFI_TIMER_STOP).
Ok, I'm confused. In efi_create_event() you set efi_events[i] to the parameter "type" which is enum efi_event_type, no?
So what are the semantics of the "type" field vs "trigger_type"?
To clarify that, please make the "type" field its own enum and double check that it only gets assigned with values that match that enum.
No type is not an enum but a bitmask. See above.
Regards
Heinrich
efi_events[i].trigger_next = -1ULL;efi_events[i].signaled = 0;break;- } return EFI_EXIT(EFI_SUCCESS); } static efi_status_t EFIAPI efi_check_event(void *event) {
- int i;
EFI_ENTRY("%p", event);
- return EFI_EXIT(EFI_NOT_READY);
- efi_timer_check();
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
Here to we need to check for EVT_NOTIFY_SIGNAL and error out, no?
No. You may want to check if the event available even if you could wait for it.
E.g. the application sets up an event for console input. If there is some other work to do, just poll the event with CheckEvent. If all work is done, use WaitForEvent.
Well, I agree, but the spec explicitly states that CheckEvent() should error out if it sees EVT_NOTIFY_SIGNAL ;).
Alex
The EFI_SIMPLE_TEXT_INPUT_PROTOCOL requires an event WaitForKey.
We can easily signal the event in the efi_timer_check function.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- include/efi_loader.h | 5 ++++- lib/efi_loader/efi_boottime.c | 17 ++++++++++++++--- lib/efi_loader/efi_console.c | 2 +- 3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index a35b971f7e..d7344d54e4 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -92,7 +92,10 @@ struct efi_event { enum efi_event_type trigger_type; int signaled; }; - +extern struct efi_event efi_events[]; +#define WAIT_FOR_KEY_EVENT (&efi_events[0]) +/* Events below this number cannot be closed */ +#define FIRST_EDITABLE_EVENT 1
/* This list contains all UEFI objects we know of */ extern struct list_head efi_obj_list; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index ef3e7d9d52..f509d457a7 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -165,7 +165,13 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer) * Our event capabilities are very limited. Only a small limited * number of events is allowed to coexist. */ -static struct efi_event efi_events[16]; +struct efi_event efi_events[16] = { + { + /* WaitForKey */ + .type = EVT_NOTIFY_WAIT, + .trigger_next = -1ULL, + } +};
static efi_status_t EFIAPI efi_create_event( enum efi_event_type type, ulong notify_tpl, @@ -187,7 +193,8 @@ static efi_status_t EFIAPI efi_create_event( notify_function == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + /* Use first empty slot */ + for (i = FIRST_EDITABLE_EVENT; i < ARRAY_SIZE(efi_events); ++i) { if (efi_events[i].type) continue; efi_events[i].type = type; @@ -212,6 +219,10 @@ void efi_timer_check(void) int i; u64 now = timer_get_us();
+ /* Signal keystroke */ + if (tstc()) + efi_events[0].signaled = 1; + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { if (!(efi_events[i].type & EVT_TIMER) || efi_events[i].trigger_type == EFI_TIMER_STOP || @@ -315,7 +326,7 @@ static efi_status_t EFIAPI efi_close_event(void *event) int i;
EFI_ENTRY("%p", event); - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { + for (i = FIRST_EDITABLE_EVENT; i < ARRAY_SIZE(efi_events); ++i) { if (event != &efi_events[i]) continue; efi_events[i].type = 0; diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index 8ef7326fef..b2d9ff5497 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -424,5 +424,5 @@ static efi_status_t EFIAPI efi_cin_read_key_stroke( const struct efi_simple_input_interface efi_con_in = { .reset = efi_cin_reset, .read_key_stroke = efi_cin_read_key_stroke, - .wait_for_key = NULL, + .wait_for_key = WAIT_FOR_KEY_EVENT, };
On 05.07.17 19:47, Heinrich Schuchardt wrote:
The EFI_SIMPLE_TEXT_INPUT_PROTOCOL requires an event WaitForKey.
We can easily signal the event in the efi_timer_check function.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
include/efi_loader.h | 5 ++++- lib/efi_loader/efi_boottime.c | 17 ++++++++++++++--- lib/efi_loader/efi_console.c | 2 +- 3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index a35b971f7e..d7344d54e4 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -92,7 +92,10 @@ struct efi_event { enum efi_event_type trigger_type; int signaled; };
+extern struct efi_event efi_events[]; +#define WAIT_FOR_KEY_EVENT (&efi_events[0]) +/* Events below this number cannot be closed */ +#define FIRST_EDITABLE_EVENT 1
/* This list contains all UEFI objects we know of */ extern struct list_head efi_obj_list; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index ef3e7d9d52..f509d457a7 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -165,7 +165,13 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
- Our event capabilities are very limited. Only a small limited
- number of events is allowed to coexist.
*/ -static struct efi_event efi_events[16]; +struct efi_event efi_events[16] = {
- {
/* WaitForKey */.type = EVT_NOTIFY_WAIT,.trigger_next = -1ULL,- }
+};
This moves quite a bit of data from bss to rodata. Can you initialize it with a function call instead?
static efi_status_t EFIAPI efi_create_event( enum efi_event_type type, ulong notify_tpl, @@ -187,7 +193,8 @@ static efi_status_t EFIAPI efi_create_event( notify_function == NULL) return EFI_EXIT(EFI_INVALID_PARAMETER);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
- /* Use first empty slot */
- for (i = FIRST_EDITABLE_EVENT; i < ARRAY_SIZE(efi_events); ++i) {
Do we really need to have FIRST_EDITABLE_EVENT here? We ignore events with a type already set anyway. Sure, we waste a few cycles, but the code would be more consistent if we always looped through all events.
if (efi_events[i].type) continue; efi_events[i].type = type;@@ -212,6 +219,10 @@ void efi_timer_check(void) int i; u64 now = timer_get_us();
- /* Signal keystroke */
- if (tstc())
efi_events[0].signaled = 1;
Can we somehow contain all callers of tstc() into efi_console? Maybe register the event from efi_console as a periodic event that always triggers?
Alex
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { if (!(efi_events[i].type & EVT_TIMER) || efi_events[i].trigger_type == EFI_TIMER_STOP ||
@@ -315,7 +326,7 @@ static efi_status_t EFIAPI efi_close_event(void *event) int i;
EFI_ENTRY("%p", event);
- for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
- for (i = FIRST_EDITABLE_EVENT; i < ARRAY_SIZE(efi_events); ++i) { if (event != &efi_events[i]) continue; efi_events[i].type = 0;
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index 8ef7326fef..b2d9ff5497 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -424,5 +424,5 @@ static efi_status_t EFIAPI efi_cin_read_key_stroke( const struct efi_simple_input_interface efi_con_in = { .reset = efi_cin_reset, .read_key_stroke = efi_cin_read_key_stroke,
- .wait_for_key = NULL,
- .wait_for_key = WAIT_FOR_KEY_EVENT, };
If efi_check_event is called in a loop waiting for keyboard input the screen is flooded with debug messages.
So only write debug message for other events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- lib/efi_loader/efi_boottime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index f509d457a7..ec29143306 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -341,7 +341,12 @@ static efi_status_t EFIAPI efi_check_event(void *event) { int i;
- EFI_ENTRY("%p", event); + /* Dont spam us on wait for key */ + if (event == &efi_events[0]) + efi_restore_gd(); + else + EFI_ENTRY("%p", event); + efi_timer_check(); for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { if (event != &efi_events[i])
On 05.07.17 19:47, Heinrich Schuchardt wrote:
If efi_check_event is called in a loop waiting for keyboard input the screen is flooded with debug messages.
So only write debug message for other events.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Hm, I don't really like that patch :). Is there no other way? Maybe we should add a verbosity level to EFI_ENTRY()?
Alex
lib/efi_loader/efi_boottime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index f509d457a7..ec29143306 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -341,7 +341,12 @@ static efi_status_t EFIAPI efi_check_event(void *event) { int i;
- EFI_ENTRY("%p", event);
- /* Dont spam us on wait for key */
- if (event == &efi_events[0])
efi_restore_gd();- else
EFI_ENTRY("%p", event);- efi_timer_check(); for (i = 0; i < ARRAY_SIZE(efi_events); ++i) { if (event != &efi_events[i])
participants (2)
-
Alexander Graf -
Heinrich Schuchardt