diff mbox series

[U-Boot,RFC,v4,06/16] env: add variable storage attribute support

Message ID 20190717082525.891-7-takahiro.akashi@linaro.org
State RFC
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: non-volatile variables support | expand

Commit Message

AKASHI Takahiro July 17, 2019, 8:25 a.m. UTC
If U-Boot environment variable is set to ENV_FLAGS_VARSTORAGE_NON_VOLATILE,
it will be saved at env_save[_ext]().
If U-Boot environment variable is set to
ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE, it will be saved at
env_set_ext() as well as env_save[_ext]().
Otherwise, it is handled as a temporary variable.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 env/flags.c         | 130 +++++++++++++++++++++++++++++++++++++++++++-
 include/env_flags.h |  36 +++++++++++-
 2 files changed, 164 insertions(+), 2 deletions(-)

Comments

Wolfgang Denk July 19, 2019, 8:35 a.m. UTC | #1
Dear Takahiro,

In message <20190717082525.891-7-takahiro.akashi@linaro.org> you wrote:
> If U-Boot environment variable is set to ENV_FLAGS_VARSTORAGE_NON_VOLATILE,
> it will be saved at env_save[_ext]().

NAK.  This is the status quo for all environment variables, and must
remain the default.

> If U-Boot environment variable is set to
> ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE, it will be saved at
> env_set_ext() as well as env_save[_ext]().
> Otherwise, it is handled as a temporary variable.

The logic is wrong.  It should be:

ENV_FLAGS_AUTOSAVE is saved at env_set().

ENV_FLAGS_VOLATILE is not saved at env_save().

That's all.

[And it should be checked that assigning ENV_FLAGS_VOLATILE and
ENV_FLAGS_AUTOSAVE flags to the same variable is handled as an
error.]

>  static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
>  static const char env_flags_varaccess_rep[] = "aroc";
> +static const char env_flags_varstorage_rep[] = "vna";

Please fix.

> +static const char * const env_flags_varstorage_names[] = {
> +	"volatile",
> +	"non-volatile",
> +	"non-volatile-autosave",
> +};

And here.

> +/*
> + * Print the whole list of available storage flags.
> + */
> +void env_flags_print_varstorage(void)
> +{
> +	enum env_flags_varstorage curstorage = (enum env_flags_varstorage)0;
> +
> +	while (curstorage != env_flags_varstorage_end) {
> +		printf("\t%c   -\t%s\n", env_flags_varstorage_rep[curstorage],
> +		       env_flags_varstorage_names[curstorage]);
> +		curstorage++;
> +	}
> +}

This makes little sense to me.  It has nothing to do with "storage".
Also, "non-volatile" is no special case and does not need a separate
flag or state. 

Your handling suggests that these are states on the same level,
which is not the case - a variable may be volatile or not, but
non-volatile variables acan be auto-save, while volatile ones
cannot.

> +
> +/*
> + * Return the name of the storage.
> + */
> +const char *env_flags_get_varstorage_name(enum env_flags_varstorage storage)
> +{
> +	return env_flags_varstorage_names[storage];
> +}
>  #endif /* CONFIG_CMD_ENV_FLAGS */

This does not work, as one variiable can be both persistent _and_
auto-save at the same time.

> +/*
> + * Parse the flags string from a .flags attribute list into the varstorage enum.
> + */

Using an enum is inherently wrong here.

Please keep in ind that we may want toadd additional poperties which
are completely orthogonal to the flags you're adding here.  Your
enum type handling oes not allow any such additions.  Please drop
it.

> +/*
> + * Parse the binary flags from a hash table entry into the varstorage enum.
> + */
> +enum env_flags_varstorage env_flags_parse_varstorage_from_binflags(int binflags)
> +{
> +	int bits;
> +
> +	bits = binflags & ENV_FLAGS_VARSTORAGE_BIN_MASK;
> +	if (bits == ENV_FLAGS_VARSTORAGE_VOLATILE)
> +		return env_flags_varstorage_volatile;
> +	else if (bits == ENV_FLAGS_VARSTORAGE_NON_VOLATILE)
> +		return env_flags_varstorage_non_volatile;
> +	else if (bits == ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE)
> +		return env_flags_varstorage_non_volatile_autosave;
> +
> +	printf("Warning: Non-standard storage flags. (0x%x)\n",
> +	       binflags & ENV_FLAGS_VARSTORAGE_BIN_MASK);
> +
> +	return env_flags_varstorage_non_volatile;
> +}

Ditto here.

> +/*
> + * Look up the storage of a variable directly from the .flags var.
> + */
> +enum env_flags_varstorage env_flags_get_storage(const char *name)
> +{
> +	const char *flags_list = env_get(ENV_FLAGS_VAR);
> +	char flags[ENV_FLAGS_ATTR_MAX_LEN + 1];
> +
> +	if (env_flags_lookup(flags_list, name, flags))
> +		return env_flags_varstorage_non_volatile;
> +
> +	if (strlen(flags) <= ENV_FLAGS_VARSTORAGE_LOC)
> +		return env_flags_varstorage_non_volatile;
> +
> +	return env_flags_parse_varstorage(flags);
> +}

And here.

>  	binflags = env_flags_parse_vartype(flags) & ENV_FLAGS_VARTYPE_BIN_MASK;
>  	binflags |= env_flags_varaccess_mask[env_flags_parse_varaccess(flags)];
> +	switch (env_flags_parse_varstorage(flags)) {
> +	case env_flags_varstorage_volatile:
> +		/* actually no effect */
> +		binflags |= ENV_FLAGS_VARSTORAGE_VOLATILE;
> +		break;
> +	case env_flags_varstorage_non_volatile:
> +		binflags |= ENV_FLAGS_VARSTORAGE_NON_VOLATILE;
> +		break;
> +	case env_flags_varstorage_non_volatile_autosave:
> +		binflags |= ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE;
> +		break;
> +	case env_flags_varstorage_end:
> +		/* makes no sense */
> +		break;
> +	}

and here.

>  static int first_call = 1;
>  static const char *flags_list;
>  
> +static int env_flags_default(enum env_context ctx)
> +{
> +	if (ctx == ENVCTX_UBOOT)
> +		return ENV_FLAGS_VARSTORAGE_NON_VOLATILE;
> +#ifdef CONFIG_EFI_VARIABLE_USE_ENV
> +	else if (ctx == ENVCTX_UEFI)
> +		/* explicitly set by caller */
> +		return 0;
> +#endif

As mentioned several times before, ENV_FLAGS_VARSTORAGE_NON_VOLATILE
should not be needed at all, this is the default. Which means, the
whole funktion is not needed.  Drop it.

>   * Look for possible flags for a newly added variable
>   * This is called specifically when the variable did not exist in the hash
> @@ -434,6 +553,9 @@ void env_flags_init(ENTRY *var_entry)
>  	/* if any flags were found, set the binary form to the entry */
>  	if (!ret && strlen(flags))
>  		var_entry->flags = env_parse_flags_to_bin(flags);
> +	/* TODO: check early? */

What does that mean??

> +	/*
> +	 * TODO: context specific check necessary?
> +	 * For example, STORAGE_NON_VOLATILE and STORAGE_NON_VOLATILE_AUTOSAVE
> +	 * must not be mixed in one context.
> +	 */

They must never be mixed.  Yes, this must be checked and handled.

> +enum env_flags_varstorage {
> +	env_flags_varstorage_volatile,			/* 'v' */
> +	env_flags_varstorage_non_volatile,		/* 'n' */
> +	env_flags_varstorage_non_volatile_autosave,	/* 'a' */
> +	env_flags_varstorage_end
> +};

NAK, see above.  We don;t need a whole new set of flags, just add
"VOLATILE" and "AUTOSAVE" to the existing ones.

Best regards,

Wolfgang Denk
diff mbox series

Patch

diff --git a/env/flags.c b/env/flags.c
index 79dccc05fe27..c1db891a2343 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -29,6 +29,7 @@ 
 
 static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
 static const char env_flags_varaccess_rep[] = "aroc";
+static const char env_flags_varstorage_rep[] = "vna";
 static const int env_flags_varaccess_mask[] = {
 	0,
 	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
@@ -57,6 +58,12 @@  static const char * const env_flags_varaccess_names[] = {
 	"change-default",
 };
 
+static const char * const env_flags_varstorage_names[] = {
+	"volatile",
+	"non-volatile",
+	"non-volatile-autosave",
+};
+
 /*
  * Print the whole list of available type flags.
  */
@@ -85,6 +92,20 @@  void env_flags_print_varaccess(void)
 	}
 }
 
+/*
+ * Print the whole list of available storage flags.
+ */
+void env_flags_print_varstorage(void)
+{
+	enum env_flags_varstorage curstorage = (enum env_flags_varstorage)0;
+
+	while (curstorage != env_flags_varstorage_end) {
+		printf("\t%c   -\t%s\n", env_flags_varstorage_rep[curstorage],
+		       env_flags_varstorage_names[curstorage]);
+		curstorage++;
+	}
+}
+
 /*
  * Return the name of the type.
  */
@@ -100,6 +121,14 @@  const char *env_flags_get_varaccess_name(enum env_flags_varaccess access)
 {
 	return env_flags_varaccess_names[access];
 }
+
+/*
+ * Return the name of the storage.
+ */
+const char *env_flags_get_varstorage_name(enum env_flags_varstorage storage)
+{
+	return env_flags_varstorage_names[storage];
+}
 #endif /* CONFIG_CMD_ENV_FLAGS */
 
 /*
@@ -146,6 +175,30 @@  enum env_flags_varaccess env_flags_parse_varaccess(const char *flags)
 	return env_flags_varaccess_any;
 }
 
+/*
+ * Parse the flags string from a .flags attribute list into the varstorage enum.
+ */
+enum env_flags_varstorage env_flags_parse_varstorage(const char *flags)
+{
+	char *storage;
+
+	if (strlen(flags) <= ENV_FLAGS_VARSTORAGE_LOC)
+		/* for compatibility */
+		return env_flags_varstorage_non_volatile;
+
+	storage = strchr(env_flags_varstorage_rep,
+			 flags[ENV_FLAGS_VARSTORAGE_LOC]);
+
+	if (storage)
+		return (enum env_flags_varstorage)
+			(storage - &env_flags_varstorage_rep[0]);
+
+	printf("## Warning: Unknown environment variable storage '%c'\n",
+	       flags[ENV_FLAGS_VARSTORAGE_LOC]);
+
+	return env_flags_varstorage_non_volatile;
+}
+
 /*
  * Parse the binary flags from a hash table entry into the varaccess enum.
  */
@@ -164,6 +217,27 @@  enum env_flags_varaccess env_flags_parse_varaccess_from_binflags(int binflags)
 	return env_flags_varaccess_any;
 }
 
+/*
+ * Parse the binary flags from a hash table entry into the varstorage enum.
+ */
+enum env_flags_varstorage env_flags_parse_varstorage_from_binflags(int binflags)
+{
+	int bits;
+
+	bits = binflags & ENV_FLAGS_VARSTORAGE_BIN_MASK;
+	if (bits == ENV_FLAGS_VARSTORAGE_VOLATILE)
+		return env_flags_varstorage_volatile;
+	else if (bits == ENV_FLAGS_VARSTORAGE_NON_VOLATILE)
+		return env_flags_varstorage_non_volatile;
+	else if (bits == ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE)
+		return env_flags_varstorage_non_volatile_autosave;
+
+	printf("Warning: Non-standard storage flags. (0x%x)\n",
+	       binflags & ENV_FLAGS_VARSTORAGE_BIN_MASK);
+
+	return env_flags_varstorage_non_volatile;
+}
+
 static inline int is_hex_prefix(const char *value)
 {
 	return value[0] == '0' && (value[1] == 'x' || value[1] == 'X');
@@ -336,6 +410,23 @@  enum env_flags_varaccess env_flags_get_varaccess(const char *name)
 	return env_flags_parse_varaccess(flags);
 }
 
+/*
+ * Look up the storage of a variable directly from the .flags var.
+ */
+enum env_flags_varstorage env_flags_get_storage(const char *name)
+{
+	const char *flags_list = env_get(ENV_FLAGS_VAR);
+	char flags[ENV_FLAGS_ATTR_MAX_LEN + 1];
+
+	if (env_flags_lookup(flags_list, name, flags))
+		return env_flags_varstorage_non_volatile;
+
+	if (strlen(flags) <= ENV_FLAGS_VARSTORAGE_LOC)
+		return env_flags_varstorage_non_volatile;
+
+	return env_flags_parse_varstorage(flags);
+}
+
 /*
  * Validate that the proposed new value for "name" is valid according to the
  * defined flags for that variable, if any.
@@ -402,10 +493,25 @@  int env_flags_validate_env_set_params(char *name, char * const val[], int count)
  */
 static int env_parse_flags_to_bin(const char *flags)
 {
-	int binflags;
+	int binflags = 0;
 
 	binflags = env_flags_parse_vartype(flags) & ENV_FLAGS_VARTYPE_BIN_MASK;
 	binflags |= env_flags_varaccess_mask[env_flags_parse_varaccess(flags)];
+	switch (env_flags_parse_varstorage(flags)) {
+	case env_flags_varstorage_volatile:
+		/* actually no effect */
+		binflags |= ENV_FLAGS_VARSTORAGE_VOLATILE;
+		break;
+	case env_flags_varstorage_non_volatile:
+		binflags |= ENV_FLAGS_VARSTORAGE_NON_VOLATILE;
+		break;
+	case env_flags_varstorage_non_volatile_autosave:
+		binflags |= ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE;
+		break;
+	case env_flags_varstorage_end:
+		/* makes no sense */
+		break;
+	}
 
 	return binflags;
 }
@@ -413,6 +519,19 @@  static int env_parse_flags_to_bin(const char *flags)
 static int first_call = 1;
 static const char *flags_list;
 
+static int env_flags_default(enum env_context ctx)
+{
+	if (ctx == ENVCTX_UBOOT)
+		return ENV_FLAGS_VARSTORAGE_NON_VOLATILE;
+#ifdef CONFIG_EFI_VARIABLE_USE_ENV
+	else if (ctx == ENVCTX_UEFI)
+		/* explicitly set by caller */
+		return 0;
+#endif
+
+	return 0;
+}
+
 /*
  * Look for possible flags for a newly added variable
  * This is called specifically when the variable did not exist in the hash
@@ -434,6 +553,9 @@  void env_flags_init(ENTRY *var_entry)
 	/* if any flags were found, set the binary form to the entry */
 	if (!ret && strlen(flags))
 		var_entry->flags = env_parse_flags_to_bin(flags);
+	/* TODO: check early? */
+	else if (!var_entry->flags)
+		var_entry->flags = env_flags_default(var_entry->context);
 }
 
 /*
@@ -522,6 +644,12 @@  int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op,
 		}
 	}
 
+	/*
+	 * TODO: context specific check necessary?
+	 * For example, STORAGE_NON_VOLATILE and STORAGE_NON_VOLATILE_AUTOSAVE
+	 * must not be mixed in one context.
+	 */
+
 	/* check for access permission */
 #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
 	if (flag & H_FORCE)
diff --git a/include/env_flags.h b/include/env_flags.h
index 23744e395c8a..f46528691889 100644
--- a/include/env_flags.h
+++ b/include/env_flags.h
@@ -27,10 +27,18 @@  enum env_flags_varaccess {
 	env_flags_varaccess_end
 };
 
+enum env_flags_varstorage {
+	env_flags_varstorage_volatile,			/* 'v' */
+	env_flags_varstorage_non_volatile,		/* 'n' */
+	env_flags_varstorage_non_volatile_autosave,	/* 'a' */
+	env_flags_varstorage_end
+};
+
 #define ENV_FLAGS_VAR ".flags"
-#define ENV_FLAGS_ATTR_MAX_LEN 2
+#define ENV_FLAGS_ATTR_MAX_LEN 3
 #define ENV_FLAGS_VARTYPE_LOC 0
 #define ENV_FLAGS_VARACCESS_LOC 1
+#define ENV_FLAGS_VARSTORAGE_LOC 2
 
 #ifndef CONFIG_ENV_FLAGS_LIST_STATIC
 #define CONFIG_ENV_FLAGS_LIST_STATIC ""
@@ -85,6 +93,10 @@  void env_flags_print_vartypes(void);
  * Print the whole list of available access flags.
  */
 void env_flags_print_varaccess(void);
+/*
+ * Print the whole list of available storage flags.
+ */
+void env_flags_print_varstorage(void);
 /*
  * Return the name of the type.
  */
@@ -93,6 +105,10 @@  const char *env_flags_get_vartype_name(enum env_flags_vartype type);
  * Return the name of the access.
  */
 const char *env_flags_get_varaccess_name(enum env_flags_varaccess access);
+/*
+ * Return the name of the storage.
+ */
+const char *env_flags_get_varstorage_name(enum env_flags_varstorage storage);
 #endif
 
 /*
@@ -103,10 +119,19 @@  enum env_flags_vartype env_flags_parse_vartype(const char *flags);
  * Parse the flags string from a .flags attribute list into the varaccess enum.
  */
 enum env_flags_varaccess env_flags_parse_varaccess(const char *flags);
+/*
+ * Parse the flags string from a .flags attribute list into the varstorage enum.
+ */
+enum env_flags_varstorage env_flags_parse_varstorage(const char *flags);
 /*
  * Parse the binary flags from a hash table entry into the varaccess enum.
  */
 enum env_flags_varaccess env_flags_parse_varaccess_from_binflags(int binflags);
+/*
+ * Parse the binary flags from a hash table entry into the varstorage enum.
+ */
+enum env_flags_varstorage
+env_flags_parse_varstorage_from_binflags(int binflags);
 
 #ifdef CONFIG_CMD_NET
 /*
@@ -124,6 +149,10 @@  enum env_flags_vartype env_flags_get_type(const char *name);
  * Look up the access of a variable directly from the .flags var.
  */
 enum env_flags_varaccess env_flags_get_access(const char *name);
+/*
+ * Look up the storage of a variable directly from the .flags var.
+ */
+enum env_flags_varstorage env_flags_get_storage(const char *name);
 /*
  * Validate the newval for its type to conform with the requirements defined by
  * its flags (directly looked at the .flags var).
@@ -173,5 +202,10 @@  int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op,
 #define ENV_FLAGS_VARACCESS_PREVENT_OVERWR		0x00000020
 #define ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR	0x00000040
 #define ENV_FLAGS_VARACCESS_BIN_MASK			0x00000078
+#define ENV_FLAGS_VARSTORAGE_VOLATILE			0x00000000
+#define ENV_FLAGS_VARSTORAGE_NON_VOLATILE		0x00000080
+#define ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE	0x00000100
+#define ENV_FLAGS_VARSTORAGE_RESERVED			0x00000180
+#define ENV_FLAGS_VARSTORAGE_BIN_MASK			0x00000180
 
 #endif /* __ENV_FLAGS_H__ */