[U-Boot] [PATCH 1/3] efi_loader: implement multiple event support
Heinrich Schuchardt
xypron.glpk at gmx.de
Sat Jul 15 11:43:54 UTC 2017
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 at 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.
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();
>> }
>> @@ -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.
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,
>>
>
More information about the U-Boot
mailing list