On Fri, Oct 13, 2017 at 1:24 AM, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
>
> 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 <robdcl...@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?
>
When I originally wrote this patch (prior to your addition of TPL
handling), we weren't handling the 3rd case mentioned in the comment I
added, IIRC. (We also were running into a problem with an event that
shell was creating with type==0 that it would signal when the user hit
ctrl-c to interrupt a command, which was what prompted the switch to a
list.)
Looks like you fixed the first issue in efi_signal_event(), which I
overlooked when rebasing this patch.
BR,
-R
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
