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.aka...@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 */
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
