[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