[U-Boot] [PATCH 1/3] efi_loader: implement multiple event support
Alexander Graf
agraf at suse.de
Wed Jul 12 10:55:57 UTC 2017
On 05.07.17 19:47, Heinrich Schuchardt wrote:
> Up to now the boot time supported only a single event.
> This patch now allows four events.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
> include/efi_api.h | 8 ++-
> include/efi_loader.h | 23 ++++++
> lib/efi_loader/efi_boottime.c | 161 ++++++++++++++++++++++++++----------------
> 3 files changed, 131 insertions(+), 61 deletions(-)
>
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 42cd47ff08..18bef722c6 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -28,8 +28,12 @@ enum efi_event_type {
> EFI_TIMER_RELATIVE = 2
> };
>
> -#define EVT_NOTIFY_WAIT 0x00000100
> -#define EVT_NOTIFY_SIGNAL 0x00000200
> +#define EVT_TIMER 0x80000000
> +#define EVT_RUNTIME 0x40000000
> +#define EVT_NOTIFY_WAIT 0x00000100
> +#define EVT_NOTIFY_SIGNAL 0x00000200
> +#define EVT_SIGNAL_EXIT_BOOT_SERVICES 0x00000201
> +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202
>
> /* EFI Boot Services table */
> struct efi_boot_services {
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index c620652307..a35b971f7e 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -71,6 +71,29 @@ struct efi_object {
> void *handle;
> };
>
> +/**
> + * struct efi_event
> + *
> + * @type: Type of event
> + * @trigger_type: Type of timer
> + * @trigger_time: Period of the timer
> + * @trigger_next: Next time to trigger the timer
> + * @nofify_function: Function to call when the event is triggered
> + * @notify_context: Data to be passed to the notify function
> + * @signaled: The notify function was already called
> + */
> +struct efi_event {
> + u32 type;
> + unsigned long notify_tpl;
> + void (EFIAPI *notify_function)(void *event, void *context);
> + void *notify_context;
> + u64 trigger_next;
> + u32 trigger_time;
> + enum efi_event_type trigger_type;
> + int signaled;
> +};
> +
> +
> /* This list contains all UEFI objects we know of */
> extern struct list_head efi_obj_list;
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 3060c25a2a..ef3e7d9d52 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -162,21 +162,10 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
> }
>
> /*
> - * Our event capabilities are very limited. Only support a single
> - * event to exist, so we don't need to maintain lists.
> + * Our event capabilities are very limited. Only a small limited
> + * number of events is allowed to coexist.
> */
> -static struct {
> - enum efi_event_type type;
> - u32 trigger_type;
> - u32 trigger_time;
> - u64 trigger_next;
> - unsigned long notify_tpl;
> - void (EFIAPI *notify_function) (void *event, void *context);
> - void *notify_context;
> -} efi_event = {
> - /* Disable timers on bootup */
> - .trigger_next = -1ULL,
> -};
> +static struct efi_event efi_events[16];
>
> static efi_status_t EFIAPI efi_create_event(
> enum efi_event_type type, ulong notify_tpl,
> @@ -184,13 +173,10 @@ static efi_status_t EFIAPI efi_create_event(
> void *context),
> void *notify_context, void **event)
> {
> + int i;
> +
> EFI_ENTRY("%d, 0x%lx, %p, %p", type, notify_tpl, notify_function,
> notify_context);
> - if (efi_event.notify_function) {
> - /* We only support one event at a time */
> - return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> - }
> -
> if (event == NULL)
> return EFI_EXIT(EFI_INVALID_PARAMETER);
>
> @@ -201,13 +187,20 @@ static efi_status_t EFIAPI efi_create_event(
> notify_function == NULL)
> return EFI_EXIT(EFI_INVALID_PARAMETER);
>
> - efi_event.type = type;
> - efi_event.notify_tpl = notify_tpl;
> - efi_event.notify_function = notify_function;
> - efi_event.notify_context = notify_context;
> - *event = &efi_event;
> -
> - return EFI_EXIT(EFI_SUCCESS);
> + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> + if (efi_events[i].type)
Please explicitly check for EFI_TIMER_STOP here.
> + 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_EXIT(EFI_SUCCESS);
> + }
> + return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> }
>
> /*
> @@ -216,17 +209,25 @@ static efi_status_t EFIAPI efi_create_event(
> */
> void efi_timer_check(void)
> {
> + int i;
> u64 now = timer_get_us();
>
> - if (now >= efi_event.trigger_next) {
> - /* Triggering! */
> - if (efi_event.trigger_type == EFI_TIMER_PERIODIC)
> - efi_event.trigger_next += efi_event.trigger_time / 10;
> - if (efi_event.type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL))
> - efi_event.notify_function(&efi_event,
> - efi_event.notify_context);
> + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> + if (!(efi_events[i].type & EVT_TIMER) ||
> + efi_events[i].trigger_type == EFI_TIMER_STOP ||
> + now < efi_events[i].trigger_next)
> + continue;
This if clause is quite hard to read. I don't think it would hurt to
just unfold it into individual cases? You can then also document for
each why they should get ignored.
> + if (efi_events[i].trigger_type == EFI_TIMER_PERIODIC)
> + efi_events[i].trigger_next +=
> + efi_events[i].trigger_time / 10;
I stumbled over my own code here - awesome :). Can you please put the
division into a static function that tells the reader from which number
space into which number space we're converting (100ns)?
> + efi_events[i].signaled = 1;
> + if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {
> + EFI_EXIT(EFI_SUCCESS);
This is quite dangerous...
> + efi_events[i].notify_function(&efi_events[i],
> + efi_events[i].notify_context);
> + efi_restore_gd();
... I see that you do restore gd but please document this heavily with a
big, obvious comment and ideally even use EFI_ENTRY() again here instead
of hard coding efi_restore_gd().
> + }
wrong indentation
> }
> -
> WATCHDOG_RESET();
> }
>
> @@ -236,67 +237,109 @@ static efi_status_t EFIAPI efi_set_timer(void *event, int type,
> /* We don't have 64bit division available everywhere, so limit timer
> * distances to 32bit bits. */
> u32 trigger32 = trigger_time;
> + int i;
>
> EFI_ENTRY("%p, %d, %"PRIx64, event, type, trigger_time);
>
> if (trigger32 < trigger_time) {
> + trigger32 = 0xffffffff;
This change should be a separate patch I suppose. Makes bisecting things
easier.
> printf("WARNING: Truncating timer from %"PRIx64" to %x\n",
> trigger_time, trigger32);
> }
>
> - if (event != &efi_event) {
> - /* We only support one event at a time */
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> - }
> + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> + if (event != &efi_events[i])
> + continue;
>
> - switch (type) {
> - case EFI_TIMER_STOP:
> - efi_event.trigger_next = -1ULL;
> - break;
> - case EFI_TIMER_PERIODIC:
> - case EFI_TIMER_RELATIVE:
> - efi_event.trigger_next = timer_get_us() + (trigger32 / 10);
> - break;
> - default:
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + switch (type) {
> + case EFI_TIMER_STOP:
> + efi_events[i].trigger_next = -1ULL;
> + break;
> + case EFI_TIMER_PERIODIC:
> + case EFI_TIMER_RELATIVE:
> + efi_events[i].trigger_next =
> + timer_get_us() + (trigger32 / 10);
> + break;
> + default:
> + return EFI_EXIT(EFI_INVALID_PARAMETER);
> + }
> + efi_events[i].trigger_type = type;
> + efi_events[i].trigger_time = trigger_time;
> + return EFI_EXIT(EFI_SUCCESS);
> }
> - efi_event.trigger_type = type;
> - efi_event.trigger_time = trigger_time;
> -
> - return EFI_EXIT(EFI_SUCCESS);
> + return EFI_EXIT(EFI_INVALID_PARAMETER);
> }
>
> static efi_status_t EFIAPI efi_wait_for_event(unsigned long num_events,
> void *event, unsigned long *index)
> {
> - u64 now;
> + int i;
>
> EFI_ENTRY("%ld, %p, %p", num_events, event, index);
>
> - now = timer_get_us();
> - while (now < efi_event.trigger_next) { }
> - efi_timer_check();
> -
> + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> + if (event != &efi_events[i])
> + continue;
We also need to check if EVT_NOTIFY_SIGNAL is set and error out in that
case, no?
> + while (!efi_events[i].signaled)
> + efi_timer_check();
Ah, nice, so your new code also resets the watchdog. Very good :).
> + efi_events[i].signaled = 0;
Please document that line with reference to the spec.
> + break;
> + }
> return EFI_EXIT(EFI_SUCCESS);
> }
>
> static efi_status_t EFIAPI efi_signal_event(void *event)
> {
> + int i;
> +
> EFI_ENTRY("%p", event);
> + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> + if (event != &efi_events[i])
> + continue;
> + if (efi_events[i].signaled)
> + break;
> + efi_events[i].signaled = 1;
> + if (efi_events[i].type & EVT_NOTIFY_SIGNAL) {
> + EFI_EXIT(EFI_SUCCESS);
> + efi_events[i].notify_function(&efi_events[i],
> + efi_events[i].notify_context);
> + efi_restore_gd();
This looks like code repetition. Can you fold it in with the same code
above?
> + }
> + break;
> + }
> return EFI_EXIT(EFI_SUCCESS);
> }
>
> static efi_status_t EFIAPI efi_close_event(void *event)
> {
> + int i;
> +
> EFI_ENTRY("%p", event);
> - efi_event.trigger_next = -1ULL;
> + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
> + if (event != &efi_events[i])
> + continue;
> + efi_events[i].type = 0;
Use STOPPED here.
> + efi_events[i].trigger_next = -1ULL;
> + efi_events[i].signaled = 0;
> + break;
> + }
> return EFI_EXIT(EFI_SUCCESS);
> }
>
> static efi_status_t EFIAPI efi_check_event(void *event)
> {
> + int i;
> +
> EFI_ENTRY("%p", event);
> - return EFI_EXIT(EFI_NOT_READY);
> + efi_timer_check();
> + for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
Here to we need to check for EVT_NOTIFY_SIGNAL and error out, no?
Alex
> + if (event != &efi_events[i])
> + continue;
> + if (efi_events[i].signaled)
> + return EFI_EXIT(EFI_SUCCESS);
> + return EFI_EXIT(EFI_NOT_READY);
> + }
> + return EFI_EXIT(EFI_INVALID_PARAMETER);
> }
>
> static efi_status_t EFIAPI efi_install_protocol_interface(void **handle,
>
More information about the U-Boot
mailing list