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

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@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(-)

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@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 */
>

On Sat, Jun 15, 2019 at 09:01:56PM +0200, Heinrich Schuchardt wrote:
> 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@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.

Why?

> >
> >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.

So do you mean that we should replace the existing "boottime" version
of get/set_variable with my code (algorithm)?

This is a bit complicated work because we should be able to *udpate*
UEFI variables at boottime, but my version of hsearch_runtime() is
a stripped (and modified) version and doesn't support it.

Making the existing hsearch_r() executable at UEFI runtime is,
as I said before, quite painful.

> 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.

Unfortunately, this is not practical right now because there is
already some sort of assumption (and consensus) that we would re-use
"Standalone MM services", which is already there in EDK2, as
secure storage for UEFI variables.
In the case, all the cache would be bypassed.
In my old prototype, I utilized the cache but dropped that feature
for several reasons.

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

I'm not sure your intent here, but are you going to write back
the cache only once?
It won't work as every change of UEFI variable must be flushed
to persistent storage instantly.

> >+					      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.

Why "should"?
Give me any use case.
UEFI spec does not describe such a variant implementation at all.

> >+
> >  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.

What is the difference between yours and my env_efi_load/save()
with backing-storage driver?

> 
> >+	/* 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.

We should do that if possible, but please note that [str|mem]xxx() functions
are *architecture* dependent.
Do you want to mark all the functions across all the architectures?

> >+{
> >+	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?

ditto

> >+{
> >+	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.

Do you know that there are no any tests for hsearch_r()?
Moreover, this function would better be exercised well
if we could add *runtime* tests.

> 
> >+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?

efi_cmpkey() does take into consideration GUID as well as the name.

> >+	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.

I don't get your point.

> 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.

I don't get your point.

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

Freezing is implemented in 2-phase steps, and some complexity
is inevitable. I suppose adding some comments would be enough.

-Takahiro Akashi

> 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 */
> >
>

On 6/17/19 3:51 AM, AKASHI Takahiro wrote:
> On Sat, Jun 15, 2019 at 09:01:56PM +0200, Heinrich Schuchardt wrote:
>> 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@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.
>
> Why?

It is a separate set of functions. In C++ programming you wouldn't put
two classes into the same 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.
>
> So do you mean that we should replace the existing "boottime" version
> of get/set_variable with my code (algorithm)?
>
> This is a bit complicated work because we should be able to *udpate*
> UEFI variables at boottime, but my version of hsearch_runtime() is
> a stripped (and modified) version and doesn't support it.

Do we really need a multilevel hash table? I would not expect hundreds
of variables.

>
> Making the existing hsearch_r() executable at UEFI runtime is,
> as I said before, quite painful.

You could start the cache implementation with a less complicated data
structure like a linked list.

>
>> 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.
>
> Unfortunately, this is not practical right now because there is
> already some sort of assumption (and consensus) that we would re-use
> "Standalone MM services", which is already there in EDK2, as
> secure storage for UEFI variables.
> In the case, all the cache would be bypassed.
> In my old prototype, I utilized the cache but dropped that feature
> for several reasons.

What has EDK2 code to do with it?

In case of write you could do a write-through in your cache if needed.

>
>> The NV-cache content should be written to non-volatile memory on Reset()
>> and on ExitBootServices() and if possible when updating variables at
>> runtime.
>
> I'm not sure your intent here, but are you going to write back
> the cache only once?
> It won't work as every change of UEFI variable must be flushed
> to persistent storage instantly.

The cache should support write and read. Only NV variables have to be
written to a medium. If you do not support this currently just return
some error code vor NV variables. But you could accept still accept
changes to non-NV variables. This way we can test the code at runtime
even before implementing runtime persistence.

>
>>> +					      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.
>
> Why "should"?
> Give me any use case.
> UEFI spec does not describe such a variant implementation at all.

See above. I would like to be able to test setting variables at runtime
even if persisting NV variables is not yet implemented.

>
>>> +
>>>   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.
>
> What is the difference between yours and my env_efi_load/save()
> with backing-storage driver?

I cannot see that you clearly separate the functions API, cache,
persistence.

I would like to see clearly define interfaces into which we can plug
different persistence implementations.

>
>>
>>> +	/* 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.
>
> We should do that if possible, but please note that [str|mem]xxx() functions
> are *architecture* dependent.
> Do you want to mark all the functions across all the architectures?
>
>>> +{
>>> +	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?
>
> ditto
>
>>> +{
>>> +	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.
>
> Do you know that there are no any tests for hsearch_r()?
> Moreover, this function would better be exercised well
> if we could add *runtime* tests.

Have a look at test/env/hashtable.c

I think efi_selftest could work after SetVirtualMemoryMap if we do not
change the map for memory actually used by U-Boot.

Or we use a minimum Linux kernel and put our tests into an init binary.

>
>>
>>> +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?
>
> efi_cmpkey() does take into consideration GUID as well as the name.

My question concerned 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.
>
> I don't get your point.

Please, explain what you mean by freeze.

I suggest that the cache is read/write at all times.

Best regards

Heinrich

>
>> 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.
>
> I don't get your point.
>
>> This function is way too long. Pleae, break it down.
>
> Freezing is implemented in 2-phase steps, and some complexity
> is inevitable. I suppose adding some comments would be enough.
>
> -Takahiro Akashi
>
>> 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 */
>>>
>>
>

On Mon, Jun 17, 2019 at 09:52:34PM +0200, Heinrich Schuchardt wrote:
> On 6/17/19 3:51 AM, AKASHI Takahiro wrote:
> >On Sat, Jun 15, 2019 at 09:01:56PM +0200, Heinrich Schuchardt wrote:
> >>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@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.
> >
> >Why?
> 
> It is a separate set of functions. In C++ programming you wouldn't put
> two classes into the same file.

? I don't get your point.
This is not C++, but C.

> >
> >>>
> >>>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.
> >
> >So do you mean that we should replace the existing "boottime" version
> >of get/set_variable with my code (algorithm)?
> >
> >This is a bit complicated work because we should be able to *udpate*
> >UEFI variables at boottime, but my version of hsearch_runtime() is
> >a stripped (and modified) version and doesn't support it.
> 
> Do we really need a multilevel hash table? I would not expect hundreds
> of variables.

Please don't change your point suddenly.
Here we are discussing whether "The cache should be used both at runtime
and at boottime" or not.

> >
> >Making the existing hsearch_r() executable at UEFI runtime is,
> >as I said before, quite painful.
> 
> You could start the cache implementation with a less complicated data
> structure like a linked list.

This is totally a different issue. I listed this issue
in my cover letter.

> >
> >>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.
> >
> >Unfortunately, this is not practical right now because there is
> >already some sort of assumption (and consensus) that we would re-use
> >"Standalone MM services", which is already there in EDK2, as
> >secure storage for UEFI variables.
> >In the case, all the cache would be bypassed.
> >In my old prototype, I utilized the cache but dropped that feature
> >for several reasons.
> 
> What has EDK2 code to do with it?

Did you follow my comment below?
> >Unfortunately, this is not practical right now because there is
> >already some sort of assumption (and consensus) that we would re-use
> >"Standalone MM services", which is already there in EDK2, as
> >secure storage for UEFI variables.

> In case of write you could do a write-through in your cache if needed.
> 
> >
> >>The NV-cache content should be written to non-volatile memory on Reset()
> >>and on ExitBootServices() and if possible when updating variables at
> >>runtime.
> >
> >I'm not sure your intent here, but are you going to write back
> >the cache only once?
> >It won't work as every change of UEFI variable must be flushed
> >to persistent storage instantly.
> 
> The cache should support write and read. Only NV variables have to be

Why?
I don't think it make sense that we support volatile variable, but not
non-volatile variable at runtime.
Even UEFI specification doesn't describe such an irregular behavior.
See EFI_RT_SUPPORTED_XXX definitions.
Do you have a meaningful use case that you want to support?

> written to a medium. If you do not support this currently just return
> some error code vor NV variables. But you could accept still accept
> changes to non-NV variables. This way we can test the code at runtime
> even before implementing runtime persistence.
> 
> >
> >>>+					      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.
> >
> >Why "should"?
> >Give me any use case.
> >UEFI spec does not describe such a variant implementation at all.
> 
> See above. I would like to be able to test setting variables at runtime
> even if persisting NV variables is not yet implemented.

Just for testing? No real use case?
Please note that if we *have* persistent storage at runtime,
we don't need cache because we can access that storage.

> >
> >>>+
> >>>  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.
> >
> >What is the difference between yours and my env_efi_load/save()
> >with backing-storage driver?
> 
> I cannot see that you clearly separate the functions API, cache,
> persistence.

I don't get your point.
For instance, efi_boottime.c has a lot of different type of functions.
Do you want to separate them into different files?

> I would like to see clearly define interfaces into which we can plug
> different persistence implementations.

Your assertion doesn't clarify why you want to separate API and cache.

Regarding persistent storage, as I said, efi_[get|set]_variable()
will be completely replaced with the corresponding functions
(at least, in case of Standalone MM services).


> >
> >>
> >>>+	/* 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.
> >
> >We should do that if possible, but please note that [str|mem]xxx() functions
> >are *architecture* dependent.
> >Do you want to mark all the functions across all the architectures?

Agree or not agree?


> >>>+{
> >>>+	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?
> >
> >ditto
> >
> >>>+{
> >>>+	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.
> >
> >Do you know that there are no any tests for hsearch_r()?
> >Moreover, this function would better be exercised well
> >if we could add *runtime* tests.
> 
> Have a look at test/env/hashtable.c

Okay, but why not re-use this test for my stripped version?

> I think efi_selftest could work after SetVirtualMemoryMap if we do not
> change the map for memory actually used by U-Boot.

Maybe.

-Takahiro Akashi

> Or we use a minimum Linux kernel and put our tests into an init binary.
> 
> >
> >>
> >>>+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?
> >
> >efi_cmpkey() does take into consideration GUID as well as the name.
> 
> My question concerned 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.
> >
> >I don't get your point.
> 
> Please, explain what you mean by freeze.
> 
> I suggest that the cache is read/write at all times.
> 
> Best regards
> 
> Heinrich
> 
> >
> >>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.
> >
> >I don't get your point.
> >
> >>This function is way too long. Pleae, break it down.
> >
> >Freezing is implemented in 2-phase steps, and some complexity
> >is inevitable. I suppose adding some comments would be enough.
> >
> >-Takahiro Akashi
> >
> >>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 */
> >>>
> >>
> >
>

Heinrich,

On Tue, Jun 18, 2019 at 10:19:06AM +0900, AKASHI Takahiro wrote:
> On Mon, Jun 17, 2019 at 09:52:34PM +0200, Heinrich Schuchardt wrote:
> > On 6/17/19 3:51 AM, AKASHI Takahiro wrote:
> > >On Sat, Jun 15, 2019 at 09:01:56PM +0200, Heinrich Schuchardt wrote:
> > >>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@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.
> > >
> > >Why?
> > 
> > It is a separate set of functions. In C++ programming you wouldn't put
> > two classes into the same file.
> 
> ? I don't get your point.
> This is not C++, but C.
> 
> > >
> > >>>
> > >>>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.
> > >
> > >So do you mean that we should replace the existing "boottime" version
> > >of get/set_variable with my code (algorithm)?
> > >
> > >This is a bit complicated work because we should be able to *udpate*
> > >UEFI variables at boottime, but my version of hsearch_runtime() is
> > >a stripped (and modified) version and doesn't support it.
> > 
> > Do we really need a multilevel hash table? I would not expect hundreds
> > of variables.
> 
> Please don't change your point suddenly.
> Here we are discussing whether "The cache should be used both at runtime
> and at boottime" or not.
> 
> > >
> > >Making the existing hsearch_r() executable at UEFI runtime is,
> > >as I said before, quite painful.
> > 
> > You could start the cache implementation with a less complicated data
> > structure like a linked list.
> 
> This is totally a different issue. I listed this issue
> in my cover letter.
> 
> > >
> > >>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.
> > >
> > >Unfortunately, this is not practical right now because there is
> > >already some sort of assumption (and consensus) that we would re-use
> > >"Standalone MM services", which is already there in EDK2, as
> > >secure storage for UEFI variables.
> > >In the case, all the cache would be bypassed.
> > >In my old prototype, I utilized the cache but dropped that feature
> > >for several reasons.
> > 
> > What has EDK2 code to do with it?
> 
> Did you follow my comment below?
> > >Unfortunately, this is not practical right now because there is
> > >already some sort of assumption (and consensus) that we would re-use
> > >"Standalone MM services", which is already there in EDK2, as
> > >secure storage for UEFI variables.

Let me elaborate more:
My tentative "secure boot" patch is self-contained on non-secure side
and provides the functionality that UEFI specification describes, but
there is one missing and essential aspect: Tamper-resistant.
Standalone MM services is expected to provides this semantics with
the same interfaces as [Get|Set]Variable for both volatile and
non-volatile variables alike.
There can be another implementation (with yet another interfaces to
secure side), but it is currently the only implementation available
as far as I know.

-Takahiro Akashi


> > In case of write you could do a write-through in your cache if needed.
> > 
> > >
> > >>The NV-cache content should be written to non-volatile memory on Reset()
> > >>and on ExitBootServices() and if possible when updating variables at
> > >>runtime.
> > >
> > >I'm not sure your intent here, but are you going to write back
> > >the cache only once?
> > >It won't work as every change of UEFI variable must be flushed
> > >to persistent storage instantly.
> > 
> > The cache should support write and read. Only NV variables have to be
> 
> Why?
> I don't think it make sense that we support volatile variable, but not
> non-volatile variable at runtime.
> Even UEFI specification doesn't describe such an irregular behavior.
> See EFI_RT_SUPPORTED_XXX definitions.
> Do you have a meaningful use case that you want to support?
> 
> > written to a medium. If you do not support this currently just return
> > some error code vor NV variables. But you could accept still accept
> > changes to non-NV variables. This way we can test the code at runtime
> > even before implementing runtime persistence.
> > 
> > >
> > >>>+					      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.
> > >
> > >Why "should"?
> > >Give me any use case.
> > >UEFI spec does not describe such a variant implementation at all.
> > 
> > See above. I would like to be able to test setting variables at runtime
> > even if persisting NV variables is not yet implemented.
> 
> Just for testing? No real use case?
> Please note that if we *have* persistent storage at runtime,
> we don't need cache because we can access that storage.
> 
> > >
> > >>>+
> > >>>  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.
> > >
> > >What is the difference between yours and my env_efi_load/save()
> > >with backing-storage driver?
> > 
> > I cannot see that you clearly separate the functions API, cache,
> > persistence.
> 
> I don't get your point.
> For instance, efi_boottime.c has a lot of different type of functions.
> Do you want to separate them into different files?
> 
> > I would like to see clearly define interfaces into which we can plug
> > different persistence implementations.
> 
> Your assertion doesn't clarify why you want to separate API and cache.
> 
> Regarding persistent storage, as I said, efi_[get|set]_variable()
> will be completely replaced with the corresponding functions
> (at least, in case of Standalone MM services).
> 
> 
> > >
> > >>
> > >>>+	/* 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.
> > >
> > >We should do that if possible, but please note that [str|mem]xxx() functions
> > >are *architecture* dependent.
> > >Do you want to mark all the functions across all the architectures?
> 
> Agree or not agree?
> 
> 
> > >>>+{
> > >>>+	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?
> > >
> > >ditto
> > >
> > >>>+{
> > >>>+	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.
> > >
> > >Do you know that there are no any tests for hsearch_r()?
> > >Moreover, this function would better be exercised well
> > >if we could add *runtime* tests.
> > 
> > Have a look at test/env/hashtable.c
> 
> Okay, but why not re-use this test for my stripped version?
> 
> > I think efi_selftest could work after SetVirtualMemoryMap if we do not
> > change the map for memory actually used by U-Boot.
> 
> Maybe.
> 
> -Takahiro Akashi
> 
> > Or we use a minimum Linux kernel and put our tests into an init binary.
> > 
> > >
> > >>
> > >>>+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?
> > >
> > >efi_cmpkey() does take into consideration GUID as well as the name.
> > 
> > My question concerned 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.
> > >
> > >I don't get your point.
> > 
> > Please, explain what you mean by freeze.
> > 
> > I suggest that the cache is read/write at all times.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > >>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.
> > >
> > >I don't get your point.
> > >
> > >>This function is way too long. Pleae, break it down.
> > >
> > >Freezing is implemented in 2-phase steps, and some complexity
> > >is inevitable. I suppose adding some comments would be enough.
> > >
> > >-Takahiro Akashi
> > >
> > >>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 */
> > >>>
> > >>
> > >
> >

Hi all,

[...]
> > >>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.
> > >
> > >So do you mean that we should replace the existing "boottime" version
> > >of get/set_variable with my code (algorithm)?
> > >
> > >This is a bit complicated work because we should be able to *udpate*
> > >UEFI variables at boottime, but my version of hsearch_runtime() is
> > >a stripped (and modified) version and doesn't support it.
> > 
> > Do we really need a multilevel hash table? I would not expect hundreds
> > of variables.
> 
> Please don't change your point suddenly.
> Here we are discussing whether "The cache should be used both at runtime
> and at boottime" or not.
> 
Heinrich, the idea here is to present a copy of the variables on the OS
and totally disable RT variable updating from the OS.
If someone wants to update UEFI variables, we can pack them to a Capsule
(using FIT image format) and apply them on next reboot.
Given the fact that UEFI variables are not updated that often, isn't this a
viable option? If it is, then we need to keep the access separated
(as Akashi-san suggests) allowing bootime to change the variables.

> > >
> > >Making the existing hsearch_r() executable at UEFI runtime is,
> > >as I said before, quite painful.
> > 
> > You could start the cache implementation with a less complicated data
> > structure like a linked list.
> 
> This is totally a different issue. I listed this issue
> in my cover letter.
> 
> > >
> > >>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.
> > >
> > >Unfortunately, this is not practical right now because there is
> > >already some sort of assumption (and consensus) that we would re-use
> > >"Standalone MM services", which is already there in EDK2, as
> > >secure storage for UEFI variables.
> > >In the case, all the cache would be bypassed.
> > >In my old prototype, I utilized the cache but dropped that feature
> > >for several reasons.
> > 
> > What has EDK2 code to do with it?
> 
> Did you follow my comment below?
> > >Unfortunately, this is not practical right now because there is
> > >already some sort of assumption (and consensus) that we would re-use
> > >"Standalone MM services", which is already there in EDK2, as
> > >secure storage for UEFI variables.
We are already working towards having StandAloneMM as an early OP-TEE TA.
This will provide us with a secure variable storage for armv7/v8.

> 
> > In case of write you could do a write-through in your cache if needed.
> > 
> > >
> > >>The NV-cache content should be written to non-volatile memory on Reset()
> > >>and on ExitBootServices() and if possible when updating variables at
> > >>runtime.
> > >
> > >I'm not sure your intent here, but are you going to write back
> > >the cache only once?
> > >It won't work as every change of UEFI variable must be flushed
> > >to persistent storage instantly.
> > 
> > >


Thanks
/Ilias

On 6/18/19 12:34 PM, Ilias Apalodimas wrote:
> Hi all,
>
> [...]
>>>>> 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.
>>>>
>>>> So do you mean that we should replace the existing "boottime" version
>>>> of get/set_variable with my code (algorithm)?
>>>>
>>>> This is a bit complicated work because we should be able to *udpate*
>>>> UEFI variables at boottime, but my version of hsearch_runtime() is
>>>> a stripped (and modified) version and doesn't support it.
>>>
>>> Do we really need a multilevel hash table? I would not expect hundreds
>>> of variables.
>>
>> Please don't change your point suddenly.
>> Here we are discussing whether "The cache should be used both at runtime
>> and at boottime" or not.
>>
> Heinrich, the idea here is to present a copy of the variables on the OS
> and totally disable RT variable updating from the OS.
> If someone wants to update UEFI variables, we can pack them to a Capsule
> (using FIT image format) and apply them on next reboot.
> Given the fact that UEFI variables are not updated that often, isn't this a
> viable option? If it is, then we need to keep the access separated
> (as Akashi-san suggests) allowing bootime to change the variables.
>
>>>>
>>>> Making the existing hsearch_r() executable at UEFI runtime is,
>>>> as I said before, quite painful.
>>>
>>> You could start the cache implementation with a less complicated data
>>> structure like a linked list.
>>
>> This is totally a different issue. I listed this issue
>> in my cover letter.
>>
>>>>
>>>>> 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.
>>>>
>>>> Unfortunately, this is not practical right now because there is
>>>> already some sort of assumption (and consensus) that we would re-use
>>>> "Standalone MM services", which is already there in EDK2, as
>>>> secure storage for UEFI variables.
>>>> In the case, all the cache would be bypassed.
>>>> In my old prototype, I utilized the cache but dropped that feature
>>>> for several reasons.
>>>
>>> What has EDK2 code to do with it?
>>
>> Did you follow my comment below?
>>>> Unfortunately, this is not practical right now because there is
>>>> already some sort of assumption (and consensus) that we would re-use
>>>> "Standalone MM services", which is already there in EDK2, as
>>>> secure storage for UEFI variables.
> We are already working towards having StandAloneMM as an early OP-TEE TA.
> This will provide us with a secure variable storage for armv7/v8.

What would this OP-TEE binary do? - This seems to be a source of
misunderstanding when reviewing this patch.

My guess is that OP-TEE is used to read non-volatile variables only once
when starting U-Boot and to write non-volatile variables whenever they
are changed.

All further reading of non-volatile variables and all access to volatile
variables will be handled by the U-Boot internal variable cache.

For volatile variables I would assume OP-TEE to never see them. This
requires that the U-Boot variable cachek supports reading from and
writing to the cache at runtime.

StandaloneMmPkg seems to be the hardware independent part of the
solution. Where will the hardware driver reside in your OP-TEE solution?

Is the EDK2 hardware store for variables of the MACCHIATObin here:
edk2-platforms/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c?

Which hardware platform will you use for testing the U-Boot development
of you OP-TEE driver?

Best regards

Heinrich

>
>>
>>> In case of write you could do a write-through in your cache if needed.
>>>
>>>>
>>>>> The NV-cache content should be written to non-volatile memory on Reset()
>>>>> and on ExitBootServices() and if possible when updating variables at
>>>>> runtime.
>>>>
>>>> I'm not sure your intent here, but are you going to write back
>>>> the cache only once?
>>>> It won't work as every change of UEFI variable must be flushed
>>>> to persistent storage instantly.
>>>
>>>>
>
>
> Thanks
> /Ilias
>

Heinrich,

On Tue, Jun 18, 2019 at 10:45:26PM +0200, Heinrich Schuchardt wrote:
> On 6/18/19 12:34 PM, Ilias Apalodimas wrote:
> >Hi all,
> >
> >[...]
> >>>>>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.
> >>>>
> >>>>So do you mean that we should replace the existing "boottime" version
> >>>>of get/set_variable with my code (algorithm)?
> >>>>
> >>>>This is a bit complicated work because we should be able to *udpate*
> >>>>UEFI variables at boottime, but my version of hsearch_runtime() is
> >>>>a stripped (and modified) version and doesn't support it.
> >>>
> >>>Do we really need a multilevel hash table? I would not expect hundreds
> >>>of variables.
> >>
> >>Please don't change your point suddenly.
> >>Here we are discussing whether "The cache should be used both at runtime
> >>and at boottime" or not.
> >>
> >Heinrich, the idea here is to present a copy of the variables on the OS
> >and totally disable RT variable updating from the OS.
> >If someone wants to update UEFI variables, we can pack them to a Capsule
> >(using FIT image format) and apply them on next reboot.
> >Given the fact that UEFI variables are not updated that often, isn't this a
> >viable option? If it is, then we need to keep the access separated
> >(as Akashi-san suggests) allowing bootime to change the variables.
> >
> >>>>
> >>>>Making the existing hsearch_r() executable at UEFI runtime is,
> >>>>as I said before, quite painful.
> >>>
> >>>You could start the cache implementation with a less complicated data
> >>>structure like a linked list.
> >>
> >>This is totally a different issue. I listed this issue
> >>in my cover letter.
> >>
> >>>>
> >>>>>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.
> >>>>
> >>>>Unfortunately, this is not practical right now because there is
> >>>>already some sort of assumption (and consensus) that we would re-use
> >>>>"Standalone MM services", which is already there in EDK2, as
> >>>>secure storage for UEFI variables.
> >>>>In the case, all the cache would be bypassed.
> >>>>In my old prototype, I utilized the cache but dropped that feature
> >>>>for several reasons.
> >>>
> >>>What has EDK2 code to do with it?
> >>
> >>Did you follow my comment below?
> >>>>Unfortunately, this is not practical right now because there is
> >>>>already some sort of assumption (and consensus) that we would re-use
> >>>>"Standalone MM services", which is already there in EDK2, as
> >>>>secure storage for UEFI variables.
> >We are already working towards having StandAloneMM as an early OP-TEE TA.
> >This will provide us with a secure variable storage for armv7/v8.
> 
> What would this OP-TEE binary do? - This seems to be a source of
> misunderstanding when reviewing this patch.

I and Ilias will give you more details offline, here's a short(?)
answer:

Standalone MM services here means a SPD entity which provides
[Get|Set]Variable APIs to non-secure side firmware, that is
currently EDK2. So the source code of Standalone MM services
is included in EDK2 repository as a matter of fact.

Here is one drawback: It won't allow for other entities running
concurrently on secure side. One example of useful secure feature
is (software-based) TPM. So Linaro is working on modifying/transforming 
Standalone MM to one OP-TEE application, which Ilias mentioned above.

> My guess is that OP-TEE is used to read non-volatile variables only once
> when starting U-Boot and to write non-volatile variables whenever they
> are changed.

So OP-TEE version of StMM is still on-going project and I assume
that this OP-TEE app will support the same set of functionality/APIs
as StMM does.

> All further reading of non-volatile variables and all access to volatile
> variables will be handled by the U-Boot internal variable cache.
> 
> For volatile variables I would assume OP-TEE to never see them. This
> requires that the U-Boot variable cachek supports reading from and
> writing to the cache at runtime.

No. As far as I correctly understand, StMM handles volatile
variables as well as non-volatile variables.
EDK2 on non-secure side will redirect user's request directly
to secure side even without *caching* variable's values.

> StandaloneMmPkg seems to be the hardware independent part of the
> solution. Where will the hardware driver reside in your OP-TEE solution?
> 
> Is the EDK2 hardware store for variables of the MACCHIATObin here:
> edk2-platforms/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c?
> 
> Which hardware platform will you use for testing the U-Boot development
> of you OP-TEE driver?

Ilias will be able to answer those questions.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >>
> >>>In case of write you could do a write-through in your cache if needed.
> >>>
> >>>>
> >>>>>The NV-cache content should be written to non-volatile memory on Reset()
> >>>>>and on ExitBootServices() and if possible when updating variables at
> >>>>>runtime.
> >>>>
> >>>>I'm not sure your intent here, but are you going to write back
> >>>>the cache only once?
> >>>>It won't work as every change of UEFI variable must be flushed
> >>>>to persistent storage instantly.
> >>>
> >>>>
> >
> >
> >Thanks
> >/Ilias
> >
>

Heinrich,

[...]
> > >>>>Unfortunately, this is not practical right now because there is
> > >>>>already some sort of assumption (and consensus) that we would re-use
> > >>>>"Standalone MM services", which is already there in EDK2, as
> > >>>>secure storage for UEFI variables.
> > >>>>In the case, all the cache would be bypassed.
> > >>>>In my old prototype, I utilized the cache but dropped that feature
> > >>>>for several reasons.
> > >>>
> > >>>What has EDK2 code to do with it?
> > >>
> > >>Did you follow my comment below?
> > >>>>Unfortunately, this is not practical right now because there is
> > >>>>already some sort of assumption (and consensus) that we would re-use
> > >>>>"Standalone MM services", which is already there in EDK2, as
> > >>>>secure storage for UEFI variables.
> > >We are already working towards having StandAloneMM as an early OP-TEE TA.
> > >This will provide us with a secure variable storage for armv7/v8.
> > 
> > What would this OP-TEE binary do? - This seems to be a source of
> > misunderstanding when reviewing this patch.
> 
> I and Ilias will give you more details offline, here's a short(?)
> answer:
> 
> Standalone MM services here means a SPD entity which provides
> [Get|Set]Variable APIs to non-secure side firmware, that is
> currently EDK2. So the source code of Standalone MM services
> is included in EDK2 repository as a matter of fact.
> 
> Here is one drawback: It won't allow for other entities running
> concurrently on secure side. One example of useful secure feature
> is (software-based) TPM. So Linaro is working on modifying/transforming 
> Standalone MM to one OP-TEE application, which Ilias mentioned above.
> 
Exactly. The current StMM implementation exists for Armv8 *only* in SPM (Secure
Partition Manager). The idea is to make it an OP-TEE application, so we can run
it on on Armv7s as well. As Akashi-san mentions SPD (Secure Payload Dispatcher)
and SPM are mutually exclusive so having everything as OP-TEE trusted
applications gives us a number of advantages at the moment.

> > My guess is that OP-TEE is used to read non-volatile variables only once
> > when starting U-Boot and to write non-volatile variables whenever they
> > are changed.
> 
> So OP-TEE version of StMM is still on-going project and I assume
> that this OP-TEE app will support the same set of functionality/APIs
> as StMM does.
Yes that's the goal 

> 
> > All further reading of non-volatile variables and all access to volatile
> > variables will be handled by the U-Boot internal variable cache.
> > 
> > For volatile variables I would assume OP-TEE to never see them. This
> > requires that the U-Boot variable cachek supports reading from and
> > writing to the cache at runtime.
> 
> No. As far as I correctly understand, StMM handles volatile
> variables as well as non-volatile variables.
> EDK2 on non-secure side will redirect user's request directly
> to secure side even without *caching* variable's values.
> 
Similar understanding here. The question is, will we have to think of something
for non-arm architectures?

> > StandaloneMmPkg seems to be the hardware independent part of the
> > solution. Where will the hardware driver reside in your OP-TEE solution?
It depends on where your hardware is. If you have a NOR flash directly connected
to the secure world the answer is yes. 
For starters we are going to use RPMB + U-Boot supplicant.

> > 
> > Is the EDK2 hardware store for variables of the MACCHIATObin here:
> > edk2-platforms/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c?
No idea, i can ask around.

> > 
> > Which hardware platform will you use for testing the U-Boot development
> > of you OP-TEE driver?
> 
> Ilias will be able to answer those questions.
- stm32mp1 ST board based on armv7 [1]
- Socionext DeveloperBox for armv8 [2]. This has a running EDKII implementation
of StMM in SPM. The underlying firmware should be irrelevant though since the 
whole functionality is contained within an OP-TEE TA (trusted application). If i
remember correctly that will need an extra driver in OP-TEE (if we port U-Boot
on that)
- TI AM6 board [3]. I don't have the board in my hands yet, so no details on it


[1] https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html
[2] https://www.96boards.org/product/developerbox/
[3] http://www.ti.com/tool/PROCESSOR-SDK-AM65X 


Regards
/Ilias

On 6/19/19 7:13 AM, Ilias Apalodimas wrote:
> Heinrich,
>
> [...]
>>>>>>> Unfortunately, this is not practical right now because there is
>>>>>>> already some sort of assumption (and consensus) that we would re-use
>>>>>>> "Standalone MM services", which is already there in EDK2, as
>>>>>>> secure storage for UEFI variables.
>>>>>>> In the case, all the cache would be bypassed.
>>>>>>> In my old prototype, I utilized the cache but dropped that feature
>>>>>>> for several reasons.
>>>>>>
>>>>>> What has EDK2 code to do with it?
>>>>>
>>>>> Did you follow my comment below?
>>>>>>> Unfortunately, this is not practical right now because there is
>>>>>>> already some sort of assumption (and consensus) that we would re-use
>>>>>>> "Standalone MM services", which is already there in EDK2, as
>>>>>>> secure storage for UEFI variables.
>>>> We are already working towards having StandAloneMM as an early OP-TEE TA.
>>>> This will provide us with a secure variable storage for armv7/v8.
>>>
>>> What would this OP-TEE binary do? - This seems to be a source of
>>> misunderstanding when reviewing this patch.
>>
>> I and Ilias will give you more details offline, here's a short(?)
>> answer:
>>
>> Standalone MM services here means a SPD entity which provides
>> [Get|Set]Variable APIs to non-secure side firmware, that is
>> currently EDK2. So the source code of Standalone MM services
>> is included in EDK2 repository as a matter of fact.
>>
>> Here is one drawback: It won't allow for other entities running
>> concurrently on secure side. One example of useful secure feature
>> is (software-based) TPM. So Linaro is working on modifying/transforming
>> Standalone MM to one OP-TEE application, which Ilias mentioned above.
>>
> Exactly. The current StMM implementation exists for Armv8 *only* in SPM (Secure
> Partition Manager). The idea is to make it an OP-TEE application, so we can run
> it on on Armv7s as well. As Akashi-san mentions SPD (Secure Payload Dispatcher)
> and SPM are mutually exclusive so having everything as OP-TEE trusted
> applications gives us a number of advantages at the moment.
>
>>> My guess is that OP-TEE is used to read non-volatile variables only once
>>> when starting U-Boot and to write non-volatile variables whenever they
>>> are changed.
>>
>> So OP-TEE version of StMM is still on-going project and I assume
>> that this OP-TEE app will support the same set of functionality/APIs
>> as StMM does.
> Yes that's the goal

Do I understand it write:

In U-Boot we would have code that essentially provides the functionality
of EDK2's EFI_SMM_VARIABLE_PROTOCOL. Like
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
this would talk via SMI calls with the hardware drivers, in this case
the OP-TEE app.

This would allow the OP-TEE app to be used both in U-Boot and in EDK2?

>
>>
>>> All further reading of non-volatile variables and all access to volatile
>>> variables will be handled by the U-Boot internal variable cache.
>>>
>>> For volatile variables I would assume OP-TEE to never see them. This
>>> requires that the U-Boot variable cachek supports reading from and
>>> writing to the cache at runtime.
>>
>> No. As far as I correctly understand, StMM handles volatile
>> variables as well as non-volatile variables.
>> EDK2 on non-secure side will redirect user's request directly
>> to secure side even without *caching* variable's values.
>>
> Similar understanding here. The question is, will we have to think of something
> for non-arm architectures?

The design should be open for other architectures. If we use the
EFI_SMM_VARIABLE_PROTOCOL as the interface we should be able to support
other architectures.

I am just wondering why does the OP-TEE module handle all the logic of
EFI_SMM_VARIABLE_PROTOCOL. Wouldn't something like the
EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL make more sense?

This protocol could also be used to implement CapsuleUpdate().

>
>>> StandaloneMmPkg seems to be the hardware independent part of the
>>> solution. Where will the hardware driver reside in your OP-TEE solution?
> It depends on where your hardware is. If you have a NOR flash directly connected
> to the secure world the answer is yes.
> For starters we are going to use RPMB + U-Boot supplicant.
>
>>>
>>> Is the EDK2 hardware store for variables of the MACCHIATObin here:
>>> edk2-platforms/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c?
> No idea, i can ask around.
>
>>>
>>> Which hardware platform will you use for testing the U-Boot development
>>> of you OP-TEE driver?
>>
>> Ilias will be able to answer those questions.
> - stm32mp1 ST board based on armv7 [1]
> - Socionext DeveloperBox for armv8 [2]. This has a running EDKII implementation
> of StMM in SPM. The underlying firmware should be irrelevant though since the
> whole functionality is contained within an OP-TEE TA (trusted application). If i
> remember correctly that will need an extra driver in OP-TEE (if we port U-Boot
> on that)
> - TI AM6 board [3]. I don't have the board in my hands yet, so no details on it

I have a MACCHIATObin. Would this also be usable for the testing?

Regards

Heinrich

>
>
> [1] https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html
> [2] https://www.96boards.org/product/developerbox/
> [3] http://www.ti.com/tool/PROCESSOR-SDK-AM65X
>
>
> Regards
> /Ilias
>

On Wed, Jun 19, 2019 at 08:24:50AM +0200, Heinrich Schuchardt wrote:
> On 6/19/19 7:13 AM, Ilias Apalodimas wrote:
> >Heinrich,
> >
> >[...]
> >>>>>>>Unfortunately, this is not practical right now because there is
> >>>>>>>already some sort of assumption (and consensus) that we would re-use
> >>>>>>>"Standalone MM services", which is already there in EDK2, as
> >>>>>>>secure storage for UEFI variables.
> >>>>>>>In the case, all the cache would be bypassed.
> >>>>>>>In my old prototype, I utilized the cache but dropped that feature
> >>>>>>>for several reasons.
> >>>>>>
> >>>>>>What has EDK2 code to do with it?
> >>>>>
> >>>>>Did you follow my comment below?
> >>>>>>>Unfortunately, this is not practical right now because there is
> >>>>>>>already some sort of assumption (and consensus) that we would re-use
> >>>>>>>"Standalone MM services", which is already there in EDK2, as
> >>>>>>>secure storage for UEFI variables.
> >>>>We are already working towards having StandAloneMM as an early OP-TEE TA.
> >>>>This will provide us with a secure variable storage for armv7/v8.
> >>>
> >>>What would this OP-TEE binary do? - This seems to be a source of
> >>>misunderstanding when reviewing this patch.
> >>
> >>I and Ilias will give you more details offline, here's a short(?)
> >>answer:
> >>
> >>Standalone MM services here means a SPD entity which provides
> >>[Get|Set]Variable APIs to non-secure side firmware, that is
> >>currently EDK2. So the source code of Standalone MM services
> >>is included in EDK2 repository as a matter of fact.
> >>
> >>Here is one drawback: It won't allow for other entities running
> >>concurrently on secure side. One example of useful secure feature
> >>is (software-based) TPM. So Linaro is working on modifying/transforming
> >>Standalone MM to one OP-TEE application, which Ilias mentioned above.
> >>
> >Exactly. The current StMM implementation exists for Armv8 *only* in SPM (Secure
> >Partition Manager). The idea is to make it an OP-TEE application, so we can run
> >it on on Armv7s as well. As Akashi-san mentions SPD (Secure Payload Dispatcher)
> >and SPM are mutually exclusive so having everything as OP-TEE trusted
> >applications gives us a number of advantages at the moment.
> >
> >>>My guess is that OP-TEE is used to read non-volatile variables only once
> >>>when starting U-Boot and to write non-volatile variables whenever they
> >>>are changed.
> >>
> >>So OP-TEE version of StMM is still on-going project and I assume
> >>that this OP-TEE app will support the same set of functionality/APIs
> >>as StMM does.
> >Yes that's the goal
> 
> Do I understand it write:
> 
> In U-Boot we would have code that essentially provides the functionality
> of EDK2's EFI_SMM_VARIABLE_PROTOCOL. Like
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> this would talk via SMI calls with the hardware drivers, in this case
> the OP-TEE app.
> 
> This would allow the OP-TEE app to be used both in U-Boot and in EDK2?

I think so.

> >
> >>
> >>>All further reading of non-volatile variables and all access to volatile
> >>>variables will be handled by the U-Boot internal variable cache.
> >>>
> >>>For volatile variables I would assume OP-TEE to never see them. This
> >>>requires that the U-Boot variable cachek supports reading from and
> >>>writing to the cache at runtime.
> >>
> >>No. As far as I correctly understand, StMM handles volatile
> >>variables as well as non-volatile variables.
> >>EDK2 on non-secure side will redirect user's request directly
> >>to secure side even without *caching* variable's values.
> >>
> >Similar understanding here. The question is, will we have to think of something
> >for non-arm architectures?
> 
> The design should be open for other architectures. If we use the
> EFI_SMM_VARIABLE_PROTOCOL as the interface we should be able to support
> other architectures.

Yeah, but please note that EFI_SMM_VARIABLE_PROTOCOL is not part
of UEFI specification.

> I am just wondering why does the OP-TEE module handle all the logic of
> EFI_SMM_VARIABLE_PROTOCOL. Wouldn't something like the
> EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL make more sense?

It provides only read/write interfaces *without* protection.
As far as SetVariable API is concerned, you cannot compromise
any authenticated variables unless you can sign a given variable
with a trusted private key even if you can make a SMI call.

-Takahiro Akashi

> This protocol could also be used to implement CapsuleUpdate().
> 
> >
> >>>StandaloneMmPkg seems to be the hardware independent part of the
> >>>solution. Where will the hardware driver reside in your OP-TEE solution?
> >It depends on where your hardware is. If you have a NOR flash directly connected
> >to the secure world the answer is yes.
> >For starters we are going to use RPMB + U-Boot supplicant.
> >
> >>>
> >>>Is the EDK2 hardware store for variables of the MACCHIATObin here:
> >>>edk2-platforms/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c?
> >No idea, i can ask around.
> >
> >>>
> >>>Which hardware platform will you use for testing the U-Boot development
> >>>of you OP-TEE driver?
> >>
> >>Ilias will be able to answer those questions.
> >- stm32mp1 ST board based on armv7 [1]
> >- Socionext DeveloperBox for armv8 [2]. This has a running EDKII implementation
> >of StMM in SPM. The underlying firmware should be irrelevant though since the
> >whole functionality is contained within an OP-TEE TA (trusted application). If i
> >remember correctly that will need an extra driver in OP-TEE (if we port U-Boot
> >on that)
> >- TI AM6 board [3]. I don't have the board in my hands yet, so no details on it
> 
> I have a MACCHIATObin. Would this also be usable for the testing?

I think the answer will depend on what (feature) you want to test.

-Takahiro Akashi

> Regards
> 
> Heinrich
> 
> >
> >
> >[1] https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html
> >[2] https://www.96boards.org/product/developerbox/
> >[3] http://www.ti.com/tool/PROCESSOR-SDK-AM65X
> >
> >
> >Regards
> >/Ilias
> >
>