[U-Boot] [RFC 6/6] efi_loader: variable: support runtime variable access via cache

Heinrich Schuchardt xypron.glpk at gmx.de
Sat Jun 15 19:01:56 UTC 2019


On 6/5/19 6:21 AM, AKASHI Takahiro wrote:
> With this patch, cache buffer for UEFI variables will be created
> so that we will still be able to access, at least retrieve,
> UEFI variables when we exit from boottime services,
>
> This feature is a "should" behavior described in EBBR v1.0
> section 2.5.3.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> ---
>   include/efi_loader.h          |  17 ++
>   lib/efi_loader/Kconfig        |   9 +
>   lib/efi_loader/efi_boottime.c |  10 +-
>   lib/efi_loader/efi_runtime.c  |  13 +
>   lib/efi_loader/efi_variable.c | 467 ++++++++++++++++++++++++++++++++++
>   5 files changed, 515 insertions(+), 1 deletion(-)

Please, put the cache into a separate file.

>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 93f7ece814a0..acab657b9d70 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -620,6 +620,23 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>   				     const efi_guid_t *vendor, u32 attributes,
>   				     efi_uintn_t data_size, const void *data);
>
> +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING
> +efi_status_t efi_freeze_variable_table(void);
> +
> +/* runtime version of APIs */
> +efi_status_t
> +__efi_runtime EFIAPI efi_get_variable_runtime(u16 *variable_name,

I think one version of the functions serving at runtime and boottime is
enough.

The cache should be used both at runtime and at boottime. Essentially I
expect three modules working together:

UEFI API implementation <-> Cache <-> Persistence driver

I would suggest to put each of these into a separate file.

Both the API implementation and the Cache have to be available at
Boottime and at Runtime. A first version of the persistence driver may
only be working at boottime.

The NV-cache content should be written to non-volatile memory on Reset()
and on ExitBootServices() and if possible when updating variables at
runtime.

> +					      const efi_guid_t *vendor,
> +					      u32 *attributes,
> +					      efi_uintn_t *data_size,
> +					      void *data);
> +efi_status_t
> +__efi_runtime EFIAPI efi_get_next_variable_name_runtime(
> +						efi_uintn_t *variable_name_size,
> +						u16 *variable_name,
> +						const efi_guid_t *vendor);
> +#endif /* CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING */
> +
>   /*
>    * See section 3.1.3 in the v2.7 UEFI spec for more details on
>    * the layout of EFI_LOAD_OPTION.  In short it is:
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index e2ef43157568..3f284795648f 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -59,6 +59,15 @@ config EFI_RUNTIME_CONVERT_POINTER
>   	  to be called by UEFI drivers in relocating themselves to virtual
>   	  address space.
>
> +config EFI_RUNTIME_GET_VARIABLE_CACHING
> +	bool "runtime_service: GetVariable: Enable runtime access via cache (read-only)"
> +	default y
> +	help
> +	  Select this option if you want to access UEFI variables at
> +	  runtime even though you cannot update values on the fly.
> +	  With or without this option, you can access UEFI variables
> +	  at boottime.

Updates of volatile variables should always be possible.

> +
>   config EFI_DEVICE_PATH_TO_TEXT
>   	bool "Device path to text protocol"
>   	default y
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index e4abaf3601d9..14e343abbd43 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1892,6 +1892,9 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>   						  efi_uintn_t map_key)
>   {
>   	struct efi_event *evt;
> +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING
> +	efi_status_t ret;
> +#endif
>
>   	EFI_ENTRY("%p, %zx", image_handle, map_key);
>
> @@ -1921,7 +1924,12 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>   		}
>   	}
>
> -	/* TODO: Should persist EFI variables here */
> +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING

Can we have weak functions for initializing and persisting the cache, e.g.

efi_status_t __weak
efi_load_variable_cache(cache_entry *cache, size_t *size)
{
         cache->len = 0;
         return EFI_SUCCESS;
}

efi_status_t __runtime __weak
efi_write_variable_cache(cache_entry *cache, size_t size)
{
         return EFI_UNSUPPORTED;
}

Then we can override these in whatever driver we implement.


> +	/* No more variable update */
> +	ret = efi_freeze_variable_table();
> +	if (ret != EFI_SUCCESS)
> +		return EFI_EXIT(ret);
> +#endif
>
>   	board_quiesce_devices();
>
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index fc5bdee80e00..b60f70f04613 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -111,6 +111,11 @@ efi_status_t efi_init_runtime_supported(void)
>   	efi_runtime_services_supported |=
>   				EFI_RT_SUPPORTED_CONVERT_POINTER;
>   #endif
> +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING
> +	efi_runtime_services_supported |=
> +				(EFI_RT_SUPPORTED_GET_VARIABLE |
> +				 EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME);
> +#endif
>
>   	return EFI_CALL(efi_set_variable(L"RuntimeServicesSupported",
>   					 &efi_global_variable_guid,
> @@ -469,10 +474,18 @@ static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = {
>   		.patchto = NULL,
>   	}, {
>   		.ptr = &efi_runtime_services.get_variable,
> +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING
> +		.patchto = &efi_get_variable_runtime,
> +#else
>   		.patchto = &efi_device_error,
> +#endif
>   	}, {
>   		.ptr = &efi_runtime_services.get_next_variable_name,
> +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING
> +		.patchto = &efi_get_next_variable_name,
> +#else
>   		.patchto = &efi_device_error,
> +#endif
>   	}, {
>   		.ptr = &efi_runtime_services.set_variable,
>   		.patchto = &efi_device_error,
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index d9887be938c2..ee21892dd291 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -706,3 +706,470 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>
>   	return EFI_EXIT(ret);
>   }
> +
> +#ifdef CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING
> +/*
> + * runtime version of APIs
> + * We only support read-only variable access.
> + * The table is in U-Boot's hash table format, but has its own
> + * _ENTRY structure for specific use.
> + *
> + * Except for efi_freeze_variable_table(), which is to be called in
> + * exit_boot_services(), all the functions and data below must be
> + * placed in either RUNTIME_SERVICES_CODE or RUNTIME_SERVICES_DATA.
> + */
> +typedef struct _ENTRY {
> +	unsigned int used;	/* hash value; 0 for not used */
> +	size_t name;		/* name offset from itself */
> +	efi_guid_t vendor;
> +	u32 attributes;
> +	size_t data;		/* data offset from itself */
> +	size_t data_size;
> +} _ENTRY;
> +
> +static inline u16 *entry_name(_ENTRY *e) { return (void *)e + e->name; }
> +static inline u16 *entry_data(_ENTRY *e) { return (void *)e + e->data; }
> +
> +static struct hsearch_data *efi_variable_table __efi_runtime_data;
> +
> +static size_t __efi_runtime u16_strlen_runtime(const u16 *s1)


Please, do not duplicate existing functions. If they have to be runtime
simply change the existing function to __runtime.

> +{
> +	size_t n = 0;
> +
> +	while (*s1) {
> +		n++;
> +		s1++;
> +	}
> +
> +	return n;
> +}
> +
> +static int __efi_runtime memcmp_runtime(const void *m1, const void *m2,
> +					size_t n)

I dislike duplicate code. Can't we simply define the existing memcmp
function as __runtime?
> +{
> +	while (n && *(u8 *)m1 == *(u8 *)m2) {
> +		n--;
> +		m1++;
> +		m2++;
> +	}
> +
> +	if (n)
> +		return *(u8 *)m1 - *(u8 *)m2;
> +
> +	return 0;
> +}
> +
> +static void __efi_runtime memcpy_runtime(void *m1, const void *m2, size_t n)
> +{

Can't we simply define the existing memcpy function as __runtime?

> +	for (; n; n--, m1++, m2++)
> +		*(u8 *)m1 = *(u8 *)m2;
> +}
> +
> +static int __efi_runtime efi_cmpkey(_ENTRY *e, const u16 *name,
> +				    const efi_guid_t *vendor)
> +{
> +	size_t name_len;
> +
> +	name_len = u16_strlen_runtime(entry_name(e));
> +
> +	/* return zero if matched */
> +	return name_len != u16_strlen_runtime(name) ||
> +	       memcmp_runtime(entry_name(e), name, name_len * 2) ||
> +	       memcmp_runtime(e->vendor.b, vendor->b, sizeof(vendor));
> +}
> +
> +/* simplified and slightly different version of hsearch_r() */

These hash functions are so complicated that they really need a unit
test testing them rigorously.

> +static int __efi_runtime hsearch_runtime(const u16 *name,
> +					 const efi_guid_t *vendor,
> +					 ACTION action,
> +					 _ENTRY **retval,
> +					 struct hsearch_data *htab)
> +{
> +	unsigned int hval;
> +	unsigned int count;
> +	unsigned int len;
> +	unsigned int idx, new;
> +
> +	/* Compute an value for the given string. */
> +	len = u16_strlen_runtime(name);

Can't the same variable name exist for different GUIDs? Why is the GUID
not considered in the hash?

> +	hval = len;
> +	count = len;
> +	while (count-- > 0) {
> +		hval <<= 4;
> +		hval += name[count];
> +	}
> +
> +	/*
> +	 * First hash function:
> +	 * simply take the modulo but prevent zero.
> +	 */
> +	hval %= htab->size;
> +	if (hval == 0)
> +		++hval;
> +
> +	/* The first index tried. */
> +	new = -1; /* not found */
> +	idx = hval;
> +
> +	if (htab->table[idx].used) {
> +		/*
> +		 * Further action might be required according to the
> +		 * action value.
> +		 */
> +		unsigned int hval2;
> +
> +		if (htab->table[idx].used == hval &&
> +		    !efi_cmpkey(&htab->table[idx], name, vendor)) {
> +			if (action == FIND) {
> +				*retval = &htab->table[idx];
> +				return idx;
> +			}
> +			/* we don't need to support overwrite */
> +			return -1;
> +		}
> +
> +		/*
> +		 * Second hash function:
> +		 * as suggested in [Knuth]
> +		 */
> +		hval2 = 1 + hval % (htab->size - 2);
> +
> +		do {
> +			/*
> +			 * Because SIZE is prime this guarantees to
> +			 * step through all available indices.
> +			 */
> +			if (idx <= hval2)
> +				idx = htab->size + idx - hval2;
> +			else
> +				idx -= hval2;
> +
> +			/*
> +			 * If we visited all entries leave the loop
> +			 * unsuccessfully.
> +			 */
> +			if (idx == hval)
> +				break;
> +
> +			/* If entry is found use it. */
> +			if (htab->table[idx].used == hval &&
> +			    !efi_cmpkey(&htab->table[idx], name, vendor)) {
> +				if (action == FIND) {
> +					*retval = &htab->table[idx];
> +					return idx;
> +				}
> +				/* we don't need to support overwrite */
> +				return -1;
> +			}
> +		} while (htab->table[idx].used);
> +
> +		if (!htab->table[idx].used)
> +			new = idx;
> +	} else {
> +		new = idx;
> +	}
> +
> +	/*
> +	 * An empty bucket has been found.
> +	 * The following code should never be executed after
> +	 * exit_boot_services()
> +	 */
> +	if (action == ENTER) {
> +		/*
> +		 * If table is full and another entry should be
> +		 * entered return with error.
> +		 */
> +		if (htab->filled == htab->size) {
> +			*retval = NULL;
> +			return 0;
> +		}
> +
> +		/* Create new entry */
> +		htab->table[new].used = hval;
> +		++htab->filled;
> +
> +		/* return new entry */
> +		*retval = &htab->table[new];
> +		return 1;
> +	}
> +
> +	*retval = NULL;
> +	return 0;
> +}
> +
> +/* from lib/hashtable.c */
> +static inline int isprime(unsigned int number)
> +{
> +	/* no even number will be passed */
> +	unsigned int div = 3;
> +
> +	while (div * div < number && number % div != 0)
> +		div += 2;
> +
> +	return number % div != 0;
> +}
> +
> +efi_status_t efi_freeze_variable_table(void)

Please, add comments to your functions. It is not self-evident what this
function is meant to do.

I cannot imagine why a variable cache should be frozen. It is a living
data structure until the system is switched off.

For a variable cache I expect that you allocate memory before handling
the first variable and never again. At runtime you will not have chance
to allocate memory anyway.

This function is way too long. Pleae, break it down.

Best regards

Heinrich

> +{
> +	int var_num = 0;
> +	size_t var_data_size = 0;
> +	u16 *name;
> +	efi_uintn_t name_buf_len, name_len;
> +	efi_guid_t vendor;
> +	u32 attributes;
> +	u8 *mem_pool, *var_buf = NULL;
> +	size_t table_size, var_size, var_buf_size;
> +	_ENTRY *new = NULL;
> +	efi_status_t ret;
> +
> +	/* phase-1 loop */
> +	name_buf_len = 128;
> +	name = malloc(name_buf_len);
> +	if (!name)
> +		return EFI_OUT_OF_RESOURCES;
> +	name[0] = 0;
> +	for (;;) {
> +		name_len = name_buf_len;
> +		ret = EFI_CALL(efi_get_next_variable_name(&name_len, name,
> +							  &vendor));
> +		if (ret == EFI_NOT_FOUND) {
> +			break;
> +		} else if (ret == EFI_BUFFER_TOO_SMALL) {
> +			u16 *buf;
> +
> +			name_buf_len = name_len;
> +			buf = realloc(name, name_buf_len);
> +			if (!buf) {
> +				free(name);
> +				return EFI_OUT_OF_RESOURCES;
> +			}
> +			name = buf;
> +			name_len = name_buf_len;
> +			ret = EFI_CALL(efi_get_next_variable_name(&name_len,
> +								  name,
> +								  &vendor));
> +		}
> +
> +		if (ret != EFI_SUCCESS)
> +			return ret;
> +
> +		var_size = 0;
> +		ret = EFI_CALL(efi_get_variable(name, &vendor, &attributes,
> +						&var_size, NULL));
> +		if (ret != EFI_BUFFER_TOO_SMALL)
> +			return ret;
> +
> +		if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
> +			continue;
> +
> +		var_num++;
> +		var_data_size += (u16_strlen_runtime(name) + 1) * sizeof(u16);
> +		var_data_size += var_size;
> +		/* mem_pool must 2-byte aligned for u16 variable name */
> +		if (var_data_size & 0x1)
> +			var_data_size++;
> +	}
> +
> +	/*
> +	 * total of entries in hash table must be a prime number.
> +	 * The logic below comes from lib/hashtable.c
> +	 */
> +	var_num |= 1;               /* make odd */
> +	while (!isprime(var_num))
> +		var_num += 2;
> +
> +	/* We need table[var_num] for hsearch_runtime algo */
> +	table_size = sizeof(*efi_variable_table)
> +			+ sizeof(_ENTRY) * (var_num + 1) + var_data_size;
> +	ret = efi_allocate_pool(EFI_RUNTIME_SERVICES_DATA,
> +				table_size, (void **)&efi_variable_table);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	efi_variable_table->size = var_num;
> +	efi_variable_table->table = (void *)efi_variable_table
> +					+ sizeof(*efi_variable_table);
> +	mem_pool = (u8 *)efi_variable_table->table
> +			+ sizeof(_ENTRY) * (var_num + 1);
> +
> +	var_buf_size = 128;
> +	var_buf = malloc(var_buf_size);
> +	if (!var_buf) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto err;
> +	}
> +
> +	/* phase-2 loop */
> +	name[0] = 0;
> +	name_len = name_buf_len;
> +	for (;;) {
> +		name_len = name_buf_len;
> +		ret = EFI_CALL(efi_get_next_variable_name(&name_len, name,
> +							  &vendor));
> +		if (ret == EFI_NOT_FOUND)
> +			break;
> +		else if (ret != EFI_SUCCESS)
> +			goto err;
> +
> +		var_size = var_buf_size;
> +		ret = EFI_CALL(efi_get_variable(name, &vendor, &attributes,
> +						&var_size, var_buf));
> +		if (ret == EFI_BUFFER_TOO_SMALL) {
> +			free(var_buf);
> +			var_buf_size = var_size;
> +			var_buf = malloc(var_buf_size);
> +			if (!var_buf) {
> +				ret = EFI_OUT_OF_RESOURCES;
> +				goto err;
> +			}
> +			ret = EFI_CALL(efi_get_variable(name, &vendor,
> +							&attributes,
> +							&var_size, var_buf));
> +		}
> +		if (ret != EFI_SUCCESS)
> +			goto err;
> +
> +		if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
> +			continue;
> +
> +		if (hsearch_runtime(name, &vendor, ENTER, &new,
> +				    efi_variable_table) <= 0) {
> +			/* This should not happen */
> +			ret = EFI_INVALID_PARAMETER;
> +			goto err;
> +		}
> +
> +		/* allocate space from RUNTIME DATA */
> +		name_len = (u16_strlen_runtime(name) + 1) * sizeof(u16);
> +		memcpy_runtime(mem_pool, name, name_len);
> +		new->name = mem_pool - (u8 *)new; /* offset */
> +		mem_pool += name_len;
> +
> +		memcpy_runtime(&new->vendor.b, &vendor.b, sizeof(vendor));
> +
> +		new->attributes = attributes;
> +
> +		memcpy_runtime(mem_pool, var_buf, var_size);
> +		new->data = mem_pool - (u8 *)new; /* offset */
> +		new->data_size = var_size;
> +		mem_pool += var_size;
> +
> +		/* mem_pool must 2-byte aligned for u16 variable name */
> +		if ((uintptr_t)mem_pool & 0x1)
> +			mem_pool++;
> +	}
> +#ifdef DEBUG
> +	name[0] = 0;
> +	name_len = name_buf_len;
> +	for (;;) {
> +		name_len = name_buf_len;
> +		ret = efi_get_next_variable_name_runtime(&name_len, name,
> +							 &vendor);
> +		if (ret == EFI_NOT_FOUND)
> +			break;
> +		else if (ret != EFI_SUCCESS)
> +			goto err;
> +
> +		var_size = var_buf_size;
> +		ret = efi_get_variable_runtime(name, &vendor, &attributes,
> +					       &var_size, var_buf);
> +		if (ret != EFI_SUCCESS)
> +			goto err;
> +
> +		printf("%ls_%pUl:\n", name, &vendor);
> +		printf("    attributes: 0x%x\n", attributes);
> +		printf("    value (size: 0x%lx)\n", var_size);
> +	}
> +#endif
> +	ret = EFI_SUCCESS;
> +
> +err:
> +	free(name);
> +	free(var_buf);
> +	if (ret != EFI_SUCCESS && efi_variable_table) {
> +		efi_free_pool(efi_variable_table);
> +		efi_variable_table = NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +efi_status_t
> +__efi_runtime EFIAPI efi_get_variable_runtime(u16 *variable_name,
> +					      const efi_guid_t *vendor,
> +					      u32 *attributes,
> +					      efi_uintn_t *data_size,
> +					      void *data)
> +{
> +	_ENTRY *new;
> +
> +	if (!variable_name || !vendor || !data_size)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	if (hsearch_runtime(variable_name, vendor, FIND, &new,
> +			    efi_variable_table) <= 0)
> +		return EFI_NOT_FOUND;
> +
> +	if (attributes)
> +		*attributes = new->attributes;
> +	if (*data_size < new->data_size) {
> +		*data_size = new->data_size;
> +		return EFI_BUFFER_TOO_SMALL;
> +	}
> +
> +	*data_size = new->data_size;
> +	memcpy_runtime(data, entry_data(new), new->data_size);
> +
> +	return EFI_SUCCESS;
> +}
> +
> +static int prev_idx __efi_runtime_data;
> +
> +efi_status_t
> +__efi_runtime EFIAPI efi_get_next_variable_name_runtime(
> +						efi_uintn_t *variable_name_size,
> +						u16 *variable_name,
> +						const efi_guid_t *vendor)
> +{
> +	_ENTRY *e;
> +	u16 *name;
> +	efi_uintn_t name_size;
> +
> +	if (!variable_name_size || !variable_name || !vendor)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (variable_name[0]) {
> +		/* sanity check for previous variable */
> +		if (prev_idx < 0)
> +			return EFI_INVALID_PARAMETER;
> +
> +		e = &efi_variable_table->table[prev_idx];
> +		if (!e->used || efi_cmpkey(e, variable_name, vendor))
> +			return EFI_INVALID_PARAMETER;
> +	} else {
> +		prev_idx = -1;
> +	}
> +
> +	/* next variable */
> +	while (++prev_idx <= efi_variable_table->size) {
> +		e = &efi_variable_table->table[prev_idx];
> +		if (e->used)
> +			break;
> +	}
> +	if (prev_idx > efi_variable_table->size)
> +		return EFI_NOT_FOUND;
> +
> +	name = entry_name(e);
> +	name_size = (u16_strlen_runtime(name) + 1)
> +			* sizeof(u16);
> +	if (*variable_name_size < name_size) {
> +		*variable_name_size = name_size;
> +		return EFI_BUFFER_TOO_SMALL;
> +	}
> +
> +	memcpy_runtime(variable_name, name, name_size);
> +	memcpy_runtime((void *)&vendor->b, &e->vendor.b, sizeof(vendor));
> +
> +	return EFI_SUCCESS;
> +}
> +#endif /* CONFIG_EFI_RUNTIME_GET_VARIABLE_CACHING */
>



More information about the U-Boot mailing list