[U-Boot] [PATCH 07/11] efi_loader: fix events

Rob Clark robdclark at gmail.com
Wed Oct 11 22:09:17 UTC 2017


On Wed, Oct 11, 2017 at 10:49 AM, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 10.10.17 14:23, 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 | 217 +++++++++++++++++++++---------------------
>>  2 files changed, 111 insertions(+), 107 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index e6e55d2cb4..2232caca44 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -154,6 +154,7 @@ struct efi_event {
>>       enum efi_timer_delay trigger_type;
>>       bool is_queued;
>>       bool is_signaled;
>> +     struct list_head link;
>>  };
>>
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 39dcc72648..19fafe546c 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -350,11 +350,26 @@ static efi_status_t efi_create_handle(void **handle)
>>       return r;
>>  }
>>
>> +static LIST_HEAD(efi_events);
>> +
>>  /*
>> - * Our event capabilities are very limited. Only a small limited
>> - * number of events is allowed to coexist.
>> + * Check if a pointer is a valid event.
>> + *
>> + * It might be nice at some point to extend this to a more general
>> + * mechanism to check if pointers passed from the EFI world are
>> + * valid objects of a particular type.
>>   */
>> -static struct efi_event efi_events[16];
>> +static bool efi_is_event(const void *obj)
>> +{
>> +     struct efi_event *evt;
>> +
>> +     list_for_each_entry(evt, &efi_events, link) {
>> +             if (evt == obj)
>> +                     return true;
>> +     }
>> +
>> +     return false;
>> +}
>>
>>  /*
>>   * Create an event.
>> @@ -377,7 +392,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;
>> @@ -389,21 +404,24 @@ 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].is_queued = false;
>> -             efi_events[i].is_signaled = false;
>> -             *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->is_queued = false;
>> +     evt->is_signaled = false;
>> +
>> +     list_add_tail(&evt->link, &efi_events);
>> +
>> +     *event = evt;
>> +
>> +     return EFI_SUCCESS;
>>  }
>>
>>  /*
>> @@ -443,30 +461,31 @@ 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)
>> -                     continue;
>> -             if (efi_events[i].is_queued)
>> -                     efi_signal_event(&efi_events[i]);
>> -             if (!(efi_events[i].type & EVT_TIMER) ||
>> -                 now < efi_events[i].trigger_next)
>> +     /*
>> +      * TODO perhaps optimize a bit and track the time of next
>> +      * timer to expire?
>> +      */
>> +     list_for_each_entry(evt, &efi_events, link) {
>> +             if (evt->is_queued)
>> +                     efi_signal_event(evt);
>> +             if (!(evt->type & EVT_TIMER) ||
>> +                 now < evt->trigger_next)
>>                       continue;
>> -             switch (efi_events[i].trigger_type) {
>> +             switch (evt->trigger_type) {
>>               case EFI_TIMER_RELATIVE:
>> -                     efi_events[i].trigger_type = EFI_TIMER_STOP;
>> +                     evt->trigger_type = EFI_TIMER_STOP;
>>                       break;
>>               case EFI_TIMER_PERIODIC:
>> -                     efi_events[i].trigger_next +=
>> -                             efi_events[i].trigger_time;
>> +                     evt->trigger_next += evt->trigger_time;
>>                       break;
>>               default:
>>                       continue;
>>               }
>> -             efi_events[i].is_signaled = true;
>> -             efi_signal_event(&efi_events[i]);
>> +             evt->is_signaled = true;
>> +             efi_signal_event(evt);
>>       }
>>       WATCHDOG_RESET();
>>  }
>> @@ -485,7 +504,8 @@ 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;
>> +     if (!efi_is_event(event))
>> +             return EFI_INVALID_PARAMETER;
>>
>>       /*
>>        * The parameter defines a multiple of 100ns.
>> @@ -493,30 +513,25 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
>>        */
>>       do_div(trigger_time, 10);
>>
>> -     for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
>> -             if (event != &efi_events[i])
>> -                     continue;
>> +     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 =
>> -                             timer_get_us() + trigger_time;
>> -                     break;
>> -             default:
>> -                     return EFI_INVALID_PARAMETER;
>> -             }
>> -             event->trigger_type = type;
>> -             event->trigger_time = trigger_time;
>> -             event->is_signaled = false;
>> -             return EFI_SUCCESS;
>> +     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;
>>       }
>> -     return EFI_INVALID_PARAMETER;
>> +     event->trigger_type = type;
>> +     event->trigger_time = trigger_time;
>> +     event->is_signaled = false;
>> +
>> +     return EFI_SUCCESS;
>>  }
>>
>>  /*
>> @@ -555,7 +570,7 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
>>                                             struct efi_event **event,
>>                                             size_t *index)
>>  {
>> -     int i, j;
>> +     int i;
>>
>>       EFI_ENTRY("%ld, %p, %p", num_events, event, index);
>>
>> @@ -566,12 +581,8 @@ static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
>>       if (efi_tpl != TPL_APPLICATION)
>>               return EFI_EXIT(EFI_UNSUPPORTED);
>>       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:
>> +             if (!efi_is_event(event[i]))
>> +                     return EFI_EXIT(EFI_INVALID_PARAMETER);
>>               if (!event[i]->type || event[i]->type & EVT_NOTIFY_SIGNAL)
>>                       return EFI_EXIT(EFI_INVALID_PARAMETER);
>>               if (!event[i]->is_signaled)
>> @@ -614,19 +625,12 @@ 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;
>> -             if (event->is_signaled)
>> -                     break;
>> -             event->is_signaled = true;
>> -             if (event->type & EVT_NOTIFY_SIGNAL)
>> -                     efi_signal_event(event);
>> -             break;
>> -     }
>> +     if (!efi_is_event(event))
>> +             return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +     event->is_signaled = true;
>> +     if (event->type & EVT_NOTIFY_SIGNAL)
>> +             efi_signal_event(event);
>>       return EFI_EXIT(EFI_SUCCESS);
>>  }
>>
>> @@ -642,19 +646,10 @@ static efi_status_t EFIAPI efi_signal_event_ext(struct efi_event *event)
>>   */
>>  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->is_queued = false;
>> -                     event->is_signaled = false;
>> -                     return EFI_EXIT(EFI_SUCCESS);
>> -             }
>> -     }
>> -     return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +     list_del(&event->link);
>> +     free(event);
>> +     return EFI_EXIT(EFI_SUCCESS);
>>  }
>>
>>  /*
>> @@ -664,29 +659,37 @@ static efi_status_t EFIAPI efi_close_event(struct efi_event *event)
>>   * See the Unified Extensible Firmware Interface (UEFI) specification
>>   * for details.
>>   *
>> - * If an event is not signaled yet the notification function is queued.
>> + * - 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.
>>   *
>>   * @event    event to check
>>   * @return   status code
>>   */
>> -static efi_status_t EFIAPI efi_check_event(struct efi_event *event)
>> +/*
>> + */
>
> I assume this is one comment block too much?

ugg, looks like I screwed that up when rebase/mergetool'ing.. can you
drop that when applying or do you want me to resend?

BR,
-R

>> +static efi_status_t EFIAPI efi_check_event(struct efi_event *evt)
>>  {
>
>
> Alex
>
>


More information about the U-Boot mailing list