[U-Boot] [PATCH 1/3] efi_loader: implement multiple event support
Alexander Graf
agraf at suse.de
Sun Jul 16 07:25:46 UTC 2017
On 15.07.17 13:43, Heinrich Schuchardt wrote:
> On 07/12/2017 12:55 PM, Alexander Graf wrote:
>>
>>
>> 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.
>
> TimerCancel (alias EFI_TIMER_STOP) is set in TriggerTimer.
> If the status of a timer is set to TimerCancel this does not imply that
> the event can be deleted (i.e. the slot can be reused).
>
> The owner of the event might decide to call TriggerTimer for the same
> event at a later time with TimerPeriodic or TimerRelative.
>
> So I do not understand why I should check for EFI_TIMER_STOP here.
Simply because you're checking for it already, but implicitly:
enum efi_event_type {
EFI_TIMER_STOP = 0,
EFI_TIMER_PERIODIC = 1,
EFI_TIMER_RELATIVE = 2
};
and to avoid confusion, I prefer to call out enums when we match them.
Otherwise someone will think that this check will include EFI_TIMER_STOP
one day ;).
>
>>
>>> + 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...
>
> What do you mean by "quite dangerous"?
>
> We have to call the EFI application here.
> Thus we have to switch the gd context.
Yes, but switching context is always tricky :).
>
>>
>>> + 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().
>
> I used efi_restore_gd() to see less messages in debug mode.
>
> In the reply to another patch you suggested to provide a configurable
> verbosity level for EFI_ENTRY. So we can use EFI_ENTRY here.
That works too, but for clarity it's usually much nicer to have a full
function start with ENTRY and end with EXIT. That way things become more
obvious. I've wasted way too many hours on debugging ENTRY/EXIT
misplacement.
One thing that would already help readability would be to just move the
whole exit/entry dance into a separate function. The compiler will
inline it, so the compiled code should be the same. But at least for the
reader it's obvious what happens:
static void efi_call_notify(...)
{
/*
* Description about the necessity of this function
*/
EFI_EXIT();
notifier(...);
/* Go back to U-Boot space */
EFI_ENTRY();
}
>
>>
>>> + }
>>
>> wrong indentation
>
> ok
>
>>
>>> }
>>> -
>>> 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.
>
> ok
>
>>
>>> 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?
>
> Yes according to the UEFI spec.
>
>>
>>> + 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.
>
> ok
>
>>
>>> + 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?
>
> ok
>>
>>> + }
>>> + 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.
>
> This is not the trigger_type field (where we could set EFI_TIMER_STOP).
Ok, I'm confused. In efi_create_event() you set efi_events[i] to the
parameter "type" which is enum efi_event_type, no?
So what are the semantics of the "type" field vs "trigger_type"?
To clarify that, please make the "type" field its own enum and double
check that it only gets assigned with values that match that enum.
>
>>
>>> + 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?
>
> No. You may want to check if the event available even if you could wait
> for it.
>
> E.g. the application sets up an event for console input.
> If there is some other work to do, just poll the event with CheckEvent.
> If all work is done, use WaitForEvent.
Well, I agree, but the spec explicitly states that CheckEvent() should
error out if it sees EVT_NOTIFY_SIGNAL ;).
Alex
More information about the U-Boot
mailing list