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