[U-Boot] [PATCH 07/11] efi_loader: fix events
Heinrich Schuchardt
xypron.glpk at gmx.de
Fri Oct 13 05:24:23 UTC 2017
On 10/10/2017 02:23 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 | 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)
> +/*
> + */
> +static efi_status_t EFIAPI efi_check_event(struct efi_event *evt)
> {
> - int i;
> -
> - EFI_ENTRY("%p", event);
> + EFI_ENTRY("%p", evt);
> 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->is_signaled)
> - efi_signal_event(event);
> - if (event->is_signaled)
> - return EFI_EXIT(EFI_SUCCESS);
> - return EFI_EXIT(EFI_NOT_READY);
> + if (!efi_is_event(evt) || (evt->type & EVT_NOTIFY_SIGNAL))
> + return EFI_EXIT(EFI_INVALID_PARAMETER);
> + if (!evt->is_signaled && evt->notify_function)
> + EFI_CALL_VOID(evt->notify_function(evt, evt->notify_context));
Here you are doing the contrary of what you describe above:
You do not check the task priority level. You call the notification
function irrespective of the TPL.
You should queue the notification function if the current TPL is greater
or equal to the TPL of the notification functions. This is what the
removed call to function efi_signal_event was doing before your patch.
Could you, please, describe the rationale of this change. Where did you
get problems with the queuing logic?
Regards
Heinrich
> + if (evt->is_signaled) {
> + evt->is_signaled = true;
> + return EFI_EXIT(EFI_SUCCESS);
> }
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_EXIT(EFI_NOT_READY);
> }
>
> /*
> @@ -1440,15 +1443,15 @@ static void efi_exit_caches(void)
> static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
> unsigned long map_key)
> {
> - int i;
> + struct efi_event *evt;
>
> EFI_ENTRY("%p, %ld", image_handle, map_key);
>
> /* Notify that ExitBootServices is invoked. */
> - for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> - if (efi_events[i].type != EVT_SIGNAL_EXIT_BOOT_SERVICES)
> + list_for_each_entry(evt, &efi_events, link) {
> + if (evt->type != EVT_SIGNAL_EXIT_BOOT_SERVICES)
> continue;
> - efi_signal_event(&efi_events[i]);
> + efi_signal_event(evt);
> }
> /* Make sure that notification functions are not called anymore */
> efi_tpl = TPL_HIGH_LEVEL;
>
More information about the U-Boot
mailing list