[U-Boot] [PATCH v3 3/7] efi_loader: variable: split UEFI variables from U-Boot environment

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Tue, Jun 04, 2019 at 11:31:24PM +0200, Heinrich Schuchardt wrote:
> On 6/4/19 8:52 AM, AKASHI Takahiro wrote:
> >UEFI volatile variables are managed in efi_var_htab while UEFI non-volatile
> >variables are in efi_nv_var_htab. At every SetVariable API, env_efi_save()
> >will also be called to save data cache (hash table) to persistent storage.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  lib/efi_loader/Kconfig        |  10 +
> >  lib/efi_loader/efi_variable.c | 342 ++++++++++++++++++++++++++--------
> >  2 files changed, 275 insertions(+), 77 deletions(-)
> >
> >diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >index cd5436c576b1..8bf4b1754d06 100644
> >--- a/lib/efi_loader/Kconfig
> >+++ b/lib/efi_loader/Kconfig
> >@@ -18,6 +18,16 @@ config EFI_LOADER
> >
> >  if EFI_LOADER
> >
> >+choice
> >+	prompt "Select variables storage"
> >+	default EFI_VARIABLE_USE_ENV
> >+
> >+config EFI_VARIABLE_USE_ENV
> >+	bool "Same device as U-Boot environment"
> >+	select ENV_EFI
> >+
> >+endchoice
> >+
> >  config EFI_GET_TIME
> >  	bool "GetTime() runtime service"
> >  	depends on DM_RTC
> >diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >index e56053194dae..d9887be938c2 100644
> >--- a/lib/efi_loader/efi_variable.c
> >+++ b/lib/efi_loader/efi_variable.c
> >@@ -48,6 +48,66 @@
> >   * converted to utf16?
> >   */
> >
> >+/*
> >+ * We will maintain two variable database: one for volatile variables,
> >+ * the other for non-volatile variables. The former exists only in memory
> >+ * and will go away at re-boot. The latter is currently backed up by the same
> >+ * device as U-Boot environment and also works as variables cache.
> >+ *
> >+ *	struct hsearch_data efi_var_htab
> >+ *	struct hsearch_data efi_nv_var_htab
> >+ */
> >+
> >+static char *env_efi_get(const char *name, bool is_non_volatile)
> >+{
> >+	struct hsearch_data *htab;
> >+	ENTRY e, *ep;
> >+
> >+	/* WATCHDOG_RESET(); */
> >+
> >+	if (is_non_volatile)
> >+		htab = &efi_nv_var_htab;
> >+	else
> >+		htab = &efi_var_htab;
> >+
> >+	e.key   = name;
> >+	e.data  = NULL;
> >+	hsearch_r(e, FIND, &ep, htab, 0);
> >+
> >+	return ep ? ep->data : NULL;
> >+}
> >+
> >+static int env_efi_set(const char *name, const char *value,
> >+		       bool is_non_volatile)
> >+{
> >+	struct hsearch_data *htab;
> >+	ENTRY e, *ep;
> >+	int ret;
> >+
> >+	if (is_non_volatile)
> >+		htab = &efi_nv_var_htab;
> >+	else
> >+		htab = &efi_var_htab;
> >+
> >+	/* delete */
> >+	if (!value || *value == '\0') {
> >+		ret = hdelete_r(name, htab, H_PROGRAMMATIC);
> >+		return !ret;
> >+	}
> >+
> >+	/* set */
> >+	e.key   = name;
> >+	e.data  = (char *)value;
> >+	hsearch_r(e, ENTER, &ep, htab, H_PROGRAMMATIC);
> >+	if (!ep) {
> >+		printf("## Error inserting \"%s\" variable, errno=%d\n",
> >+		       name, errno);
> >+		return 1;
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> >  #define PREFIX_LEN (strlen("efi_xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx_"))
> >
> >  /**
> >@@ -147,24 +207,12 @@ static const char *parse_attr(const char *str, u32 *attrp)
> >  	return str;
> >  }
> >
> >-/**
> >- * efi_efi_get_variable() - retrieve value of a UEFI variable
> >- *
> >- * This function implements the GetVariable runtime service.
> >- *
> >- * See the Unified Extensible Firmware Interface (UEFI) specification for
> >- * details.
> >- *
> >- * @variable_name:	name of the variable
> >- * @vendor:		vendor GUID
> >- * @attributes:		attributes of the variable
> >- * @data_size:		size of the buffer to which the variable value is copied
> >- * @data:		buffer to which the variable value is copied
> >- * Return:		status code
> >- */
> >-efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >-				     const efi_guid_t *vendor, u32 *attributes,
> >-				     efi_uintn_t *data_size, void *data)
> >+static
> >+efi_status_t EFIAPI efi_get_variable_common(u16 *variable_name,
> >+					    const efi_guid_t *vendor,
> >+					    u32 *attributes,
> >+					    efi_uintn_t *data_size, void *data,
> >+					    bool is_non_volatile)
> >  {
> >  	char *native_name;
> >  	efi_status_t ret;
> >@@ -172,22 +220,19 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >  	const char *val, *s;
> >  	u32 attr;
> >
> >-	EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
> >-		  data_size, data);
> >-
> >  	if (!variable_name || !vendor || !data_size)
> >  		return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> >  	ret = efi_to_native(&native_name, variable_name, vendor);
> >  	if (ret)
> >-		return EFI_EXIT(ret);
> >+		return ret;
> >
> >  	EFI_PRINT("get '%s'\n", native_name);
> >
> >-	val = env_get(native_name);
> >+	val = env_efi_get(native_name, is_non_volatile);
> >  	free(native_name);
> >  	if (!val)
> >-		return EFI_EXIT(EFI_NOT_FOUND);
> >+		return EFI_NOT_FOUND;
> >
> >  	val = parse_attr(val, &attr);
> >
> >@@ -198,7 +243,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >
> >  		/* number of hexadecimal digits must be even */
> >  		if (len & 1)
> >-			return EFI_EXIT(EFI_DEVICE_ERROR);
> >+			return EFI_DEVICE_ERROR;
> >
> >  		/* two characters per byte: */
> >  		len /= 2;
> >@@ -210,10 +255,10 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >  		}
> >
> >  		if (!data)
> >-			return EFI_EXIT(EFI_INVALID_PARAMETER);
> >+			return EFI_INVALID_PARAMETER;
> >
> >  		if (hex2bin(data, s, len))
> >-			return EFI_EXIT(EFI_DEVICE_ERROR);
> >+			return EFI_DEVICE_ERROR;
> >
> >  		EFI_PRINT("got value: \"%s\"\n", s);
> >  	} else if ((s = prefix(val, "(utf8)"))) {
> >@@ -227,7 +272,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >  		}
> >
> >  		if (!data)
> >-			return EFI_EXIT(EFI_INVALID_PARAMETER);
> >+			return EFI_INVALID_PARAMETER;
> >
> >  		memcpy(data, s, len);
> >  		((char *)data)[len] = '\0';
> >@@ -235,13 +280,67 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >  		EFI_PRINT("got value: \"%s\"\n", (char *)data);
> >  	} else {
> >  		EFI_PRINT("invalid value: '%s'\n", val);
> >-		return EFI_EXIT(EFI_DEVICE_ERROR);
> >+		return EFI_DEVICE_ERROR;
> >  	}
> >
> >  out:
> >  	if (attributes)
> >  		*attributes = attr & EFI_VARIABLE_MASK;
> >
> >+	return ret;
> >+}
> >+
> >+static
> >+efi_status_t EFIAPI efi_get_volatile_variable(u16 *variable_name,
> >+					      const efi_guid_t *vendor,
> >+					      u32 *attributes,
> >+					      efi_uintn_t *data_size,
> >+					      void *data)
> >+{
> >+	return efi_get_variable_common(variable_name, vendor, attributes,
> >+				       data_size, data, false);
> >+}
> >+
> >+efi_status_t EFIAPI efi_get_nonvolatile_variable(u16 *variable_name,
> >+						 const efi_guid_t *vendor,
> >+						 u32 *attributes,
> >+						 efi_uintn_t *data_size,
> >+						 void *data)
> >+{
> >+	return efi_get_variable_common(variable_name, vendor, attributes,
> >+				       data_size, data, true);
> >+}
> >+
> >+/**
> >+ * efi_efi_get_variable() - retrieve value of a UEFI variable
> >+ *
> >+ * This function implements the GetVariable runtime service.
> >+ *
> >+ * See the Unified Extensible Firmware Interface (UEFI) specification for
> >+ * details.
> >+ *
> >+ * @variable_name:	name of the variable
> >+ * @vendor:		vendor GUID
> >+ * @attributes:		attributes of the variable
> >+ * @data_size:		size of the buffer to which the variable value is copied
> >+ * @data:		buffer to which the variable value is copied
> >+ * Return:		status code
> >+ */
> >+efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >+				     const efi_guid_t *vendor, u32 *attributes,
> >+				     efi_uintn_t *data_size, void *data)
> >+{
> >+	efi_status_t ret;
> >+
> >+	EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
> >+		  data_size, data);
> >+
> >+	ret = efi_get_volatile_variable(variable_name, vendor, attributes,
> >+					data_size, data);
> >+	if (ret == EFI_NOT_FOUND)
> >+		ret = efi_get_nonvolatile_variable(variable_name, vendor,
> >+						   attributes, data_size, data);
> >+
> >  	return EFI_EXIT(ret);
> >  }
> >
> >@@ -331,7 +430,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> >  					       u16 *variable_name,
> >  					       const efi_guid_t *vendor)
> >  {
> >-	char *native_name, *variable;
> >+	char *native_name, *variable, *tmp_list, *merged_list;
> >  	ssize_t name_len, list_len;
> >  	char regex[256];
> >  	char * const regexlist[] = {regex};
> >@@ -387,10 +486,39 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> >  		efi_cur_variable = NULL;
> >
> >  		snprintf(regex, 256, "efi_.*-.*-.*-.*-.*_.*");
> >-		list_len = hexport_r(&env_htab, '\n',
> >+		list_len = hexport_r(&efi_var_htab, '\n',
> >  				     H_MATCH_REGEX | H_MATCH_KEY,
> >  				     &efi_variables_list, 0, 1, regexlist);
> >-		/* 1 indicates that no match was found */
> >+		/*
> >+		 * Note: '1' indicates that nothing is matched
> >+		 */
> >+		if (list_len <= 1) {
> >+			free(efi_variables_list);
> >+			efi_variables_list = NULL;
> >+			list_len = hexport_r(&efi_nv_var_htab, '\n',
> >+					     H_MATCH_REGEX | H_MATCH_KEY,
> >+					     &efi_variables_list, 0, 1,
> >+					     regexlist);
> >+		} else {
> >+			tmp_list = NULL;
> >+			list_len = hexport_r(&efi_nv_var_htab, '\n',
> >+					     H_MATCH_REGEX | H_MATCH_KEY,
> >+					     &tmp_list, 0, 1,
> >+					     regexlist);
> >+			if (list_len <= 1) {
> >+				list_len = 2; /* don't care actual number */
> >+			} else {
> >+				/* merge two variables lists */
> >+				merged_list = malloc(strlen(efi_variables_list)
> >+							+ strlen(tmp_list) + 1);
> >+				strcpy(merged_list, efi_variables_list);
> >+				strcat(merged_list, tmp_list);
> >+				free(efi_variables_list);
> >+				free(tmp_list);
> >+				efi_variables_list = merged_list;
> >+			}
> >+		}
> >+
> >  		if (list_len <= 1)
> >  			return EFI_EXIT(EFI_NOT_FOUND);
> >
> >@@ -403,77 +531,71 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> >  	return EFI_EXIT(ret);
> >  }
> >
> >-/**
> >- * efi_efi_set_variable() - set value of a UEFI variable
> >- *
> >- * This function implements the SetVariable runtime service.
> >- *
> >- * See the Unified Extensible Firmware Interface (UEFI) specification for
> >- * details.
> >- *
> >- * @variable_name:	name of the variable
> >- * @vendor:		vendor GUID
> >- * @attributes:		attributes of the variable
> >- * @data_size:		size of the buffer with the variable value
> >- * @data:		buffer with the variable value
> >- * Return:		status code
> >- */
> >-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)
> >+static
> >+efi_status_t EFIAPI efi_set_variable_common(u16 *variable_name,
> >+					    const efi_guid_t *vendor,
> >+					    u32 attributes,
> >+					    efi_uintn_t data_size,
> >+					    const void *data,
> >+					    bool is_non_volatile)
> >  {
> >  	char *native_name = NULL, *val = NULL, *s;
> >-	efi_status_t ret = EFI_SUCCESS;
> >+	efi_uintn_t size;
> >  	u32 attr;
> >-
> >-	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> >-		  data_size, data);
> >+	efi_status_t ret = EFI_SUCCESS;
> >
> >  	/* TODO: implement APPEND_WRITE */
> >  	if (!variable_name || !vendor ||
> >  	    (attributes & EFI_VARIABLE_APPEND_WRITE)) {
> >  		ret = EFI_INVALID_PARAMETER;
> >-		goto out;
> >+		goto err;
> >  	}
> >
> >  	ret = efi_to_native(&native_name, variable_name, vendor);
> >  	if (ret)
> >-		goto out;
> >+		goto err;
> >
> >  #define ACCESS_ATTR (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)
> >
> >-	if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> >-		/* delete the variable: */
> >-		env_set(native_name, NULL);
> >-		ret = EFI_SUCCESS;
> >-		goto out;
> >+	/* check if a variable exists */
> >+	size = 0;
> >+	ret = EFI_CALL(efi_get_variable(variable_name, vendor, &attr,
> >+					&size, NULL));
> >+	if (ret == EFI_BUFFER_TOO_SMALL) {
> >+		if ((is_non_volatile && !(attr & EFI_VARIABLE_NON_VOLATILE)) ||
> >+		    (!is_non_volatile && (attr & EFI_VARIABLE_NON_VOLATILE))) {
> >+			ret = EFI_INVALID_PARAMETER;
> >+			goto err;
> >+		}
> >  	}
> >
> >-	val = env_get(native_name);
> >-	if (val) {
> >-		parse_attr(val, &attr);
> >-
> >-		/* We should not free val */
> >-		val = NULL;
> >-		if (attr & READ_ONLY) {
> >-			ret = EFI_WRITE_PROTECTED;
> >+	/* delete a variable */
> >+	if (data_size == 0 || !(attributes & ACCESS_ATTR)) {
> >+		if (size) {
> >+			if (attr & READ_ONLY) {
> >+				ret = EFI_WRITE_PROTECTED;
> >+				goto err;
> >+			}
> >  			goto out;
> >  		}
> >+		ret = EFI_SUCCESS;
> >+		goto err; /* not error, but nothing to do */
> >+	}
> >
> >+	/* create/modify a variable */
> >+	if (size && attr != attributes) {
> >  		/*
> >  		 * attributes won't be changed
> >  		 * TODO: take care of APPEND_WRITE once supported
> >  		 */
> >-		if (attr != attributes) {
> >-			ret = EFI_INVALID_PARAMETER;
> >-			goto out;
> >-		}
> >+		ret = EFI_INVALID_PARAMETER;
> >+		goto err;
> >  	}
> >
> >  	val = malloc(2 * data_size + strlen("{ro,run,boot,nv}(blob)") + 1);
> >  	if (!val) {
> >  		ret = EFI_OUT_OF_RESOURCES;
> >-		goto out;
> >+		goto err;
> >  	}
> >
> >  	s = val;
> >@@ -487,7 +609,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >  		       EFI_VARIABLE_RUNTIME_ACCESS);
> >  	s += sprintf(s, "{");
> >  	while (attributes) {
> >-		u32 attr = 1 << (ffs(attributes) - 1);
> >+		attr = 1 << (ffs(attributes) - 1);
> >
> >  		if (attr == EFI_VARIABLE_NON_VOLATILE)
> >  			s += sprintf(s, "nv");
> >@@ -509,12 +631,78 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >
> >  	EFI_PRINT("setting: %s=%s\n", native_name, val);
> >
> >-	if (env_set(native_name, val))
> >+out:
> >+	ret = EFI_SUCCESS;
> >+	if (env_efi_set(native_name, val, is_non_volatile))
> >  		ret = EFI_DEVICE_ERROR;
> >
> >-out:
> >+err:
> >  	free(native_name);
> >  	free(val);
> >
> >+	return ret;
> >+}
> >+
> >+static
> >+efi_status_t EFIAPI efi_set_volatile_variable(u16 *variable_name,
> 
> Once you have implemented this we can make the variable service
> available at runtime. So I suggest to define everything here as __runtime.

Good question! I intended so when I started this work, but
we can't do that partly because bunch of code from U-Boot, which is not
part of RUNTIME_CODE, will be exercised and partly because all the entries
in a hash table are allocated by *malloc*, which are again not part of
RUNTIME_DATA.
It is quite painful to modify the code and data so that they are accessible
at UEFI runtime.

Instead, I implemented "caching" features for runtime access.
I'm going to post the patch set (as RFC) later today.

> Why do you any of these three functions (efi_set_volatile_variable(),
> efi_set_nonvolatile_variable(), efi_set_nonvolatile_variable()). Just
> use an if based on attributes in efi_set_variable() to call
> env_efi_save, when a non-volatile function is changed.

I don't get you point. Please clarify your comment.

> What is the advantage of having two separate RAM stores? Can't the save
> function sort out what to save and what not to save according to the NV
> flag?

Technically, we can do that, but it is nothing but inefficient.
Just FYI, EDK2 also maintains separate buffers.

> >+					      const efi_guid_t *vendor,
> >+					      u32 attributes,
> >+					      efi_uintn_t data_size,
> >+					      const void *data)
> >+{
> >+	return efi_set_variable_common(variable_name, vendor, attributes,
> >+				       data_size, data, false);
> >+}
> >+
> >+efi_status_t EFIAPI efi_set_nonvolatile_variable(u16 *variable_name,
> >+						 const efi_guid_t *vendor,
> >+						 u32 attributes,
> >+						 efi_uintn_t data_size,
> >+						 const void *data)
> >+{
> >+	efi_status_t ret;
> >+
> >+	ret = efi_set_variable_common(variable_name, vendor, attributes,
> >+				      data_size, data, true);
> >+	if (ret == EFI_SUCCESS)
> >+		/* FIXME: what if save failed? */
> >+		if (env_efi_save())
> 
> Somewhere we need the ability to switch between different backends. Will
> that be in env_efi_save()?

That is why I put the related code in "env."

Thanks,
-Takahiro Akashi

> >+			ret = EFI_DEVICE_ERROR;
> >+
> >+	return ret;
> >+}
> >+
> >+/**
> >+ * efi_efi_set_variable() - set value of a UEFI variable
> >+ *
> >+ * This function implements the SetVariable runtime service.
> >+ *
> >+ * See the Unified Extensible Firmware Interface (UEFI) specification for
> >+ * details.
> >+ *
> >+ * @variable_name:	name of the variable
> >+ * @vendor:		vendor GUID
> >+ * @attributes:		attributes of the variable
> >+ * @data_size:		size of the buffer with the variable value
> >+ * @data:		buffer with the variable value
> >+ * Return:		status code
> >+ */
> >+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)
> >+{
> >+	efi_status_t ret;
> >+
> >+	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> >+		  data_size, data);
> >+
> >+	if (attributes & EFI_VARIABLE_NON_VOLATILE)
> >+		ret = efi_set_nonvolatile_variable(variable_name, vendor,
> >+						   attributes,
> >+						   data_size, data);
> >+	else
> >+		ret = efi_set_volatile_variable(variable_name, vendor,
> >+						attributes, data_size, data);
> >+
> >  	return EFI_EXIT(ret);
> >  }
> >
>