[U-Boot] efi_loader: fix events
Rob Clark
robdclark at gmail.com
Thu Sep 14 21:14:06 UTC 2017
On Thu, Sep 14, 2017 at 4:51 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 09/12/2017 06:35 PM, Rob Clark wrote:
>> An event can be created with type==0, Shell.efi does this for an event
>> that is set when Ctrl-C is typed. So our current approach of having a
>> fixed set of timer slots, and determining which slots are unused by
>> type==0 doesn't work so well. But we don't have any particularly good
>> reason to have a fixed table of events, so just dynamically allocate
>> them and keep a list.
>>
>> Also fixes an incorrect implementation of CheckEvent() which was (a)
>> incorrectly returning an error if type==0, and (b) didn't handle the
>> case of an unsignaled event with a notify callback.
>>
>> With these fixes (plus implementation of SIMPLE_TEXT_INPUT_EX protocol),
>> Ctrl-C works in Shell.efi.
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>> include/efi_loader.h | 1 +
>> lib/efi_loader/efi_boottime.c | 167 +++++++++++++++++++-----------------------
>> 2 files changed, 78 insertions(+), 90 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 291ea86568..9dc1ea4b88 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -149,6 +149,7 @@ struct efi_event {
>> u64 trigger_time;
>> enum efi_timer_delay trigger_type;
>> int signaled;
>> + struct list_head link;
>
> Wouldn't we normally put this first in the structure?
It should not be required to be first, and anything that requires it
to be first is a bug.. you could have an object in multiple different
lists, for example.
>> };
>>
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 5db8ea9090..be3c34b9e0 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -255,11 +255,7 @@ static efi_status_t efi_create_handle(void **handle)
>> return r;
>> }
>>
>> -/*
>> - * Our event capabilities are very limited. Only a small limited
>> - * number of events is allowed to coexist.
>> - */
>> -static struct efi_event efi_events[16];
>> +static LIST_HEAD(efi_events);
>>
>> efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,
>> void (EFIAPI *notify_function) (
>> @@ -267,7 +263,7 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,
>> void *context),
>> void *notify_context, struct efi_event **event)
>> {
>> - int i;
>> + struct efi_event *evt;
>>
>> if (event == NULL)
>> return EFI_INVALID_PARAMETER;
>> @@ -279,20 +275,23 @@ efi_status_t efi_create_event(uint32_t type, UINTN notify_tpl,
>> notify_function == NULL)
>> return EFI_INVALID_PARAMETER;
>>
>> - 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_SUCCESS;
>> - }
>> - return EFI_OUT_OF_RESOURCES;
>> + evt = calloc(1, sizeof(*evt));
>> + if (!evt)
>> + return EFI_OUT_OF_RESOURCES;
>> +
>> + evt->type = type;
>> + evt->notify_tpl = notify_tpl;
>> + evt->notify_function = notify_function;
>> + evt->notify_context = notify_context;
>> + /* Disable timers on bootup */
>> + evt->trigger_next = -1ULL;
>> + evt->signaled = 0;
>> +
>> + list_add_tail(&evt->link, &efi_events);
>> +
>> + *event = evt;
>> +
>> + return EFI_SUCCESS;
>> }
>>
>> static efi_status_t EFIAPI efi_create_event_ext(
>> @@ -315,21 +314,24 @@ static efi_status_t EFIAPI efi_create_event_ext(
>> */
>> void efi_timer_check(void)
>> {
>> - int i;
>> + struct efi_event *evt;
>> u64 now = timer_get_us();
>>
>> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
>> - if (!efi_events[i].type ||
>> - !(efi_events[i].type & EVT_TIMER) ||
>> - efi_events[i].trigger_type == EFI_TIMER_STOP ||
>> - now < efi_events[i].trigger_next)
>> + /*
>> + * TODO perhaps optimize a bit and track the time of next
>> + * timer expiration?
>> + */
>
> The next expiration time is set below.
> What do you want to change?
I meant a global value for whichever is next timer to expire.. so
efi_timer_check() could have a fast-path.
background: I was thinking about adding efi_timer_check() to
EFI_ENTRY() and/or EFI_EXIT().. something I was playing around with
locally but havent turned into something fully baked yet. But I guess
if we call efi_timer_check() so often we don't want to loop thru all
the events each time if there is nothing ready to expire.
Possibly I could word that TODO comment better, it was mostly a note for myself.
>> + list_for_each_entry(evt, &efi_events, link) {
>> + if (!evt->type || !(evt->type & EVT_TIMER) ||
>> + evt->trigger_type == EFI_TIMER_STOP ||
>> + now < evt->trigger_next)
>> continue;
>> - if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC) {
>> - efi_events[i].trigger_next +=
>> - efi_events[i].trigger_time;
>> - efi_events[i].signaled = 0;
>> + if (evt->trigger_type == EFI_TIMER_PERIODIC) {
>> + evt->trigger_next +=
>> + evt->trigger_time;
>> + evt->signaled = 0;
>> }
>> - efi_signal_event(&efi_events[i]);
>> + efi_signal_event(evt);
>> }
>> WATCHDOG_RESET();
>> }
>> @@ -337,37 +339,32 @@ void efi_timer_check(void)
>> efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
>> uint64_t trigger_time)
>> {
>> - int i;
>> -
>> /*
>> * The parameter defines a multiple of 100ns.
>> * We use multiples of 1000ns. So divide by 10.
>> */
>> trigger_time = efi_div10(trigger_time);
>>
>> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
>> - if (event != &efi_events[i])
>> - continue;
>
> Please, check that event relates to an event in our list.
> We do not want to accept an invalid pointer.
but efi payload passing bogus pointer into EFI API is a generic
problem, not something specific to events.
We could add some type checking across the board by doing something
like adding a type-signature as first element in each object and
checking that the first (for example) four bytes match what we expect.
I vaguely recall seeing some code in edk2 (I think?) doing something
along those lines. Perhaps a useful thing to do, but I think that
should apply for all object types, and not be something event
specific.
And we can't even do that for things like file objects, where the efi
payload expects to dereference fxn ptrs in the struct.. maybe we can
stuff the type signature *before* the beginning of the object??
Anyways, I don't really believe that check that I dropped adds
anything of value.
>> + if (!(event->type & EVT_TIMER))
>> + return EFI_INVALID_PARAMETER;
>>
>> - if (!(event->type & EVT_TIMER))
>> - break;
>> - switch (type) {
>> - case EFI_TIMER_STOP:
>> - event->trigger_next = -1ULL;
>> - break;
>> - case EFI_TIMER_PERIODIC:
>> - case EFI_TIMER_RELATIVE:
>> - event->trigger_next =
>> + switch (type) {
>> + case EFI_TIMER_STOP:
>> + event->trigger_next = -1ULL;
>> + break;
>> + case EFI_TIMER_PERIODIC:
>> + case EFI_TIMER_RELATIVE:
>> + event->trigger_next =
>> timer_get_us() + trigger_time;
>> - break;
>> - default:
>> - return EFI_INVALID_PARAMETER;
>> - }
>> - event->trigger_type = type;
>> - event->trigger_time = trigger_time;
>> - return EFI_SUCCESS;
>> + break;
>> + default:
>> + return EFI_INVALID_PARAMETER;
>> }
>> - return EFI_INVALID_PARAMETER;
>> +
>> + event->trigger_type = type;
>> + event->trigger_time = trigger_time;
>> +
>> + return EFI_SUCCESS;
>> }
>>
>> static efi_status_t EFIAPI efi_set_timer_ext(struct efi_event *event,
>> @@ -382,7 +379,7 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
>> struct efi_event **event,
>> unsigned long *index)
>> {
>> - int i, j;
>> + int i;
>>
>> EFI_ENTRY("%ld, %p, %p", num_events, event, index);
>>
>> @@ -390,12 +387,6 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
>> if (!num_events || !event)
>> return EFI_EXIT(EFI_INVALID_PARAMETER);
>> for (i = 0; i < num_events; ++i) {
>> - for (j = 0; j < ARRAY_SIZE(efi_events); ++j) {
>> - if (event[i] == &efi_events[j])
>> - goto known_event;
>> - }
>> - return EFI_EXIT(EFI_INVALID_PARAMETER);
>> -known_event:
>
> Check that the event pointer is in our list.
>
>> if (!event[i]->type || event[i]->type & EVT_NOTIFY_SIGNAL)
>> return EFI_EXIT(EFI_INVALID_PARAMETER);
>> }
>> @@ -424,50 +415,46 @@ out:
>>
>> static efi_status_t EFIAPI efi_signal_event_ext(struct efi_event *event)
>> {
>> - int i;
>> -
>> EFI_ENTRY("%p", event);
>> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
>> - if (event != &efi_events[i])
>> - continue;
>> - efi_signal_event(event);
>> - break;
>> - }
>> + efi_signal_event(event);
>> return EFI_EXIT(EFI_SUCCESS);
>> }
>>
>> static efi_status_t EFIAPI efi_close_event(struct efi_event *event)
>> {
>> - int i;
>> -
>> EFI_ENTRY("%p", event);
>> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
>> - if (event == &efi_events[i]) {
>> - event->type = 0;
>> - event->trigger_next = -1ULL;
>> - event->signaled = 0;
>> - return EFI_EXIT(EFI_SUCCESS);
>> - }
>> - }
>> - return EFI_EXIT(EFI_INVALID_PARAMETER);
>
> Return an error code if the event was not in our list.
>
>> + list_del(&event->link);
>> + free(event);
>> + return EFI_EXIT(EFI_SUCCESS);
>> }
>>
>> +/*
>> + * - If Event is in the signaled state, it is cleared and EFI_SUCCESS
>> + * is returned.
>> + *
>> + * - If Event is not in the signaled state and has no notification
>> + * function, EFI_NOT_READY is returned.
>> + *
>> + * - If Event is not in the signaled state but does have a notification
>> + * function, the notification function is queued at the event’s
>> + * notification task priority level. If the execution of the
>> + * notification function causes Event to be signaled, then the signaled
>> + * state is cleared and EFI_SUCCESS is returned; if the Event is not
>> + * signaled, then EFI_NOT_READY is returned.
>> + */
>> static efi_status_t EFIAPI efi_check_event(struct efi_event *event)
>> {
>> - int i;
>> -
>> EFI_ENTRY("%p", event);
>> efi_timer_check();
>> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
>> - if (event != &efi_events[i])
>> - continue;
>> - if (!event->type || event->type & EVT_NOTIFY_SIGNAL)
>> - break;
>> - if (event->signaled)
>> - return EFI_EXIT(EFI_SUCCESS);
>> - return EFI_EXIT(EFI_NOT_READY);
>
> Check that the event is on our list.
>
>> + if (event->type & EVT_NOTIFY_SIGNAL)
>> + return EFI_EXIT(EFI_INVALID_PARAMETER);
>> + if (!event->signaled && event->notify_function)
>> + EFI_CALL(event->notify_function(event, event->notify_context));
>> + if (event->signaled) {
>> + event->signaled = 0;
>> + return EFI_EXIT(EFI_SUCCESS);
>> }
>> - return EFI_EXIT(EFI_INVALID_PARAMETER);
>> + return EFI_EXIT(EFI_NOT_READY);
>> }
>>
>> static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
>>
>
> The changes make sense. But unfortunately the patch series is not
> applicable to efi-next.
>
> Either rebase it or provide information concerning the prerequisite patches.
atm I can't really rebase my stack of patches on efi-next until the
readdir patches land and efi-next is rebased. Once that happens I'll
rebase on efi-next. (Hopefully that happens before next Tues since
latter part of next week I probably won't have an opportunity to test
the rebase on real hw.)
BR,
-R
> Please, check in all functions that the event parameter relates to a
> pointer on our list. Otherwise return EFI_INVALID_PARAMETER.
>
> Best regards
>
> Heinrich
More information about the U-Boot
mailing list