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

On Wed, Oct 11, 2017 at 10:49 AM, Alexander Graf agraf@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@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