[U-Boot] [PATCH 1/3] efi_loader: implement multiple event support
Heinrich Schuchardt
xypron.glpk at gmx.de
Mon Jul 17 10:43:55 UTC 2017
On 07/16/2017 09:25 AM, Alexander Graf wrote:
>
>
> 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 ;).
We are talking about this structure after applying upconfing v2 of the
patch:
/**
* struct efi_event
*
* @type: Type of event provided by CreateEvent
* @notify_tpl: Task priority level of notifications
* @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
* @trigger_type: Type of timer provided by SetTimer
* @signaled: The notify function was already called
*/
struct efi_event {
u32 type;
unsigned long notify_tpl;
void (EFIAPI *notify_function)(struct efi_event *event, void
*context);
void *notify_context;
u64 trigger_next;
u32 trigger_time;
enum efi_event_type trigger_type;
int signaled;
};
Field type is a bitmask containing:
EVT_TIMER, EVT_RUNTIME, EVT_NOTIFY_WAIT, EVT_NOTIFY_SIGNAL,
EVT_SIGNAL_EXIT_BOOT_SERVICES, EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE
Type is set in efi_create_event().
Type is zero if the event slot is empty.
Type is non-zero if the event slot is used.
Field trigger_type contains either of
EFI_TIMER_STOP, EFI_TIMER_PERIODIC, or EFI_TIMER_RELATIVE.
Trigger_type is set in efi_set_timer().
>
>>
>>>
>>>> + 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();
> }
>
Will be done in v2 of the patch.
>>
>>>
>>>> + }
>>>
>>> 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.
>
No type is not an enum but a bitmask. See above.
Regards
Heinrich
>>
>>>
>>>> + 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