* [U-Boot] [RFC, PATCH v4 01/16] hashtable: extend interfaces to handle entries with context
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 6:58 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context AKASHI Takahiro
` (16 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
The following interfaces are extended to accept an additional argument,
context:
hsearch_r() -> hsearch_ext()
hmatch_r() -> hmatch_ext()
hdelete_r() -> hdelete_ext()
hexport_r() -> hexport_ext()
himport_r() -> himport_ext()
A context of an entry does not have any specific meaning in hash
routines, except that it can be used one of matching parameters.
This feature will be used to add multiple U-Boot environment context
support in successive commits.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
include/search.h | 15 +++++++++
lib/hashtable.c | 80 +++++++++++++++++++++++++++++++++++++++---------
2 files changed, 81 insertions(+), 14 deletions(-)
diff --git a/include/search.h b/include/search.h
index 5d07b49073cc..9cece695d726 100644
--- a/include/search.h
+++ b/include/search.h
@@ -31,6 +31,7 @@ typedef enum {
} ACTION;
typedef struct entry {
+ unsigned int context; /* opaque data for table operations */
const char *key;
char *data;
int (*callback)(const char *name, const char *value, enum env_op op,
@@ -77,6 +78,8 @@ extern void hdestroy_r(struct hsearch_data *__htab);
* */
extern int hsearch_r(ENTRY __item, ACTION __action, ENTRY ** __retval,
struct hsearch_data *__htab, int __flag);
+extern int hsearch_ext(ENTRY __item, ACTION __action, ENTRY ** __retval,
+ struct hsearch_data *__htab, int __flag);
/*
* Search for an entry matching "__match". Otherwise, Same semantics
@@ -84,14 +87,22 @@ extern int hsearch_r(ENTRY __item, ACTION __action, ENTRY ** __retval,
*/
extern int hmatch_r(const char *__match, int __last_idx, ENTRY ** __retval,
struct hsearch_data *__htab);
+extern int hmatch_ext(const char *__match, int __last_idx, ENTRY ** __retval,
+ struct hsearch_data *__htab, unsigned int ctx);
/* Search and delete entry matching "__key" in internal hash table. */
extern int hdelete_r(const char *__key, struct hsearch_data *__htab,
int __flag);
+extern int hdelete_ext(const char *__key, struct hsearch_data *__htab,
+ unsigned int ctx, int __flag);
extern ssize_t hexport_r(struct hsearch_data *__htab,
const char __sep, int __flag, char **__resp, size_t __size,
int argc, char * const argv[]);
+extern ssize_t hexport_ext(struct hsearch_data *__htab, unsigned int ctx,
+ const char __sep, int __flag,
+ char **__resp, size_t __size,
+ int argc, char * const argv[]);
/*
* nvars: length of vars array
@@ -101,6 +112,10 @@ extern int himport_r(struct hsearch_data *__htab,
const char *__env, size_t __size, const char __sep,
int __flag, int __crlf_is_lf, int nvars,
char * const vars[]);
+extern int himport_ext(struct hsearch_data *__htab, unsigned int ctx,
+ const char *__env, size_t __size, const char __sep,
+ int __flag, int __crlf_is_lf, int nvars,
+ char * const vars[]);
/* Walk the whole table calling the callback on each element */
extern int hwalk_r(struct hsearch_data *__htab, int (*callback)(ENTRY *));
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 0d288d12d991..d6975d9cafc6 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -200,8 +200,8 @@ void hdestroy_r(struct hsearch_data *htab)
* example for functions like hdelete().
*/
-int hmatch_r(const char *match, int last_idx, ENTRY ** retval,
- struct hsearch_data *htab)
+int hmatch_ext(const char *match, int last_idx, ENTRY ** retval,
+ struct hsearch_data *htab, unsigned int ctx)
{
unsigned int idx;
size_t key_len = strlen(match);
@@ -209,6 +209,8 @@ int hmatch_r(const char *match, int last_idx, ENTRY ** retval,
for (idx = last_idx + 1; idx < htab->size; ++idx) {
if (htab->table[idx].used <= 0)
continue;
+ if (htab->table[idx].entry.context != ctx)
+ continue;
if (!strncmp(match, htab->table[idx].entry.key, key_len)) {
*retval = &htab->table[idx].entry;
return idx;
@@ -220,6 +222,12 @@ int hmatch_r(const char *match, int last_idx, ENTRY ** retval,
return 0;
}
+int hmatch_r(const char *match, int last_idx, ENTRY ** retval,
+ struct hsearch_data *htab)
+{
+ return hmatch_ext(match, last_idx, retval, htab, 0);
+}
+
/*
* Compare an existing entry with the desired key, and overwrite if the action
* is ENTER. This is simply a helper function for hsearch_r().
@@ -230,6 +238,14 @@ static inline int _compare_and_overwrite_entry(ENTRY item, ACTION action,
{
if (htab->table[idx].used == hval
&& strcmp(item.key, htab->table[idx].entry.key) == 0) {
+ /* No duplicated keys allowed */
+ if (item.context != htab->table[idx].entry.context) {
+ debug("context mismatch %s\n", item.key);
+ __set_errno(EINVAL);
+ *retval = NULL;
+ return 0;
+ }
+
/* Overwrite existing value? */
if ((action == ENTER) && (item.data != NULL)) {
/* check for permission */
@@ -270,8 +286,8 @@ static inline int _compare_and_overwrite_entry(ENTRY item, ACTION action,
return -1;
}
-int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
- struct hsearch_data *htab, int flag)
+int hsearch_ext(ENTRY item, ACTION action, ENTRY ** retval,
+ struct hsearch_data *htab, int flag)
{
unsigned int hval;
unsigned int count;
@@ -371,6 +387,7 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
idx = first_deleted;
htab->table[idx].used = hval;
+ htab->table[idx].entry.context = item.context;
htab->table[idx].entry.key = strdup(item.key);
htab->table[idx].entry.data = strdup(item.data);
if (!htab->table[idx].entry.key ||
@@ -420,6 +437,15 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
return 0;
}
+int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
+ struct hsearch_data *htab, int flag)
+{
+ ENTRY e;
+
+ e = item;
+ e.context = 0;
+ return hsearch_ext(e, action, retval, htab, flag);
+}
/*
* hdelete()
@@ -445,16 +471,18 @@ static void _hdelete(const char *key, struct hsearch_data *htab, ENTRY *ep,
--htab->filled;
}
-int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
+int hdelete_ext(const char *key, struct hsearch_data *htab, unsigned int ctx,
+ int flag)
{
ENTRY e, *ep;
int idx;
debug("hdelete: DELETE key \"%s\"\n", key);
+ e.context = ctx;
e.key = (char *)key;
- idx = hsearch_r(e, FIND, &ep, htab, 0);
+ idx = hsearch_ext(e, FIND, &ep, htab, 0);
if (idx == 0) {
__set_errno(ESRCH);
return 0; /* not found */
@@ -483,6 +511,11 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
return 1;
}
+int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
+{
+ return hdelete_ext(key, htab, 0, flag);
+}
+
#if !(defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_SAVEENV))
/*
* hexport()
@@ -592,9 +625,9 @@ static int match_entry(ENTRY *ep, int flag,
return 0;
}
-ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag,
- char **resp, size_t size,
- int argc, char * const argv[])
+ssize_t hexport_ext(struct hsearch_data *htab, unsigned int ctx,
+ const char sep, int flag, char **resp, size_t size,
+ int argc, char * const argv[])
{
ENTRY *list[htab->size];
char *res, *p;
@@ -618,8 +651,12 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag,
if (htab->table[i].used > 0) {
ENTRY *ep = &htab->table[i].entry;
- int found = match_entry(ep, flag, argc, argv);
+ int found;
+ if (ep->context != ctx)
+ continue;
+
+ found = match_entry(ep, flag, argc, argv);
if ((argc > 0) && (found == 0))
continue;
@@ -709,8 +746,14 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag,
return size;
}
-#endif
+ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag,
+ char **resp, size_t size,
+ int argc, char * const argv[])
+{
+ return hexport_ext(htab, 0, sep, flag, resp, size, argc, argv);
+}
+#endif
/*
* himport()
@@ -782,7 +825,7 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[])
* '\0' and '\n' have really been tested.
*/
-int himport_r(struct hsearch_data *htab,
+int himport_ext(struct hsearch_data *htab, unsigned int ctx,
const char *env, size_t size, const char sep, int flag,
int crlf_is_lf, int nvars, char * const vars[])
{
@@ -898,7 +941,7 @@ int himport_r(struct hsearch_data *htab,
if (!drop_var_from_set(name, nvars, localvars))
continue;
- if (hdelete_r(name, htab, flag) == 0)
+ if (hdelete_ext(name, htab, ctx, flag) == 0)
debug("DELETE ERROR ##############################\n");
continue;
@@ -926,10 +969,11 @@ int himport_r(struct hsearch_data *htab,
continue;
/* enter into hash table */
+ e.context = ctx;
e.key = name;
e.data = value;
- hsearch_r(e, ENTER, &rv, htab, flag);
+ hsearch_ext(e, ENTER, &rv, htab, flag);
if (rv == NULL)
printf("himport_r: can't insert \"%s=%s\" into hash table\n",
name, value);
@@ -968,6 +1012,14 @@ end:
return 1; /* everything OK */
}
+int himport_r(struct hsearch_data *htab,
+ const char *env, size_t size, const char sep, int flag,
+ int crlf_is_lf, int nvars, char * const vars[])
+{
+ return himport_ext(htab, 0, env, size, sep, flag, crlf_is_lf,
+ nvars, vars);
+}
+
/*
* hwalk_r()
*/
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 01/16] hashtable: extend interfaces to handle entries with context
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 01/16] hashtable: extend interfaces to handle entries with context AKASHI Takahiro
@ 2019-07-19 6:58 ` Wolfgang Denk
2019-07-19 7:44 ` AKASHI Takahiro
0 siblings, 1 reply; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 6:58 UTC (permalink / raw)
To: u-boot
Dear AKASHI Takahiro,
In message <20190717082525.891-2-takahiro.akashi@linaro.org> you wrote:
> The following interfaces are extended to accept an additional argument,
> context:
> hsearch_r() -> hsearch_ext()
> hmatch_r() -> hmatch_ext()
> hdelete_r() -> hdelete_ext()
> hexport_r() -> hexport_ext()
> himport_r() -> himport_ext()
Please avoid such renaming; keep the exiting names as is, and just
add new names (with descriptive names) where needed.
> --- a/include/search.h
> +++ b/include/search.h
> @@ -31,6 +31,7 @@ typedef enum {
> } ACTION;
>
> typedef struct entry {
> + unsigned int context; /* opaque data for table operations */
> const char *key;
Please keep the key the first entry.
> @@ -230,6 +238,14 @@ static inline int _compare_and_overwrite_entry(ENTRY item, ACTION action,
> {
> if (htab->table[idx].used == hval
> && strcmp(item.key, htab->table[idx].entry.key) == 0) {
> + /* No duplicated keys allowed */
> + if (item.context != htab->table[idx].entry.context) {
> + debug("context mismatch %s\n", item.key);
> + __set_errno(EINVAL);
> + *retval = NULL;
> + return 0;
> + }
If I understand correctly what you are doing here, then this needs a
different solution. As is, the code just fails without a way to fix
this. And the reason is that the matching is only done on the key,
while actually he context is important as well. The logical
conclusion seems to be that the context must be part of the key.
> +int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
> + struct hsearch_data *htab, int flag)
> +{
> + ENTRY e;
> +
> + e = item;
> + e.context = 0;
Please do not use '0' here; use the proper enum name.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In the beginning, I was made. I didn't ask to be made. No one consul-
ted with me or considered my feelings in this matter. But if it
brought some passing fancy to some lowly humans as they haphazardly
pranced their way through life's mournful jungle, then so be it.
- Marvin the Paranoid Android
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 01/16] hashtable: extend interfaces to handle entries with context
2019-07-19 6:58 ` Wolfgang Denk
@ 2019-07-19 7:44 ` AKASHI Takahiro
2019-07-19 9:49 ` Wolfgang Denk
0 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-19 7:44 UTC (permalink / raw)
To: u-boot
On Fri, Jul 19, 2019 at 08:58:56AM +0200, Wolfgang Denk wrote:
> Dear AKASHI Takahiro,
>
> In message <20190717082525.891-2-takahiro.akashi@linaro.org> you wrote:
> > The following interfaces are extended to accept an additional argument,
> > context:
> > hsearch_r() -> hsearch_ext()
> > hmatch_r() -> hmatch_ext()
> > hdelete_r() -> hdelete_ext()
> > hexport_r() -> hexport_ext()
> > himport_r() -> himport_ext()
>
> Please avoid such renaming; keep the exiting names as is, and just
> add new names (with descriptive names) where needed.
As I said in a reply to your previous comment, I have not renamed them.
> > --- a/include/search.h
> > +++ b/include/search.h
> > @@ -31,6 +31,7 @@ typedef enum {
> > } ACTION;
> >
> > typedef struct entry {
> > + unsigned int context; /* opaque data for table operations */
> > const char *key;
>
> Please keep the key the first entry.
Why?
>
> > @@ -230,6 +238,14 @@ static inline int _compare_and_overwrite_entry(ENTRY item, ACTION action,
> > {
> > if (htab->table[idx].used == hval
> > && strcmp(item.key, htab->table[idx].entry.key) == 0) {
> > + /* No duplicated keys allowed */
> > + if (item.context != htab->table[idx].entry.context) {
> > + debug("context mismatch %s\n", item.key);
> > + __set_errno(EINVAL);
> > + *retval = NULL;
> > + return 0;
> > + }
>
> If I understand correctly what you are doing here, then this needs a
> different solution. As is, the code just fails without a way to fix
> this. And the reason is that the matching is only done on the key,
> while actually he context is important as well.
The intention here is to prevent a context of any entry from
being changed. I believe that this is a reasonable assumption
and restriction.
> The logical
> conclusion seems to be that the context must be part of the key.
Do you mean that an actual name of variable, for example, "foo"
should be, say, "uboot_foo"?
This will also make the implementation of env_save/load a bit ugly again.
> > +int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
> > + struct hsearch_data *htab, int flag)
> > +{
> > + ENTRY e;
> > +
> > + e = item;
> > + e.context = 0;
>
> Please do not use '0' here; use the proper enum name.
I intentionally did so because I don't want to pollute "hash table"
functions with env-specific features so that hash functions will be
used for other purposes.
I admit that it is already polluted with flags though.
-Takahiro Akashi
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> In the beginning, I was made. I didn't ask to be made. No one consul-
> ted with me or considered my feelings in this matter. But if it
> brought some passing fancy to some lowly humans as they haphazardly
> pranced their way through life's mournful jungle, then so be it.
> - Marvin the Paranoid Android
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 01/16] hashtable: extend interfaces to handle entries with context
2019-07-19 7:44 ` AKASHI Takahiro
@ 2019-07-19 9:49 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 9:49 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190719074421.GQ21948@linaro.org> you wrote:
>
> > > hsearch_r() -> hsearch_ext()
> > > hmatch_r() -> hmatch_ext()
> > > hdelete_r() -> hdelete_ext()
> > > hexport_r() -> hexport_ext()
> > > himport_r() -> himport_ext()
> >
> > Please avoid such renaming; keep the exiting names as is, and just
> > add new names (with descriptive names) where needed.
>
> As I said in a reply to your previous comment, I have not renamed them.
Effectively this is what you are doing. All teh names in the code
get changes, without need.
> > > typedef struct entry {
> > > + unsigned int context; /* opaque data for table operations */
> > > const char *key;
> >
> > Please keep the key the first entry.
>
> Why?
Because of the significance, for aligment, etc.
> > > && strcmp(item.key, htab->table[idx].entry.key) == 0) {
> > > + /* No duplicated keys allowed */
> > > + if (item.context != htab->table[idx].entry.context) {
> > > + debug("context mismatch %s\n", item.key);
> > > + __set_errno(EINVAL);
> > > + *retval = NULL;
> > > + return 0;
> > > + }
> >
> > If I understand correctly what you are doing here, then this needs a
> > different solution. As is, the code just fails without a way to fix
> > this. And the reason is that the matching is only done on the key,
> > while actually he context is important as well.
>
> The intention here is to prevent a context of any entry from
> being changed. I believe that this is a reasonable assumption
> and restriction.
This raises an interesting question - do have contexts separate
namespaces, i. e. can we have the variable "foo" both for U-Boot and
for UEFI, with different values? Looking at the external storage
format, we could, as contets do not share storage location. Looking
at the internal storage, we could, if we make the "key" not only
consist of the name, but of a [name, context] pair.
This mightbe confusing for plain "printf", but this would not hurt, I
think.
I have no strong position on this. Using a common name space is
maybe a bit easier code-wise, but not much. In any case, the error
message shoulkd be fixed - as is, the end user would not understand
what the problem was.
> > The logical
> > conclusion seems to be that the context must be part of the key.
>
> Do you mean that an actual name of variable, for example, "foo"
> should be, say, "uboot_foo"?
No, but the hash table shoulw eventually use a [name, context] pair
as key, at least when you implement separate name spaces.
> This will also make the implementation of env_save/load a bit ugly again.
>
> > > +int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
> > > + struct hsearch_data *htab, int flag)
> > > +{
> > > + ENTRY e;
> > > +
> > > + e = item;
> > > + e.context = 0;
> >
> > Please do not use '0' here; use the proper enum name.
>
> I intentionally did so because I don't want to pollute "hash table"
> functions with env-specific features so that hash functions will be
> used for other purposes.
> I admit that it is already polluted with flags though.
And using '0' is based on the assumption that U-Boot is the first
enum, which is not documented anywhere. Please fix.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The pathology is to want control, not that you ever get it, because
of course you never do." - Gregory Bateson
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 01/16] hashtable: extend interfaces to handle entries with context AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 7:38 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 03/16] env: extend interfaces to label variable with context AKASHI Takahiro
` (15 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
The following interfaces are extended to accept an additional argument,
context:
env_import() -> env_import_ext()
env_export() -> env_export_ext()
env_load() -> env_load_ext()
env_save() -> env_save_ext()
With this patch applied, we will be able to have different U-Boot
environments each of which has its own "context," or dedicated
backing storage.
Currently, there are two contexts defined:
ENVCTX_UBOOT: the legacy U-Boot environment variables
ENVCTX_UEFI: UEFI variables which will be utilized in
successive commits.
Along with those changes, "env_t" structure is also modified so that
we will be able to load/save environment data of arbitrary size,
instead of fixed size of ENV_SIZE.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
env/common.c | 80 ++++++++++++++++++++++-----
env/env.c | 92 +++++++++++++++++++++++--------
include/asm-generic/global_data.h | 7 ++-
include/env_default.h | 1 +
include/environment.h | 85 +++++++++++++++++++++++++---
5 files changed, 220 insertions(+), 45 deletions(-)
diff --git a/env/common.c b/env/common.c
index bd340fe9d52d..c2a2d9f22feb 100644
--- a/env/common.c
+++ b/env/common.c
@@ -107,34 +107,48 @@ int set_default_vars(int nvars, char * const vars[], int flags)
* Check if CRC is valid and (if yes) import the environment.
* Note that "buf" may or may not be aligned.
*/
-int env_import(const char *buf, int check)
+int env_import_ext(const char *buf, enum env_context ctx, int check)
{
- env_t *ep = (env_t *)buf;
+ env_hdr_t *ep = (env_hdr_t *)buf;
if (check) {
uint32_t crc;
memcpy(&crc, &ep->crc, sizeof(crc));
- if (crc32(0, ep->data, ENV_SIZE) != crc) {
- set_default_env("bad CRC", 0);
+ if (crc32(0, ep->data, ep->data_size) != crc) {
+ if (ctx == ENVCTX_UBOOT)
+ set_default_env("bad CRC", 0);
+
return -ENOMSG; /* needed for env_load() */
}
}
- if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
- 0, NULL)) {
+ if (himport_ext(&env_htab, ctx, (char *)ep->data, ep->data_size,
+ /*
+ * FIXME:
+ * H_NOCLEAR is necessary here to handle
+ * multiple contexts simultaneously.
+ * This, however, breaks backward compatibility.
+ */
+ '\0', H_NOCLEAR, 0, 0, NULL)) {
gd->flags |= GD_FLG_ENV_READY;
return 0;
}
pr_err("Cannot import environment: errno = %d\n", errno);
- set_default_env("import failed", 0);
+ if (ctx == ENVCTX_UBOOT)
+ set_default_env("import failed", 0);
return -EIO;
}
+int env_import(const char *buf, int check)
+{
+ return env_import_ext(buf, ENVCTX_UBOOT, check);
+}
+
#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
static unsigned char env_flags;
@@ -199,30 +213,68 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
env_flags = ep->flags;
return env_import((char *)ep, 0);
}
+
+int env_import_redund_ext(const char *buf1, int buf1_read_fail,
+ const char *buf2, int buf2_read_fail,
+ enum env_context ctx)
+{
+ /* TODO */
+ return 0;
+}
#endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
/* Export the environment and generate CRC for it. */
-int env_export(env_t *env_out)
+int env_export_ext(env_hdr_t **env_out, enum env_context ctx)
{
- char *res;
+ unsigned char *data;
+ size_t size;
ssize_t len;
+ env_hdr_t *envp;
- res = (char *)env_out->data;
- len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
+ if (*env_out) {
+ data = (*env_out)->data;
+ size = (*env_out)->data_size;
+ } else {
+ data = NULL;
+ size = 0;
+ }
+ len = hexport_ext(&env_htab, ctx, '\0', 0, (char **)&data, size,
+ 0, NULL);
if (len < 0) {
- pr_err("Cannot export environment: errno = %d\n", errno);
+ pr_err("Cannot export environment: errno = %d\n",
+ errno);
return 1;
}
- env_out->crc = crc32(0, env_out->data, ENV_SIZE);
+ if (*env_out) {
+ envp = *env_out;
+ } else {
+ envp = malloc(sizeof(*envp) + len);
+ if (!envp) {
+ pr_err("Out of memory\n");
+ free(data);
+ return 1;
+ }
+ *env_out = envp;
+
+ envp->data_size = len;
+ memcpy(envp->data, data, len);
+ free(data);
+ }
+ envp->crc = crc32(0, envp->data, envp->data_size);
#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
- env_out->flags = ++env_flags; /* increase the serial */
+ envp->flags = ++env_flags; /* increase the serial */
#endif
return 0;
}
+int env_export(env_t *env_out)
+{
+ return env_export_ext((env_hdr_t **)&env_out, ENVCTX_UBOOT);
+}
+
void env_relocate(void)
{
#if defined(CONFIG_NEEDS_MANUAL_RELOC)
diff --git a/env/env.c b/env/env.c
index 4b417b90a291..e4c9f1d779a0 100644
--- a/env/env.c
+++ b/env/env.c
@@ -85,12 +85,12 @@ static enum env_location env_locations[] = {
#endif
};
-static bool env_has_inited(enum env_location location)
+static bool env_has_inited(enum env_context ctx, enum env_location location)
{
- return gd->env_has_init & BIT(location);
+ return gd->env_has_init[ctx] & BIT(location);
}
-static void env_set_inited(enum env_location location)
+static void env_set_inited(enum env_context ctx, enum env_location location)
{
/*
* We're using a 32-bits bitmask stored in gd (env_has_init)
@@ -99,7 +99,7 @@ static void env_set_inited(enum env_location location)
*/
BUILD_BUG_ON(ARRAY_SIZE(env_locations) > BITS_PER_LONG);
- gd->env_has_init |= BIT(location);
+ gd->env_has_init[ctx] |= BIT(location);
}
/**
@@ -120,16 +120,21 @@ static void env_set_inited(enum env_location location)
* Returns:
* an enum env_location value on success, a negative error code otherwise
*/
-__weak enum env_location env_get_location(enum env_operation op, int prio)
+__weak enum env_location env_get_location_ext(enum env_context ctx,
+ enum env_operation op, int prio)
{
if (prio >= ARRAY_SIZE(env_locations))
return ENVL_UNKNOWN;
- gd->env_load_prio = prio;
+ gd->env_load_prio[ctx] = prio;
return env_locations[prio];
}
+__weak enum env_location env_get_location(enum env_operation op, int prio)
+{
+ return env_get_location_ext(ENVCTX_UBOOT, op, prio);
+}
/**
* env_driver_lookup() - Finds the most suited environment location
@@ -143,11 +148,16 @@ __weak enum env_location env_get_location(enum env_operation op, int prio)
* Returns:
* NULL on error, a pointer to a struct env_driver otherwise
*/
-static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
+static struct env_driver *env_driver_lookup(enum env_context ctx,
+ enum env_operation op, int prio)
{
- enum env_location loc = env_get_location(op, prio);
+ enum env_location loc;
struct env_driver *drv;
+ if (ctx == ENVCTX_UBOOT)
+ loc = env_get_location(op, prio);
+ else
+ loc = env_get_location_ext(ctx, op, prio);
if (loc == ENVL_UNKNOWN)
return NULL;
@@ -174,19 +184,21 @@ int env_get_char(int index)
return env_get_char_spec(index);
}
-int env_load(void)
+int env_load_ext(enum env_context ctx)
{
struct env_driver *drv;
int best_prio = -1;
int prio;
- for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {
+ for (prio = 0; (drv = env_driver_lookup(ctx, ENVOP_LOAD, prio));
+ prio++) {
int ret;
- if (!drv->load)
+ if ((ctx == ENVCTX_UBOOT && !drv->load) ||
+ (ctx != ENVCTX_UBOOT && !drv->load_ext))
continue;
- if (!env_has_inited(drv->location))
+ if (!env_has_inited(ctx, drv->location))
continue;
printf("Loading Environment from %s... ", drv->name);
@@ -195,7 +207,10 @@ int env_load(void)
* drv->load() in some underlying API, and it must be exactly
* one message.
*/
- ret = drv->load();
+ if (ctx == ENVCTX_UBOOT)
+ ret = drv->load();
+ else
+ ret = drv->load_ext(ctx);
if (!ret) {
printf("OK\n");
return 0;
@@ -221,27 +236,39 @@ int env_load(void)
debug("Selecting environment with bad CRC\n");
else
best_prio = 0;
- env_get_location(ENVOP_LOAD, best_prio);
+ if (ctx == ENVCTX_UBOOT)
+ env_get_location(ENVOP_LOAD, best_prio);
+ else
+ env_get_location_ext(ctx, ENVOP_LOAD, best_prio);
return -ENODEV;
}
-int env_save(void)
+int env_load(void)
+{
+ return env_load_ext(ENVCTX_UBOOT);
+}
+
+int env_save_ext(enum env_context ctx)
{
struct env_driver *drv;
- drv = env_driver_lookup(ENVOP_SAVE, gd->env_load_prio);
+ drv = env_driver_lookup(ctx, ENVOP_SAVE, gd->env_load_prio[ctx]);
if (drv) {
int ret;
- if (!drv->save)
+ if ((ctx == ENVCTX_UBOOT && !drv->save) ||
+ (ctx != ENVCTX_UBOOT && !drv->save_ext))
return -ENODEV;
- if (!env_has_inited(drv->location))
+ if (!env_has_inited(ctx, drv->location))
return -ENODEV;
printf("Saving Environment to %s... ", drv->name);
- ret = drv->save();
+ if (ctx == ENVCTX_UBOOT)
+ ret = drv->save();
+ else
+ ret = drv->save_ext(ctx);
if (ret)
printf("Failed (%d)\n", ret);
else
@@ -254,15 +281,36 @@ int env_save(void)
return -ENODEV;
}
+int env_save(void)
+{
+ return env_save_ext(ENVCTX_UBOOT);
+}
+
int env_init(void)
{
struct env_driver *drv;
int ret = -ENOENT;
- int prio;
+ int ctx, prio;
+
+ /* other than ENVCTX_UBOOT */
+ for (ctx = 1; ctx < ENVCTX_COUNT; ctx++) {
+ for (prio = 0; (drv = env_driver_lookup(ctx, ENVOP_INIT, prio));
+ prio++) {
+ if (!drv->init_ext || !(ret = drv->init_ext(ctx)))
+ env_set_inited(ctx, drv->location);
+
+ debug("%s: Environment %s(%d) init done (ret=%d)\n",
+ __func__, drv->name, ctx, ret);
+ }
+ }
- for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
+ /* ENVCTX_UBOOT */
+ ret = -ENOENT;
+ for (prio = 0; (drv = env_driver_lookup(ENVCTX_UBOOT, ENVOP_INIT,
+ prio));
+ prio++) {
if (!drv->init || !(ret = drv->init()))
- env_set_inited(drv->location);
+ env_set_inited(ENVCTX_UBOOT, drv->location);
debug("%s: Environment %s init done (ret=%d)\n", __func__,
drv->name, ret);
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 02a3ed683821..cc2debfd2deb 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -20,6 +20,7 @@
*/
#ifndef __ASSEMBLY__
+#include <environment.h>
#include <fdtdec.h>
#include <membuff.h>
#include <linux/list.h>
@@ -50,8 +51,10 @@ typedef struct global_data {
#endif
unsigned long env_addr; /* Address of Environment struct */
unsigned long env_valid; /* Environment valid? enum env_valid */
- unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */
- int env_load_prio; /* Priority of the loaded environment */
+ unsigned long env_has_init[ENVCTX_COUNT_MAX];
+ /* Bitmask of boolean of struct env_location offsets */
+ int env_load_prio[ENVCTX_COUNT_MAX];
+ /* Priority of the loaded environment */
unsigned long ram_base; /* Base address of RAM used by U-Boot */
unsigned long ram_top; /* Top address of RAM used by U-Boot */
diff --git a/include/env_default.h b/include/env_default.h
index 86b639d3e283..dcf1523293f5 100644
--- a/include/env_default.h
+++ b/include/env_default.h
@@ -15,6 +15,7 @@ env_t environment __UBOOT_ENV_SECTION__(environment) = {
#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
1, /* Flags: valid */
#endif
+ ENV_SIZE,
{
#elif defined(DEFAULT_ENV_INSTANCE_STATIC)
static char default_environment[] = {
diff --git a/include/environment.h b/include/environment.h
index cd966761416e..9fa085a9b728 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -134,21 +134,31 @@ extern unsigned long nand_env_oob_offset;
#include "compiler.h"
#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
-# define ENV_HEADER_SIZE (sizeof(uint32_t) + 1)
-
# define ACTIVE_FLAG 1
# define OBSOLETE_FLAG 0
+
+#define ENVIRONMENT_HEADER \
+ uint32_t crc; /* CRC32 over data bytes */ \
+ uint32_t data_size; /* Environment data size */ \
+ unsigned char flags; /* active/obsolete flags */
#else
-# define ENV_HEADER_SIZE (sizeof(uint32_t))
+#define ENVIRONMENT_HEADER \
+ uint32_t crc; /* CRC32 over data bytes */ \
+ uint32_t data_size; /* Environment data size */
#endif
+typedef struct environment_hdr {
+ ENVIRONMENT_HEADER
+ unsigned char data[]; /* Environment data */
+} env_hdr_t;
+
+# define ENV_HEADER_SIZE (sizeof(env_hdr_t))
+
+/* For compatibility */
#define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
typedef struct environment_s {
- uint32_t crc; /* CRC32 over data bytes */
-#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
- unsigned char flags; /* active/obsolete flags */
-#endif
+ ENVIRONMENT_HEADER
unsigned char data[ENV_SIZE]; /* Environment data */
} env_t;
@@ -202,6 +212,11 @@ enum env_operation {
ENVOP_SAVE, /* we want to call the save function */
};
+enum env_context {
+ ENVCTX_UBOOT,
+ ENVCTX_COUNT,
+};
+
struct env_driver {
const char *name;
enum env_location location;
@@ -216,6 +231,17 @@ struct env_driver {
*/
int (*load)(void);
+ /**
+ * load_ext() - Load given environment context from storage
+ *
+ * This method is optional. If not provided, no environment will be
+ * loaded.
+ *
+ * @ctx: context to be loaded
+ * Return: 0 if OK, -ve on error
+ */
+ int (*load_ext)(enum env_context ctx);
+
/**
* save() - Save the environment to storage
*
@@ -225,6 +251,16 @@ struct env_driver {
*/
int (*save)(void);
+ /**
+ * save_ext() - Save given environment context to storage
+ *
+ * This method is required for 'saveenv' to work.
+ *
+ * @ctx: context to be saved
+ * Return: 0 if OK, -ve on error
+ */
+ int (*save_ext)(enum env_context ctx);
+
/**
* init() - Set up the initial pre-relocation environment
*
@@ -234,6 +270,17 @@ struct env_driver {
* other -ve on error
*/
int (*init)(void);
+
+ /**
+ * init() - Set up the initial pre-relocation environment
+ *
+ * This method is optional.
+ *
+ * @ctx: context to be saved
+ * @return 0 if OK, -ENOENT if no initial environment could be found,
+ * other -ve on error
+ */
+ int (*init_ext)(enum env_context ctx);
};
/* Declare a new environment location driver */
@@ -269,14 +316,19 @@ int set_default_vars(int nvars, char * const vars[], int flags);
/* Import from binary representation into hash table */
int env_import(const char *buf, int check);
+int env_import_ext(const char *buf, enum env_context ctx, int check);
/* Export from hash table into binary representation */
int env_export(env_t *env_out);
+int env_export_ext(env_hdr_t **env_out, enum env_context ctx);
#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
/* Select and import one of two redundant environments */
int env_import_redund(const char *buf1, int buf1_status,
const char *buf2, int buf2_status);
+int env_import_redund_ext(const char *buf1, int buf1_status,
+ const char *buf2, int buf2_status,
+ enum env_context ctx);
#endif
/**
@@ -296,6 +348,14 @@ int env_get_char(int index);
*/
int env_load(void);
+/**
+ * env_load_ext() - Load given environment context from storage
+ *
+ * @ctx: context to be loaded
+ * @return 0 if OK, -ve on error
+ */
+int env_load_ext(enum env_context ctx);
+
/**
* env_save() - Save the environment to storage
*
@@ -303,6 +363,14 @@ int env_load(void);
*/
int env_save(void);
+/**
+ * env_save_ext() - Save given environment context to storage
+ *
+ * @ctx: context to be saved
+ * @return 0 if OK, -ve on error
+ */
+int env_save_ext(enum env_context ctx);
+
/**
* env_fix_drivers() - Updates envdriver as per relocation
*/
@@ -314,4 +382,7 @@ int eth_env_set_enetaddr(const char *name, const uint8_t *enetaddr);
#endif /* DO_DEPS_ONLY */
+/* FIXME: ENVCTX_COUNT is protected by DO_DEPS_ONLY */
+#define ENVCTX_COUNT_MAX 2
+
#endif /* _ENVIRONMENT_H_ */
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context AKASHI Takahiro
@ 2019-07-19 7:38 ` Wolfgang Denk
2019-07-19 8:15 ` AKASHI Takahiro
0 siblings, 1 reply; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 7:38 UTC (permalink / raw)
To: u-boot
Dear AKASHI Takahiro,
In message <20190717082525.891-3-takahiro.akashi@linaro.org> you wrote:
> The following interfaces are extended to accept an additional argument,
> context:
> env_import() -> env_import_ext()
> env_export() -> env_export_ext()
> env_load() -> env_load_ext()
> env_save() -> env_save_ext()
Please don't such renames, see previous comments.
> -int env_import(const char *buf, int check)
> +int env_import_ext(const char *buf, enum env_context ctx, int check)
I think this needs at least additional documentation.
From the explantions so far, a context is always associated with a
variable, i. e. it is a property of the variable. Here it becomes
clear that the context has an additional meaning, separate storage.
It should be documented that one block of variables such as handled
by the env import / export routines will always only process
veriables of a specific context.
> - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
> - 0, NULL)) {
> + if (himport_ext(&env_htab, ctx, (char *)ep->data, ep->data_size,
> + /*
> + * FIXME:
> + * H_NOCLEAR is necessary here to handle
> + * multiple contexts simultaneously.
> + * This, however, breaks backward compatibility.
> + */
> + '\0', H_NOCLEAR, 0, 0, NULL)) {
This is the result of a deign bug. You can never have "multiple
contexts simultaneously", because the code as you designed it always
has only one context per data block - and I can;t see how this could
be changed, as the external storage format ('name=value\0') does not
provide a way to include variable-specific context information.
> - set_default_env("import failed", 0);
> + if (ctx == ENVCTX_UBOOT)
> + set_default_env("import failed", 0);
Should we not also have default settings for other contexts as well?
> +int env_import_redund_ext(const char *buf1, int buf1_read_fail,
> + const char *buf2, int buf2_read_fail,
> + enum env_context ctx)
> +{
> + /* TODO */
> + return 0;
!!
Do you plan to implement redundant storage for UEFI data as well? It
would make sense, wouldn't it?
> - pr_err("Cannot export environment: errno = %d\n", errno);
> + pr_err("Cannot export environment: errno = %d\n",
> + errno);
Why did you change this?
> - env_out->crc = crc32(0, env_out->data, ENV_SIZE);
> + if (*env_out) {
> + envp = *env_out;
> + } else {
> + envp = malloc(sizeof(*envp) + len);
> + if (!envp) {
> + pr_err("Out of memory\n");
> + free(data);
> + return 1;
> + }
> + *env_out = envp;
> +
> + envp->data_size = len;
> + memcpy(envp->data, data, len);
> + free(data);
> + }
It seems you have a memory leak here. I cannot find a free(envp)
anywhere.
Speaking about memory... what is the overall code / data size impact
of this patch set?
> --- a/env/env.c
> +++ b/env/env.c
> @@ -85,12 +85,12 @@ static enum env_location env_locations[] = {
> #endif
> };
>
> -static bool env_has_inited(enum env_location location)
> +static bool env_has_inited(enum env_context ctx, enum env_location location)
> {
> - return gd->env_has_init & BIT(location);
> + return gd->env_has_init[ctx] & BIT(location);
> }
This should be re-considered. GD is a tight resource, so increasing
it's size should be really justified. Also, I wonder if we really
should initialize all contexts the same. We should keep in mind
that many boards already need the (U-Boot) environment in SPL, but
there resource usage is critical in most cases.
I think it would make a lot of sense to initialize only the U-Boot
environment early (like in SPL), and delay this for all other
contexts until U-Boot porper is running, where we have sufficient
resources available.
> -static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
> +static struct env_driver *env_driver_lookup(enum env_context ctx,
> + enum env_operation op, int prio)
> {
> - enum env_location loc = env_get_location(op, prio);
> + enum env_location loc;
> struct env_driver *drv;
>
> + if (ctx == ENVCTX_UBOOT)
> + loc = env_get_location(op, prio);
> + else
> + loc = env_get_location_ext(ctx, op, prio);
This makes no sense. ENVCTX_UBOOT is just one of the available
contexts; no "if" should be needed here.
> - if (!drv->load)
> + if ((ctx == ENVCTX_UBOOT && !drv->load) ||
> + (ctx != ENVCTX_UBOOT && !drv->load_ext))
> continue;
Ditto here.
> - ret = drv->load();
> + if (ctx == ENVCTX_UBOOT)
> + ret = drv->load();
> + else
> + ret = drv->load_ext(ctx);
...and here.
> - env_get_location(ENVOP_LOAD, best_prio);
> + if (ctx == ENVCTX_UBOOT)
> + env_get_location(ENVOP_LOAD, best_prio);
> + else
> + env_get_location_ext(ctx, ENVOP_LOAD, best_prio);
...and here.
> - if (!drv->save)
> + if ((ctx == ENVCTX_UBOOT && !drv->save) ||
> + (ctx != ENVCTX_UBOOT && !drv->save_ext))
> return -ENODEV;
...and here.
> - if (!env_has_inited(drv->location))
> + if (!env_has_inited(ctx, drv->location))
> return -ENODEV;
...and here.
> - ret = drv->save();
> + if (ctx == ENVCTX_UBOOT)
> + ret = drv->save();
> + else
> + ret = drv->save_ext(ctx);
...and here.
> int env_init(void)
> {
> struct env_driver *drv;
> int ret = -ENOENT;
> - int prio;
> + int ctx, prio;
Error. Context is an enum.
> + /* other than ENVCTX_UBOOT */
> + for (ctx = 1; ctx < ENVCTX_COUNT; ctx++) {
Ugly code. "ctx" is an enum, so "1" is a magic number here that
should not be used.
Please fix.
> - for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
> + /* ENVCTX_UBOOT */
> + ret = -ENOENT;
> + for (prio = 0; (drv = env_driver_lookup(ENVCTX_UBOOT, ENVOP_INIT,
> + prio));
> + prio++) {
See problem above. U-Boot context should not need separate code, it
is just one context among others...
...unless you really split initialization into early init (U-Boot,
eventually already in SPL) and late init (all other contexts,
delayed until U-Boot proper).
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -20,6 +20,7 @@
> */
>
> #ifndef __ASSEMBLY__
> +#include <environment.h>
> #include <fdtdec.h>
> #include <membuff.h>
> #include <linux/list.h>
> @@ -50,8 +51,10 @@ typedef struct global_data {
> #endif
> unsigned long env_addr; /* Address of Environment struct */
> unsigned long env_valid; /* Environment valid? enum env_valid */
> - unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */
> - int env_load_prio; /* Priority of the loaded environment */
> + unsigned long env_has_init[ENVCTX_COUNT_MAX];
> + /* Bitmask of boolean of struct env_location offsets */
> + int env_load_prio[ENVCTX_COUNT_MAX];
> + /* Priority of the loaded environment */
NAK. Please keep this information out of GD. This is a tight
resource, we must not extend it for such purposes.
> --- a/include/env_default.h
> +++ b/include/env_default.h
> @@ -15,6 +15,7 @@ env_t environment __UBOOT_ENV_SECTION__(environment) = {
> #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> 1, /* Flags: valid */
> #endif
> + ENV_SIZE,
Full NAK!!!
You *MUST NOT* change the data structure of the environment on
external storage, as this breaks compatibility with all existing
systems. This is a strict no-go.
> diff --git a/include/environment.h b/include/environment.h
> index cd966761416e..9fa085a9b728 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -134,21 +134,31 @@ extern unsigned long nand_env_oob_offset;
> #include "compiler.h"
>
> #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> -# define ENV_HEADER_SIZE (sizeof(uint32_t) + 1)
> -
> # define ACTIVE_FLAG 1
> # define OBSOLETE_FLAG 0
> +
> +#define ENVIRONMENT_HEADER \
> + uint32_t crc; /* CRC32 over data bytes */ \
> + uint32_t data_size; /* Environment data size */ \
> + unsigned char flags; /* active/obsolete flags */
> #else
> -# define ENV_HEADER_SIZE (sizeof(uint32_t))
> +#define ENVIRONMENT_HEADER \
> + uint32_t crc; /* CRC32 over data bytes */ \
> + uint32_t data_size; /* Environment data size */
> #endif
>
> +typedef struct environment_hdr {
> + ENVIRONMENT_HEADER
> + unsigned char data[]; /* Environment data */
> +} env_hdr_t;
> +
> +# define ENV_HEADER_SIZE (sizeof(env_hdr_t))
> +
> +/* For compatibility */
> #define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
>
> typedef struct environment_s {
> - uint32_t crc; /* CRC32 over data bytes */
> -#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> - unsigned char flags; /* active/obsolete flags */
> -#endif
> + ENVIRONMENT_HEADER
> unsigned char data[ENV_SIZE]; /* Environment data */
> } env_t;
Full NAK!!!
You *MUST NOT* change the data structure of the environment on
external storage, as this breaks compatibility with all existing
systems. This is a strict no-go.
> */
> int (*load)(void);
>
> + /**
> + * load_ext() - Load given environment context from storage
> + *
> + * This method is optional. If not provided, no environment will be
> + * loaded.
> + *
> + * @ctx: context to be loaded
> + * Return: 0 if OK, -ve on error
> + */
> + int (*load_ext)(enum env_context ctx);
> +
> /**
> * save() - Save the environment to storage
> *
> @@ -225,6 +251,16 @@ struct env_driver {
> */
> int (*save)(void);
>
> + /**
> + * save_ext() - Save given environment context to storage
> + *
> + * This method is required for 'saveenv' to work.
> + *
> + * @ctx: context to be saved
> + * Return: 0 if OK, -ve on error
> + */
> + int (*save_ext)(enum env_context ctx);
> +
> /**
> * init() - Set up the initial pre-relocation environment
> *
> @@ -234,6 +270,17 @@ struct env_driver {
> * other -ve on error
> */
> int (*init)(void);
> +
> + /**
> + * init() - Set up the initial pre-relocation environment
> + *
> + * This method is optional.
> + *
> + * @ctx: context to be saved
> + * @return 0 if OK, -ENOENT if no initial environment could be found,
> + * other -ve on error
> + */
> + int (*init_ext)(enum env_context ctx);
NAK. We should ot need always pairs of functions foo() and
foo_ext(). There should always be only foo(), which supports
contexts.
> /* Import from binary representation into hash table */
> int env_import(const char *buf, int check);
> +int env_import_ext(const char *buf, enum env_context ctx, int check);
>
> /* Export from hash table into binary representation */
> int env_export(env_t *env_out);
> +int env_export_ext(env_hdr_t **env_out, enum env_context ctx);
>
> #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> /* Select and import one of two redundant environments */
> int env_import_redund(const char *buf1, int buf1_status,
> const char *buf2, int buf2_status);
> +int env_import_redund_ext(const char *buf1, int buf1_status,
> + const char *buf2, int buf2_status,
> + enum env_context ctx);
> #endif
Ditto here and following.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
panic: kernel trap (ignored)
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context
2019-07-19 7:38 ` Wolfgang Denk
@ 2019-07-19 8:15 ` AKASHI Takahiro
2019-07-19 10:04 ` Wolfgang Denk
0 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-19 8:15 UTC (permalink / raw)
To: u-boot
On Fri, Jul 19, 2019 at 09:38:11AM +0200, Wolfgang Denk wrote:
> Dear AKASHI Takahiro,
>
> In message <20190717082525.891-3-takahiro.akashi@linaro.org> you wrote:
> > The following interfaces are extended to accept an additional argument,
> > context:
> > env_import() -> env_import_ext()
> > env_export() -> env_export_ext()
> > env_load() -> env_load_ext()
> > env_save() -> env_save_ext()
>
> Please don't such renames, see previous comments.
I didn't rename them.
> > -int env_import(const char *buf, int check)
> > +int env_import_ext(const char *buf, enum env_context ctx, int check)
env_import() is re-defined using env_import_ext() below.
> I think this needs at least additional documentation.
>
> From the explantions so far, a context is always associated with a
> variable, i. e. it is a property of the variable. Here it becomes
> clear that the context has an additional meaning, separate storage.
I described in my cover letter.
> It should be documented that one block of variables such as handled
> by the env import / export routines will always only process
> veriables of a specific context.
Okay, but this is not specific to this function.
I'm going to add detailed function descriptions
if you agree with these new interfaces. Do you?
>
> > - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
> > - 0, NULL)) {
> > + if (himport_ext(&env_htab, ctx, (char *)ep->data, ep->data_size,
> > + /*
> > + * FIXME:
> > + * H_NOCLEAR is necessary here to handle
> > + * multiple contexts simultaneously.
> > + * This, however, breaks backward compatibility.
> > + */
> > + '\0', H_NOCLEAR, 0, 0, NULL)) {
>
> This is the result of a deign bug. You can never have "multiple
> contexts simultaneously", because the code as you designed it always
> has only one context per data block - and I can;t see how this could
> be changed, as the external storage format ('name=value\0') does not
> provide a way to include variable-specific context information.
Please read path#14 where a fat driver is modified to maintain UEFI
variables in U-Boot environment hash tables.
If UEFI variables need not be maintained in a single U-Boot environment,
my whole patch set goes in vain. (This is my "Plan B" implementation.)
> > - set_default_env("import failed", 0);
> > + if (ctx == ENVCTX_UBOOT)
> > + set_default_env("import failed", 0);
>
> Should we not also have default settings for other contexts as well?
Handling for default (initial) variables may vary context to context.
It should be implemented depending on their requirements.
> > +int env_import_redund_ext(const char *buf1, int buf1_read_fail,
> > + const char *buf2, int buf2_read_fail,
> > + enum env_context ctx)
> > +{
> > + /* TODO */
> > + return 0;
>
> !!
>
> Do you plan to implement redundant storage for UEFI data as well? It
> would make sense, wouldn't it?
Yes and no.
My current focus is to determine if my approach is acceptable or not.
> > - pr_err("Cannot export environment: errno = %d\n", errno);
> > + pr_err("Cannot export environment: errno = %d\n",
> > + errno);
>
> Why did you change this?
Maybe a mistake?
> > - env_out->crc = crc32(0, env_out->data, ENV_SIZE);
> > + if (*env_out) {
> > + envp = *env_out;
> > + } else {
> > + envp = malloc(sizeof(*envp) + len);
> > + if (!envp) {
> > + pr_err("Out of memory\n");
> > + free(data);
> > + return 1;
> > + }
> > + *env_out = envp;
> > +
> > + envp->data_size = len;
> > + memcpy(envp->data, data, len);
> > + free(data);
> > + }
>
> It seems you have a memory leak here. I cannot find a free(envp)
> anywhere.
envp will be returned to a caller via env_out.
> Speaking about memory... what is the overall code / data size impact
> of this patch set?
No statistics yet.
>
> > --- a/env/env.c
> > +++ b/env/env.c
> > @@ -85,12 +85,12 @@ static enum env_location env_locations[] = {
> > #endif
> > };
> >
> > -static bool env_has_inited(enum env_location location)
> > +static bool env_has_inited(enum env_context ctx, enum env_location location)
> > {
> > - return gd->env_has_init & BIT(location);
> > + return gd->env_has_init[ctx] & BIT(location);
> > }
>
> This should be re-considered. GD is a tight resource, so increasing
> it's size should be really justified. Also, I wonder if we really
> should initialize all contexts the same. We should keep in mind
> that many boards already need the (U-Boot) environment in SPL, but
> there resource usage is critical in most cases.
Yes, I think that I have not paid enough attentions to SPL case.
I will appreciate your suggestions.
> I think it would make a lot of sense to initialize only the U-Boot
> environment early (like in SPL), and delay this for all other
> contexts until U-Boot porper is running, where we have sufficient
> resources available.
Yeah, but see my comment below.
>
> > -static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
> > +static struct env_driver *env_driver_lookup(enum env_context ctx,
> > + enum env_operation op, int prio)
> > {
> > - enum env_location loc = env_get_location(op, prio);
> > + enum env_location loc;
> > struct env_driver *drv;
> >
> > + if (ctx == ENVCTX_UBOOT)
> > + loc = env_get_location(op, prio);
> > + else
> > + loc = env_get_location_ext(ctx, op, prio);
>
> This makes no sense. ENVCTX_UBOOT is just one of the available
> contexts; no "if" should be needed here.
NAK.
env_get_location is currently defined as a weak function.
So this hack is necessary, in particular, on a system where
UEFI is not enabled and env_get_location is yet overwritten.
> > - if (!drv->load)
> > + if ((ctx == ENVCTX_UBOOT && !drv->load) ||
> > + (ctx != ENVCTX_UBOOT && !drv->load_ext))
> > continue;
>
> Ditto here.
>
> > - ret = drv->load();
> > + if (ctx == ENVCTX_UBOOT)
> > + ret = drv->load();
> > + else
> > + ret = drv->load_ext(ctx);
>
> ...and here.
>
> > - env_get_location(ENVOP_LOAD, best_prio);
> > + if (ctx == ENVCTX_UBOOT)
> > + env_get_location(ENVOP_LOAD, best_prio);
> > + else
> > + env_get_location_ext(ctx, ENVOP_LOAD, best_prio);
>
> ...and here.
>
> > - if (!drv->save)
> > + if ((ctx == ENVCTX_UBOOT && !drv->save) ||
> > + (ctx != ENVCTX_UBOOT && !drv->save_ext))
> > return -ENODEV;
>
> ...and here.
>
> > - if (!env_has_inited(drv->location))
> > + if (!env_has_inited(ctx, drv->location))
> > return -ENODEV;
>
> ...and here.
>
> > - ret = drv->save();
> > + if (ctx == ENVCTX_UBOOT)
> > + ret = drv->save();
> > + else
> > + ret = drv->save_ext(ctx);
>
> ...and here.
>
>
> > int env_init(void)
> > {
> > struct env_driver *drv;
> > int ret = -ENOENT;
> > - int prio;
> > + int ctx, prio;
>
> Error. Context is an enum.
>
> > + /* other than ENVCTX_UBOOT */
> > + for (ctx = 1; ctx < ENVCTX_COUNT; ctx++) {
>
> Ugly code. "ctx" is an enum, so "1" is a magic number here that
> should not be used.
There are already bunch of code where enum is interchangeably
used as an integer.
> Please fix.
>
> > - for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
> > + /* ENVCTX_UBOOT */
> > + ret = -ENOENT;
> > + for (prio = 0; (drv = env_driver_lookup(ENVCTX_UBOOT, ENVOP_INIT,
> > + prio));
> > + prio++) {
>
> See problem above. U-Boot context should not need separate code, it
> is just one context among others...
It seems to me that this comment is conflicting with your assertion
below.
> ...unless you really split initialization into early init (U-Boot,
> eventually already in SPL) and late init (all other contexts,
> delayed until U-Boot proper).
>
> > --- a/include/asm-generic/global_data.h
> > +++ b/include/asm-generic/global_data.h
> > @@ -20,6 +20,7 @@
> > */
> >
> > #ifndef __ASSEMBLY__
> > +#include <environment.h>
> > #include <fdtdec.h>
> > #include <membuff.h>
> > #include <linux/list.h>
> > @@ -50,8 +51,10 @@ typedef struct global_data {
> > #endif
> > unsigned long env_addr; /* Address of Environment struct */
> > unsigned long env_valid; /* Environment valid? enum env_valid */
> > - unsigned long env_has_init; /* Bitmask of boolean of struct env_location offsets */
> > - int env_load_prio; /* Priority of the loaded environment */
> > + unsigned long env_has_init[ENVCTX_COUNT_MAX];
> > + /* Bitmask of boolean of struct env_location offsets */
> > + int env_load_prio[ENVCTX_COUNT_MAX];
> > + /* Priority of the loaded environment */
>
> NAK. Please keep this information out of GD. This is a tight
> resource, we must not extend it for such purposes.
But in this way, we will have to handle contexts differently
depending on whether it is ENVCTX_UBOOT or not.
> > --- a/include/env_default.h
> > +++ b/include/env_default.h
> > @@ -15,6 +15,7 @@ env_t environment __UBOOT_ENV_SECTION__(environment) = {
> > #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> > 1, /* Flags: valid */
> > #endif
> > + ENV_SIZE,
>
> Full NAK!!!
>
> You *MUST NOT* change the data structure of the environment on
> external storage, as this breaks compatibility with all existing
> systems. This is a strict no-go.
Let me think, but I don't have a good idea yet.
> > diff --git a/include/environment.h b/include/environment.h
> > index cd966761416e..9fa085a9b728 100644
> > --- a/include/environment.h
> > +++ b/include/environment.h
> > @@ -134,21 +134,31 @@ extern unsigned long nand_env_oob_offset;
> > #include "compiler.h"
> >
> > #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> > -# define ENV_HEADER_SIZE (sizeof(uint32_t) + 1)
> > -
> > # define ACTIVE_FLAG 1
> > # define OBSOLETE_FLAG 0
> > +
> > +#define ENVIRONMENT_HEADER \
> > + uint32_t crc; /* CRC32 over data bytes */ \
> > + uint32_t data_size; /* Environment data size */ \
> > + unsigned char flags; /* active/obsolete flags */
> > #else
> > -# define ENV_HEADER_SIZE (sizeof(uint32_t))
> > +#define ENVIRONMENT_HEADER \
> > + uint32_t crc; /* CRC32 over data bytes */ \
> > + uint32_t data_size; /* Environment data size */
> > #endif
> >
> > +typedef struct environment_hdr {
> > + ENVIRONMENT_HEADER
> > + unsigned char data[]; /* Environment data */
> > +} env_hdr_t;
> > +
> > +# define ENV_HEADER_SIZE (sizeof(env_hdr_t))
> > +
> > +/* For compatibility */
> > #define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
> >
> > typedef struct environment_s {
> > - uint32_t crc; /* CRC32 over data bytes */
> > -#ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> > - unsigned char flags; /* active/obsolete flags */
> > -#endif
> > + ENVIRONMENT_HEADER
> > unsigned char data[ENV_SIZE]; /* Environment data */
> > } env_t;
>
> Full NAK!!!
>
> You *MUST NOT* change the data structure of the environment on
> external storage, as this breaks compatibility with all existing
> systems. This is a strict no-go.
>
> > */
> > int (*load)(void);
> >
> > + /**
> > + * load_ext() - Load given environment context from storage
> > + *
> > + * This method is optional. If not provided, no environment will be
> > + * loaded.
> > + *
> > + * @ctx: context to be loaded
> > + * Return: 0 if OK, -ve on error
> > + */
> > + int (*load_ext)(enum env_context ctx);
> > +
> > /**
> > * save() - Save the environment to storage
> > *
> > @@ -225,6 +251,16 @@ struct env_driver {
> > */
> > int (*save)(void);
> >
> > + /**
> > + * save_ext() - Save given environment context to storage
> > + *
> > + * This method is required for 'saveenv' to work.
> > + *
> > + * @ctx: context to be saved
> > + * Return: 0 if OK, -ve on error
> > + */
> > + int (*save_ext)(enum env_context ctx);
> > +
> > /**
> > * init() - Set up the initial pre-relocation environment
> > *
> > @@ -234,6 +270,17 @@ struct env_driver {
> > * other -ve on error
> > */
> > int (*init)(void);
> > +
> > + /**
> > + * init() - Set up the initial pre-relocation environment
> > + *
> > + * This method is optional.
> > + *
> > + * @ctx: context to be saved
> > + * @return 0 if OK, -ENOENT if no initial environment could be found,
> > + * other -ve on error
> > + */
> > + int (*init_ext)(enum env_context ctx);
>
> NAK. We should ot need always pairs of functions foo() and
> foo_ext(). There should always be only foo(), which supports
> contexts.
This is a transition state as I mentioned in "known issues/TODOs"
in my cover letter.
If you agree, I can remove all the existing interfaces
but only when all the storage drivers are converted.
Thanks,
-Takahiro Akashi
> > /* Import from binary representation into hash table */
> > int env_import(const char *buf, int check);
> > +int env_import_ext(const char *buf, enum env_context ctx, int check);
> >
> > /* Export from hash table into binary representation */
> > int env_export(env_t *env_out);
> > +int env_export_ext(env_hdr_t **env_out, enum env_context ctx);
> >
> > #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> > /* Select and import one of two redundant environments */
> > int env_import_redund(const char *buf1, int buf1_status,
> > const char *buf2, int buf2_status);
> > +int env_import_redund_ext(const char *buf1, int buf1_status,
> > + const char *buf2, int buf2_status,
> > + enum env_context ctx);
> > #endif
>
> Ditto here and following.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> panic: kernel trap (ignored)
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context
2019-07-19 8:15 ` AKASHI Takahiro
@ 2019-07-19 10:04 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 10:04 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190719081556.GR21948@linaro.org> you wrote:
>
> Okay, but this is not specific to this function.
> I'm going to add detailed function descriptions
> if you agree with these new interfaces. Do you?
I agee with the interface, but not with the new names. We should
use the existing names, and not change them. Only add new functions
where needed.
> > > + if (himport_ext(&env_htab, ctx, (char *)ep->data, ep->data_size,
> > > + /*
> > > + * FIXME:
> > > + * H_NOCLEAR is necessary here to handle
> > > + * multiple contexts simultaneously.
> > > + * This, however, breaks backward compatibility.
> > > + */
> > > + '\0', H_NOCLEAR, 0, 0, NULL)) {
> >
> > This is the result of a deign bug. You can never have "multiple
> > contexts simultaneously", because the code as you designed it always
> > has only one context per data block - and I can;t see how this could
> > be changed, as the external storage format ('name=value\0') does not
> > provide a way to include variable-specific context information.
>
> Please read path#14 where a fat driver is modified to maintain UEFI
> variables in U-Boot environment hash tables.
>
> If UEFI variables need not be maintained in a single U-Boot environment,
> my whole patch set goes in vain. (This is my "Plan B" implementation.)
I was talking about external storage - there you can have only one
context in a data block. Internally (in the hash table), the context
should probably be art of the key, so there cannot be any such
conflicts either.
> > > - set_default_env("import failed", 0);
> > > + if (ctx == ENVCTX_UBOOT)
> > > + set_default_env("import failed", 0);
> >
> > Should we not also have default settings for other contexts as well?
>
> Handling for default (initial) variables may vary context to context.
> It should be implemented depending on their requirements.
It should be handled in a single, generic way, and we allow that a
context defines his own implementation (say, through weak default
handlers which can be overwritten by context-specific code).
> > > +int env_import_redund_ext(const char *buf1, int buf1_read_fail,
> > > + const char *buf2, int buf2_read_fail,
> > > + enum env_context ctx)
> > > +{
> > > + /* TODO */
> > > + return 0;
> >
> > !!
> >
> > Do you plan to implement redundant storage for UEFI data as well? It
> > would make sense, wouldn't it?
>
> Yes and no.
> My current focus is to determine if my approach is acceptable or not.
OK - but this shows clealy the disadvantages of defining new names.
Please get rid of all this _ext stuff...
> > It seems you have a memory leak here. I cannot find a free(envp)
> > anywhere.
>
> envp will be returned to a caller via env_out.
Yes, but where does it get freed?
> > Speaking about memory... what is the overall code / data size impact
> > of this patch set?
>
> No statistics yet.
Can you please provide some?
> > > + if (ctx == ENVCTX_UBOOT)
> > > + loc = env_get_location(op, prio);
> > > + else
> > > + loc = env_get_location_ext(ctx, op, prio);
> >
> > This makes no sense. ENVCTX_UBOOT is just one of the available
> > contexts; no "if" should be needed here.
>
> NAK.
> env_get_location is currently defined as a weak function.
> So this hack is necessary, in particular, on a system where
> UEFI is not enabled and env_get_location is yet overwritten.
Well, this mechanism need to be adapted for contexts, then It may
indeed makle sense to provide the same overwrite possibiltiy for
each context.
> > > - if (!drv->load)
> > > + if ((ctx == ENVCTX_UBOOT && !drv->load) ||
> > > + (ctx != ENVCTX_UBOOT && !drv->load_ext))
> > > continue;
> >
> > Ditto here.
> >
> > > - ret = drv->load();
> > > + if (ctx == ENVCTX_UBOOT)
> > > + ret = drv->load();
> > > + else
> > > + ret = drv->load_ext(ctx);
> >
> > ...and here.
> >
> > > - env_get_location(ENVOP_LOAD, best_prio);
> > > + if (ctx == ENVCTX_UBOOT)
> > > + env_get_location(ENVOP_LOAD, best_prio);
> > > + else
> > > + env_get_location_ext(ctx, ENVOP_LOAD, best_prio);
> >
> > ...and here.
> >
> > > - if (!drv->save)
> > > + if ((ctx == ENVCTX_UBOOT && !drv->save) ||
> > > + (ctx != ENVCTX_UBOOT && !drv->save_ext))
> > > return -ENODEV;
> >
> > ...and here.
> >
> > > - if (!env_has_inited(drv->location))
> > > + if (!env_has_inited(ctx, drv->location))
> > > return -ENODEV;
> >
> > ...and here.
> >
> > > - ret = drv->save();
> > > + if (ctx == ENVCTX_UBOOT)
> > > + ret = drv->save();
> > > + else
> > > + ret = drv->save_ext(ctx);
> >
> > ...and here.
All this code _begs_ for cleanup.
> >
> > > int env_init(void)
> > > {
> > > struct env_driver *drv;
> > > int ret = -ENOENT;
> > > - int prio;
> > > + int ctx, prio;
> >
> > Error. Context is an enum.
> >
> > > + /* other than ENVCTX_UBOOT */
> > > + for (ctx = 1; ctx < ENVCTX_COUNT; ctx++) {
> >
> > Ugly code. "ctx" is an enum, so "1" is a magic number here that
> > should not be used.
>
> There are already bunch of code where enum is interchangeably
> used as an integer.
There is no good excuse for following bad examples. There is not
even any documentation that mentions that ENVCTX_UBOOT has to be the
first entry in the enum.
> > > - for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
> > > + /* ENVCTX_UBOOT */
> > > + ret = -ENOENT;
> > > + for (prio = 0; (drv = env_driver_lookup(ENVCTX_UBOOT, ENVOP_INIT,
> > > + prio));
> > > + prio++) {
> >
> > See problem above. U-Boot context should not need separate code, it
> > is just one context among others...
>
> It seems to me that this comment is conflicting with your assertion
> below.
No. You can still iterate over the whole enum, and in the look
dosomething like "if (CTX == ENVCTX_UBOOT) continue;" or such,
without relying on a specific numeric value. This is much more
reliable.
And you can also extend this with proper testing for running in SPL
ir not, etc.
> > NAK. Please keep this information out of GD. This is a tight
> > resource, we must not extend it for such purposes.
>
> But in this way, we will have to handle contexts differently
> depending on whether it is ENVCTX_UBOOT or not.
Yes, indeed, U-Boot environment may be handled differently, but we
can still share the same code.
> > > --- a/include/env_default.h
> > > +++ b/include/env_default.h
> > > @@ -15,6 +15,7 @@ env_t environment __UBOOT_ENV_SECTION__(environment) = {
> > > #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT
> > > 1, /* Flags: valid */
> > > #endif
> > > + ENV_SIZE,
> >
> > Full NAK!!!
> >
> > You *MUST NOT* change the data structure of the environment on
> > external storage, as this breaks compatibility with all existing
> > systems. This is a strict no-go.
>
> Let me think, but I don't have a good idea yet.
You can only _pre_pend the size field in a bigger structure, which
has size first, followed by the existing struct env.
> > NAK. We should ot need always pairs of functions foo() and
> > foo_ext(). There should always be only foo(), which supports
> > contexts.
>
> This is a transition state as I mentioned in "known issues/TODOs"
> in my cover letter.
> If you agree, I can remove all the existing interfaces
> but only when all the storage drivers are converted.
Adding the additional context parameter seems not so much a problem.
This can be a single big commit including all the drivers, which get
passed a '0' argument which they may ignore. The introdduce
contexts as a scond step.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's hard to think of you as the end result of millions of years of
evolution.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 03/16] env: extend interfaces to label variable with context
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 01/16] hashtable: extend interfaces to handle entries with context AKASHI Takahiro
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:09 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 04/16] env: flash: add U-Boot environment context support AKASHI Takahiro
` (14 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
The following interfaces are extended to allow for accepting an additional
argument, env_context.
env_get() -> env_get_ext()
env_set() -> env_get_ext()
Relevant env commands are synced with this change to maintain the semantics
of existing U-Boot environment.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
cmd/nvedit.c | 82 ++++++++++++++++++++++++++++++++++-------------
include/exports.h | 3 ++
2 files changed, 62 insertions(+), 23 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 49d3b5bdf466..cc80ba712767 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -87,7 +87,7 @@ int get_env_id(void)
*
* Returns 0 in case of error, or length of printed string
*/
-static int env_print(char *name, int flag)
+static int env_print_ext(enum env_context ctx, char *name, int flag)
{
char *res = NULL;
ssize_t len;
@@ -96,6 +96,7 @@ static int env_print(char *name, int flag)
ENTRY e, *ep;
e.key = name;
+ e.context = ctx;
e.data = NULL;
hsearch_r(e, FIND, &ep, &env_htab, flag);
if (ep == NULL)
@@ -105,7 +106,7 @@ static int env_print(char *name, int flag)
}
/* print whole list */
- len = hexport_r(&env_htab, '\n', flag, &res, 0, 0, NULL);
+ len = hexport_ext(&env_htab, ctx, '\n', flag, &res, 0, 0, NULL);
if (len > 0) {
puts(res);
@@ -118,17 +119,15 @@ static int env_print(char *name, int flag)
return 0;
}
-static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc,
- char * const argv[])
+static int do_env_print_ext(cmd_tbl_t *cmdtp, int flag, int argc,
+ char * const argv[], enum env_context ctx)
{
int i;
int rcode = 0;
int env_flag = H_HIDE_DOT;
-#if defined(CONFIG_CMD_NVEDIT_EFI)
- if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
- return do_env_print_efi(cmdtp, flag, --argc, ++argv);
-#endif
+ if (ctx == ENVCTX_UEFI)
+ return do_env_print_efi(cmdtp, flag, argc, argv);
if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') {
argc--;
@@ -138,7 +137,7 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc,
if (argc == 1) {
/* print all env vars */
- rcode = env_print(NULL, env_flag);
+ rcode = env_print_ext(ctx, NULL, env_flag);
if (!rcode)
return 1;
printf("\nEnvironment size: %d/%ld bytes\n",
@@ -149,7 +148,7 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc,
/* print selected env vars */
env_flag &= ~H_HIDE_DOT;
for (i = 1; i < argc; ++i) {
- int rc = env_print(argv[i], env_flag);
+ int rc = env_print_ext(ctx, argv[i], env_flag);
if (!rc) {
printf("## Error: \"%s\" not defined\n", argv[i]);
++rcode;
@@ -159,6 +158,19 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc,
return rcode;
}
+static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc,
+ char * const argv[])
+{
+#if defined(CONFIG_CMD_NVEDIT_EFI)
+ if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
+ return do_env_print_ext(cmdtp, flag, --argc, ++argv,
+ ENVCTX_UEFI);
+ else
+#endif
+
+ return do_env_print_ext(cmdtp, flag, argc, argv, ENVCTX_UBOOT);
+}
+
#ifdef CONFIG_CMD_GREPENV
static int do_env_grep(cmd_tbl_t *cmdtp, int flag,
int argc, char * const argv[])
@@ -220,7 +232,8 @@ DONE:
* Set a new environment variable,
* or replace or delete an existing one.
*/
-static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
+static int _do_env_set(int flag, int argc, char * const argv[], int env_flag,
+ enum env_context ctx)
{
int i, len;
char *name, *value, *s;
@@ -228,10 +241,8 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
debug("Initial value for argc=%d\n", argc);
-#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
- if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
- return do_env_set_efi(NULL, flag, --argc, ++argv);
-#endif
+ if (ctx == ENVCTX_UEFI)
+ return do_env_set_efi(NULL, flag, argc, argv);
while (argc > 1 && **(argv + 1) == '-') {
char *arg = *++argv;
@@ -286,6 +297,7 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
*--s = '\0';
e.key = name;
+ e.context = ctx;
e.data = value;
hsearch_r(e, ENTER, &ep, &env_htab, env_flag);
free(value);
@@ -298,7 +310,8 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
return 0;
}
-int env_set(const char *varname, const char *varvalue)
+int env_set_ext(const enum env_context ctx,
+ const char *varname, const char *varvalue)
{
const char * const argv[4] = { "setenv", varname, varvalue, NULL };
@@ -307,9 +320,16 @@ int env_set(const char *varname, const char *varvalue)
return 1;
if (varvalue == NULL || varvalue[0] == '\0')
- return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC);
+ return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC,
+ ctx);
else
- return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC);
+ return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC,
+ ctx);
+}
+
+int env_set(const char *varname, const char *varvalue)
+{
+ return env_set_ext(ENVCTX_UBOOT, varname, varvalue);
}
/**
@@ -393,7 +413,14 @@ static int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
if (argc < 2)
return CMD_RET_USAGE;
- return _do_env_set(flag, argc, argv, H_INTERACTIVE);
+#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
+ if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
+ return _do_env_set(flag, --argc, ++argv, H_INTERACTIVE,
+ ENVCTX_UEFI);
+ else
+#endif
+
+ return _do_env_set(flag, argc, argv, H_INTERACTIVE, ENVCTX_UBOOT);
}
/*
@@ -471,7 +498,7 @@ int do_env_ask(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
}
/* Continue calling setenv code */
- return _do_env_set(flag, len, local_args, H_INTERACTIVE);
+ return _do_env_set(flag, len, local_args, H_INTERACTIVE, ENVCTX_UBOOT);
}
#endif
@@ -654,12 +681,14 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc,
if (buffer[0] == '\0') {
const char * const _argv[3] = { "setenv", argv[1], NULL };
- return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE);
+ return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE,
+ ENVCTX_UBOOT);
} else {
const char * const _argv[4] = { "setenv", argv[1], buffer,
NULL };
- return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE);
+ return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE,
+ ENVCTX_UBOOT);
}
}
#endif /* CONFIG_CMD_EDITENV */
@@ -670,7 +699,7 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc,
* return address of storage for that variable,
* or NULL if not found
*/
-char *env_get(const char *name)
+char *env_get_ext(const enum env_context ctx, const char *name)
{
if (gd->flags & GD_FLG_ENV_READY) { /* after import into hashtable */
ENTRY e, *ep;
@@ -678,6 +707,7 @@ char *env_get(const char *name)
WATCHDOG_RESET();
e.key = name;
+ e.context = ctx;
e.data = NULL;
hsearch_r(e, FIND, &ep, &env_htab, 0);
@@ -691,6 +721,11 @@ char *env_get(const char *name)
return NULL;
}
+char *env_get(const char *name)
+{
+ return env_get_ext(ENVCTX_UBOOT, name);
+}
+
/*
* Look up variable from environment for restricted C runtime env.
*/
@@ -1173,6 +1208,7 @@ static int do_env_exists(cmd_tbl_t *cmdtp, int flag, int argc,
return CMD_RET_USAGE;
e.key = argv[1];
+ e.context = ENVCTX_UBOOT;
e.data = NULL;
hsearch_r(e, FIND, &ep, &env_htab, 0);
diff --git a/include/exports.h b/include/exports.h
index a4b862f19178..0c39d9f16f3d 100644
--- a/include/exports.h
+++ b/include/exports.h
@@ -26,8 +26,11 @@ unsigned long get_timer(unsigned long);
int vprintf(const char *, va_list);
unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base);
int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
+enum env_context; /* defined in environment.h */
char *env_get(const char *name);
+char *env_get_ext(enum env_context, const char *name);
int env_set(const char *varname, const char *value);
+int env_set_ext(enum env_context, const char *varname, const char *value);
long simple_strtol(const char *cp, char **endp, unsigned int base);
int strcmp(const char *cs, const char *ct);
unsigned long ustrtoul(const char *cp, char **endp, unsigned int base);
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 03/16] env: extend interfaces to label variable with context
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 03/16] env: extend interfaces to label variable with context AKASHI Takahiro
@ 2019-07-19 8:09 ` Wolfgang Denk
2019-07-19 8:25 ` AKASHI Takahiro
0 siblings, 1 reply; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:09 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190717082525.891-4-takahiro.akashi@linaro.org> you wrote:
> The following interfaces are extended to allow for accepting an additional
> argument, env_context.
> env_get() -> env_get_ext()
> env_set() -> env_get_ext()
Please don't, see previous comments.
> Relevant env commands are synced with this change to maintain the semantics
> of existing U-Boot environment.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> cmd/nvedit.c | 82 ++++++++++++++++++++++++++++++++++-------------
> include/exports.h | 3 ++
> 2 files changed, 62 insertions(+), 23 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 49d3b5bdf466..cc80ba712767 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -87,7 +87,7 @@ int get_env_id(void)
> *
> * Returns 0 in case of error, or length of printed string
> */
> -static int env_print(char *name, int flag)
> +static int env_print_ext(enum env_context ctx, char *name, int flag)
> {
> char *res = NULL;
> ssize_t len;
> @@ -96,6 +96,7 @@ static int env_print(char *name, int flag)
> ENTRY e, *ep;
>
> e.key = name;
> + e.context = ctx;
> e.data = NULL;
> hsearch_r(e, FIND, &ep, &env_htab, flag);
> if (ep == NULL)
> -#if defined(CONFIG_CMD_NVEDIT_EFI)
> - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> - return do_env_print_efi(cmdtp, flag, --argc, ++argv);
> -#endif
> + if (ctx == ENVCTX_UEFI)
> + return do_env_print_efi(cmdtp, flag, argc, argv);
I don't like this. It doesn't scale. ENVCTX_UEFI is just one
context among others; it should not need a "if" here to handle.
> +{
> +#if defined(CONFIG_CMD_NVEDIT_EFI)
> + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
Please use proper argument handling, options can be combined, so the
'e' might not be the second character.
Also, this should probably changed to support a generic "context"
specific handling, though "-c context" or such, with U-Boot being
the default.
This again allows to get rid of all these "if"s
> -#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
> - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> - return do_env_set_efi(NULL, flag, --argc, ++argv);
> -#endif
> + if (ctx == ENVCTX_UEFI)
> + return do_env_set_efi(NULL, flag, argc, argv);
Ditto here.
> +#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
> + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> + return _do_env_set(flag, --argc, ++argv, H_INTERACTIVE,
> + ENVCTX_UEFI);
> + else
> +#endif
And here.
> const char * const _argv[3] = { "setenv", argv[1], NULL };
>
> - return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE);
> + return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE,
> + ENVCTX_UBOOT);
> } else {
> const char * const _argv[4] = { "setenv", argv[1], buffer,
> NULL };
>
> - return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE);
> + return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE,
> + ENVCTX_UBOOT);
Also here. ENVCTX_UBOOT is not a special context and should not need
special handling.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I've got to get something inside me. Some coffee or something. And
then the world will somehow be better.
- Terry Pratchett, _Men at Arms_
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 03/16] env: extend interfaces to label variable with context
2019-07-19 8:09 ` Wolfgang Denk
@ 2019-07-19 8:25 ` AKASHI Takahiro
2019-07-19 13:06 ` Wolfgang Denk
0 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-19 8:25 UTC (permalink / raw)
To: u-boot
On Fri, Jul 19, 2019 at 10:09:19AM +0200, Wolfgang Denk wrote:
> Dear Takahiro,
>
> In message <20190717082525.891-4-takahiro.akashi@linaro.org> you wrote:
> > The following interfaces are extended to allow for accepting an additional
> > argument, env_context.
> > env_get() -> env_get_ext()
> > env_set() -> env_get_ext()
>
> Please don't, see previous comments.
NAK again:)
> > Relevant env commands are synced with this change to maintain the semantics
> > of existing U-Boot environment.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> > cmd/nvedit.c | 82 ++++++++++++++++++++++++++++++++++-------------
> > include/exports.h | 3 ++
> > 2 files changed, 62 insertions(+), 23 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 49d3b5bdf466..cc80ba712767 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -87,7 +87,7 @@ int get_env_id(void)
> > *
> > * Returns 0 in case of error, or length of printed string
> > */
> > -static int env_print(char *name, int flag)
> > +static int env_print_ext(enum env_context ctx, char *name, int flag)
> > {
> > char *res = NULL;
> > ssize_t len;
> > @@ -96,6 +96,7 @@ static int env_print(char *name, int flag)
> > ENTRY e, *ep;
> >
> > e.key = name;
> > + e.context = ctx;
> > e.data = NULL;
> > hsearch_r(e, FIND, &ep, &env_htab, flag);
> > if (ep == NULL)
> > -#if defined(CONFIG_CMD_NVEDIT_EFI)
> > - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> > - return do_env_print_efi(cmdtp, flag, --argc, ++argv);
> > -#endif
> > + if (ctx == ENVCTX_UEFI)
> > + return do_env_print_efi(cmdtp, flag, argc, argv);
>
> I don't like this. It doesn't scale. ENVCTX_UEFI is just one
> context among others; it should not need a "if" here to handle.
Unfortunately, this is necessary.
As you know, we want to allow different implementations of UEFI
variables while we want to use "env" commands in the same way.
do_env_print_efi(), for example, is implemented using *UEFI interfaces*,
neither env_*() nor h*_r().
> > +{
> > +#if defined(CONFIG_CMD_NVEDIT_EFI)
> > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
>
> Please use proper argument handling, options can be combined, so the
> 'e' might not be the second character.
I think there are bunch of code like this in U-Boot. No?
> Also, this should probably changed to support a generic "context"
> specific handling, though "-c context" or such, with U-Boot being
> the default.
Yes, but please note that this option, -e, is already merged
in the upstream.
> This again allows to get rid of all these "if"s
I agree, but only when yet another context be introduced.
Thanks,
-Takahiro Akashi
> > -#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
> > - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> > - return do_env_set_efi(NULL, flag, --argc, ++argv);
> > -#endif
> > + if (ctx == ENVCTX_UEFI)
> > + return do_env_set_efi(NULL, flag, argc, argv);
>
> Ditto here.
>
> > +#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
> > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> > + return _do_env_set(flag, --argc, ++argv, H_INTERACTIVE,
> > + ENVCTX_UEFI);
> > + else
> > +#endif
>
> And here.
>
> > const char * const _argv[3] = { "setenv", argv[1], NULL };
> >
> > - return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE);
> > + return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE,
> > + ENVCTX_UBOOT);
> > } else {
> > const char * const _argv[4] = { "setenv", argv[1], buffer,
> > NULL };
> >
> > - return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE);
> > + return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE,
> > + ENVCTX_UBOOT);
>
> Also here. ENVCTX_UBOOT is not a special context and should not need
> special handling.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> I've got to get something inside me. Some coffee or something. And
> then the world will somehow be better.
> - Terry Pratchett, _Men at Arms_
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 03/16] env: extend interfaces to label variable with context
2019-07-19 8:25 ` AKASHI Takahiro
@ 2019-07-19 13:06 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 13:06 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190719082504.GS21948@linaro.org> you wrote:
>
> > > + if (ctx == ENVCTX_UEFI)
> > > + return do_env_print_efi(cmdtp, flag, argc, argv);
> >
> > I don't like this. It doesn't scale. ENVCTX_UEFI is just one
> > context among others; it should not need a "if" here to handle.
>
> Unfortunately, this is necessary.
> As you know, we want to allow different implementations of UEFI
> variables while we want to use "env" commands in the same way.
> do_env_print_efi(), for example, is implemented using *UEFI interfaces*,
> neither env_*() nor h*_r().
It is certainly not necessary to add conditional code here for each
and every potential context. It should be able to provide a generic
function, and each context can either use the default code, or
implement his own. Then all you have to do here is to call the
context's specific function pointer. No need for conditional code.
> > > +#if defined(CONFIG_CMD_NVEDIT_EFI)
> > > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> >
> > Please use proper argument handling, options can be combined, so the
> > 'e' might not be the second character.
>
> I think there are bunch of code like this in U-Boot. No?
Maybe, but this does not make it better / correct.
> > Also, this should probably changed to support a generic "context"
> > specific handling, though "-c context" or such, with U-Boot being
> > the default.
>
> Yes, but please note that this option, -e, is already merged
> in the upstream.
This can be fixed, then?
> > This again allows to get rid of all these "if"s
>
> I agree, but only when yet another context be introduced.
No, we should do it right from the beginning. It is not friendly to
implement quick and dirty code and let the next who wants to use it
do the cleanup.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A Sense of humor can help you overlook the unattractive, tolerate the
unpleasant, cope with the unexpected and smile through the
unbearable. - Moshe Waldoks
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 04/16] env: flash: add U-Boot environment context support
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (2 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 03/16] env: extend interfaces to label variable with context AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:14 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 05/16] env: fat: " AKASHI Takahiro
` (13 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
This patch shows how environment storage drivers should be modified
at the minimum to support contexts.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
env/flash.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/env/flash.c b/env/flash.c
index dca6567a097d..e1afd8358f30 100644
--- a/env/flash.c
+++ b/env/flash.c
@@ -47,13 +47,13 @@ DECLARE_GLOBAL_DATA_PTR;
#if defined(CONFIG_ENV_ADDR_REDUND) && defined(CMD_SAVEENV) || \
!defined(CONFIG_ENV_ADDR_REDUND) && defined(INITENV)
#ifdef ENV_IS_EMBEDDED
-static env_t *env_ptr = &environment;
+static env_hdr_t *env_ptr = &environment;
#else /* ! ENV_IS_EMBEDDED */
-static env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
+static env_hdr_t *env_ptr = (env_hdr_t *)CONFIG_ENV_ADDR;
#endif /* ENV_IS_EMBEDDED */
#endif
-static __maybe_unused env_t *flash_addr = (env_t *)CONFIG_ENV_ADDR;
+static __maybe_unused env_hdr_t *flash_addr = (env_hdr_t *)CONFIG_ENV_ADDR;
/* CONFIG_ENV_ADDR is supposed to be on sector boundary */
static ulong __maybe_unused end_addr =
@@ -61,7 +61,8 @@ static ulong __maybe_unused end_addr =
#ifdef CONFIG_ENV_ADDR_REDUND
-static env_t __maybe_unused *flash_addr_new = (env_t *)CONFIG_ENV_ADDR_REDUND;
+static env_hdr_t __maybe_unused *flash_addr_new =
+ (env_hdr_t *)CONFIG_ENV_ADDR_REDUND;
/* CONFIG_ENV_ADDR_REDUND is supposed to be on sector boundary */
static ulong __maybe_unused end_addr_new =
@@ -81,9 +82,10 @@ static int env_flash_init(void)
ulong addr1 = (ulong)&(flash_addr->data);
ulong addr2 = (ulong)&(flash_addr_new->data);
- crc1_ok = crc32(0, flash_addr->data, ENV_SIZE) == flash_addr->crc;
- crc2_ok =
- crc32(0, flash_addr_new->data, ENV_SIZE) == flash_addr_new->crc;
+ crc1_ok = crc32(0, flash_addr->data, flash_addr->data_size)
+ == flash_addr->crc;
+ crc2_ok = crc32(0, flash_addr_new->data, flash_addr->data_size)
+ == flash_addr_new->crc;
if (crc1_ok && !crc2_ok) {
gd->env_addr = addr1;
@@ -118,7 +120,7 @@ static int env_flash_init(void)
#ifdef CMD_SAVEENV
static int env_flash_save(void)
{
- env_t env_new;
+ env_hdr_t env_new;
char *saved_data = NULL;
char flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG;
int rc = 1;
@@ -193,7 +195,7 @@ static int env_flash_save(void)
puts("done\n");
{
- env_t *etmp = flash_addr;
+ env_hdr_t *etmp = flash_addr;
ulong ltmp = end_addr;
flash_addr = flash_addr_new;
@@ -223,7 +225,7 @@ done:
#ifdef INITENV
static int env_flash_init(void)
{
- if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
+ if (crc32(0, env_ptr->data, env_ptr->data_size) == env_ptr->crc) {
gd->env_addr = (ulong)&(env_ptr->data);
gd->env_valid = ENV_VALID;
return 0;
@@ -267,6 +269,7 @@ static int env_flash_save(void)
if (flash_sect_protect(0, (long)flash_addr, end_addr))
goto done;
+ env_new.data_size = ENV_SIZE;
rc = env_export(&env_new);
if (rc)
goto done;
@@ -311,7 +314,7 @@ static int env_flash_load(void)
{
#ifdef CONFIG_ENV_ADDR_REDUND
if (gd->env_addr != (ulong)&(flash_addr->data)) {
- env_t *etmp = flash_addr;
+ env_hdr_t *etmp = flash_addr;
ulong ltmp = end_addr;
flash_addr = flash_addr_new;
@@ -322,7 +325,8 @@ static int env_flash_load(void)
}
if (flash_addr_new->flags != OBSOLETE_FLAG &&
- crc32(0, flash_addr_new->data, ENV_SIZE) == flash_addr_new->crc) {
+ crc32(0, flash_addr_new->data, flash_addr_new->data_size)
+ == flash_addr_new->crc) {
char flag = OBSOLETE_FLAG;
gd->env_valid = ENV_REDUND;
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 04/16] env: flash: add U-Boot environment context support
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 04/16] env: flash: add U-Boot environment context support AKASHI Takahiro
@ 2019-07-19 8:14 ` Wolfgang Denk
2019-07-19 8:30 ` AKASHI Takahiro
0 siblings, 1 reply; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:14 UTC (permalink / raw)
To: u-boot
Dear AKASHI Takahiro,
In message <20190717082525.891-5-takahiro.akashi@linaro.org> you wrote:
> This patch shows how environment storage drivers should be modified
> at the minimum to support contexts.
This commit message is misleading. No part of this patch is related
to contexts. What it acually does is adding support for variable
sized environment blocks. The commit message should be fixed.
Also this commit makes me wonder if you have tested your patches for
bisectability. it seems changes that belong to together (like
making the envionment size variable) are split across patches. This
needs probably refacturing / resorting.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What can it profit a man to gain the whole world and to come to his
property with a gastric ulcer, a blown prostate, and bifocals?
-- John Steinbeck, _Cannery Row_
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 04/16] env: flash: add U-Boot environment context support
2019-07-19 8:14 ` Wolfgang Denk
@ 2019-07-19 8:30 ` AKASHI Takahiro
2019-07-19 13:11 ` Wolfgang Denk
0 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-19 8:30 UTC (permalink / raw)
To: u-boot
On Fri, Jul 19, 2019 at 10:14:06AM +0200, Wolfgang Denk wrote:
> Dear AKASHI Takahiro,
>
> In message <20190717082525.891-5-takahiro.akashi@linaro.org> you wrote:
> > This patch shows how environment storage drivers should be modified
> > at the minimum to support contexts.
>
> This commit message is misleading. No part of this patch is related
> to contexts. What it acually does is adding support for variable
> sized environment blocks. The commit message should be fixed.
Please note that this patch (or even all the patches in this set)
are not intended to be merged as they are.
This is an RFC in order for me to determine if you agree with
the approach I take here or not. This is the primary goal.
The code itself is nothing but a help for your understandings
of my basic ideas.
>
> Also this commit makes me wonder if you have tested your patches for
> bisectability.
Again, not.
Thanks,
-Takahiro Akashi
> it seems changes that belong to together (like
> making the envionment size variable) are split across patches. This
> needs probably refacturing / resorting.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> What can it profit a man to gain the whole world and to come to his
> property with a gastric ulcer, a blown prostate, and bifocals?
> -- John Steinbeck, _Cannery Row_
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 04/16] env: flash: add U-Boot environment context support
2019-07-19 8:30 ` AKASHI Takahiro
@ 2019-07-19 13:11 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 13:11 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190719083010.GT21948@linaro.org> you wrote:
> On Fri, Jul 19, 2019 at 10:14:06AM +0200, Wolfgang Denk wrote:
> > Dear AKASHI Takahiro,
> >
> > In message <20190717082525.891-5-takahiro.akashi@linaro.org> you wrote:
> > > This patch shows how environment storage drivers should be modified
> > > at the minimum to support contexts.
> >
> > This commit message is misleading. No part of this patch is related
> > to contexts. What it acually does is adding support for variable
> > sized environment blocks. The commit message should be fixed.
>
> Please note that this patch (or even all the patches in this set)
> are not intended to be merged as they are.
> This is an RFC in order for me to determine if you agree with
> the approach I take here or not. This is the primary goal.
> The code itself is nothing but a help for your understandings
> of my basic ideas.
Agreed. Please consider my comment as help for restructuring this
patrch set and improving commit messages.
> > Also this commit makes me wonder if you have tested your patches for
> > bisectability.
>
> Again, not.
I already mentioned that I think the whole patch set should be
restructured. For example, adding support for volatile vaariables
is one logical thing; adding support for auto-save another one;
extending the env (and eventually called driver) functions by a
dummy context parameter (which is initially 0 everywhere, and unused
in the code) is a single step before you actually introduce
contexts, etc.
For each step, code size changes (especially for SPL) and
bisectability shpuld be checked. But these are actually general
rules for all patches ;-)
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is a nanocentury.
- Tom Duff, Bell Labs
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 05/16] env: fat: add U-Boot environment context support
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (3 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 04/16] env: flash: add U-Boot environment context support AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:21 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 06/16] env: add variable storage attribute support AKASHI Takahiro
` (12 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
Please note that the aim of this patch is to illustrate how we can
extend the existing backing storage drivers for env interfaces to
support U-Boot environment context.
We will be able to support more devices as well as more contexts
in a similar way. Existing drivers still work exactly in the same
way as before while they are not extended yet.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
env/fat.c | 103 ++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 84 insertions(+), 19 deletions(-)
diff --git a/env/fat.c b/env/fat.c
index 7f74c64dfe7e..e4a672d2730a 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -30,23 +30,44 @@
# endif
#endif
+static struct evn_fat_context {
+ char *interface;
+ char *dev_and_part;
+ char *file;
+} fat_context[ENVCTX_COUNT] = {
+#if defined(CONFIG_ENV_FAT_INTERFACE) && \
+ defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE)
+ [ENVCTX_UBOOT] = {
+ CONFIG_ENV_FAT_INTERFACE,
+ CONFIG_ENV_FAT_DEVICE_AND_PART,
+ CONFIG_ENV_FAT_FILE,
+ },
+#endif
+};
+
#ifdef CMD_SAVEENV
-static int env_fat_save(void)
+static int env_fat_save_ext(enum env_context ctx)
{
env_t __aligned(ARCH_DMA_MINALIGN) env_new;
+ env_hdr_t *envp;
struct blk_desc *dev_desc = NULL;
disk_partition_t info;
int dev, part;
int err;
loff_t size;
- err = env_export(&env_new);
+ if (!fat_context[ctx].interface)
+ return -EIO;
+
+ env_new.data_size = ENV_SIZE;
+ envp = (env_hdr_t *)&env_new;
+ err = env_export_ext(&envp, ctx);
if (err)
return err;
- part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
- CONFIG_ENV_FAT_DEVICE_AND_PART,
- &dev_desc, &info, 1);
+ part = blk_get_device_part_str(fat_context[ctx].interface,
+ fat_context[ctx].dev_and_part,
+ &dev_desc, &info, 1);
if (part < 0)
return 1;
@@ -57,28 +78,34 @@ static int env_fat_save(void)
* will calling it. The missing \n is intentional.
*/
printf("Unable to use %s %d:%d... ",
- CONFIG_ENV_FAT_INTERFACE, dev, part);
+ fat_context[ctx].interface, dev, part);
return 1;
}
- err = file_fat_write(CONFIG_ENV_FAT_FILE, (void *)&env_new, 0, sizeof(env_t),
- &size);
+ err = file_fat_write(fat_context[ctx].file, (void *)envp, 0,
+ sizeof(env_hdr_t) + envp->data_size, &size);
if (err == -1) {
/*
* This printf is embedded in the messages from env_save that
* will calling it. The missing \n is intentional.
*/
printf("Unable to write \"%s\" from %s%d:%d... ",
- CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part);
+ fat_context[ctx].file, fat_context[ctx].interface,
+ dev, part);
return 1;
}
return 0;
}
+
+static int env_fat_save(void)
+{
+ return env_fat_save_ext(ENVCTX_UBOOT);
+}
#endif /* CMD_SAVEENV */
#ifdef LOADENV
-static int env_fat_load(void)
+static int env_fat_load_ext(enum env_context ctx)
{
ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
struct blk_desc *dev_desc = NULL;
@@ -86,14 +113,17 @@ static int env_fat_load(void)
int dev, part;
int err;
+ if (!fat_context[ctx].interface)
+ return -EIO;
+
#ifdef CONFIG_MMC
- if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc"))
+ if (!strcmp(fat_context[ctx].interface, "mmc"))
mmc_initialize(NULL);
#endif
- part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
- CONFIG_ENV_FAT_DEVICE_AND_PART,
- &dev_desc, &info, 1);
+ part = blk_get_device_part_str(fat_context[ctx].interface,
+ fat_context[ctx].dev_and_part,
+ &dev_desc, &info, 1);
if (part < 0)
goto err_env_relocate;
@@ -104,37 +134,72 @@ static int env_fat_load(void)
* will calling it. The missing \n is intentional.
*/
printf("Unable to use %s %d:%d... ",
- CONFIG_ENV_FAT_INTERFACE, dev, part);
+ fat_context[ctx].interface, dev, part);
goto err_env_relocate;
}
- err = file_fat_read(CONFIG_ENV_FAT_FILE, buf, CONFIG_ENV_SIZE);
+ err = file_fat_read(fat_context[ctx].file, buf, CONFIG_ENV_SIZE);
if (err == -1) {
/*
* This printf is embedded in the messages from env_save that
* will calling it. The missing \n is intentional.
*/
printf("Unable to read \"%s\" from %s%d:%d... ",
- CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part);
+ fat_context[ctx].file, fat_context[ctx].interface,
+ dev, part);
goto err_env_relocate;
}
- return env_import(buf, 1);
+ if (((env_hdr_t *)buf)->data_size <= 1)
+ return 0;
+
+ return env_import_ext(buf, ctx, 1);
err_env_relocate:
- set_default_env(NULL, 0);
+ if (ctx == ENVCTX_UBOOT)
+ set_default_env(NULL, 0);
return -EIO;
}
+
+static int env_fat_load(void)
+{
+ return env_fat_load_ext(ENVCTX_UBOOT);
+}
#endif /* LOADENV */
+#if defined(CMD_LOADENV) || defined(CMD_SAVEENV)
+static int env_fat_init_ext(enum env_context ctx)
+{
+ if (ctx != ENVCTX_UBOOT)
+ return 0;
+ /*
+ * Note:
+ * We can't report 0 here because low-level storage driver,
+ * like scsi, may not have been detected yet@boot time.
+ */
+ return -ENOENT;
+}
+
+static int env_fat_init(void)
+{
+ return env_fat_init_ext(ENVCTX_UBOOT);
+}
+#endif
+
U_BOOT_ENV_LOCATION(fat) = {
.location = ENVL_FAT,
ENV_NAME("FAT")
#ifdef LOADENV
.load = env_fat_load,
+ .load_ext = env_fat_load_ext,
#endif
#ifdef CMD_SAVEENV
.save = env_save_ptr(env_fat_save),
+ .save_ext = env_save_ptr(env_fat_save_ext),
+#endif
+#if defined(CMD_LOADENV) || defined(CMD_SAVEENV)
+ .init = env_fat_init,
+ .init_ext = env_fat_init_ext,
#endif
};
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 05/16] env: fat: add U-Boot environment context support
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 05/16] env: fat: " AKASHI Takahiro
@ 2019-07-19 8:21 ` Wolfgang Denk
2019-07-19 8:35 ` AKASHI Takahiro
0 siblings, 1 reply; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:21 UTC (permalink / raw)
To: u-boot
Dear AKASHI Takahiro,
In message <20190717082525.891-6-takahiro.akashi@linaro.org> you wrote:
> Please note that the aim of this patch is to illustrate how we can
> extend the existing backing storage drivers for env interfaces to
> support U-Boot environment context.
>
> We will be able to support more devices as well as more contexts
> in a similar way. Existing drivers still work exactly in the same
> way as before while they are not extended yet.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> env/fat.c | 103 ++++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 84 insertions(+), 19 deletions(-)
>
> diff --git a/env/fat.c b/env/fat.c
> index 7f74c64dfe7e..e4a672d2730a 100644
> --- a/env/fat.c
> +++ b/env/fat.c
> @@ -30,23 +30,44 @@
> # endif
> #endif
>
> +static struct evn_fat_context {
> + char *interface;
> + char *dev_and_part;
> + char *file;
> +} fat_context[ENVCTX_COUNT] = {
> +#if defined(CONFIG_ENV_FAT_INTERFACE) && \
> + defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE)
> + [ENVCTX_UBOOT] = {
> + CONFIG_ENV_FAT_INTERFACE,
> + CONFIG_ENV_FAT_DEVICE_AND_PART,
> + CONFIG_ENV_FAT_FILE,
> + },
> +#endif
> +};
Does it make sense to define this in a FAT specific way? I guess we
could come up with a common structure that covers all supported
storage devices and use this here.
Also, the "#if defined" looks really dangerous to me, as missing
#defines will go unnoticed, and in the end you are accessing
uninitialized data...
Did you review the patchset for memory leaks?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If you can't explain it to a six year old, you don't understand it
yourself. - Albert Einstein
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 05/16] env: fat: add U-Boot environment context support
2019-07-19 8:21 ` Wolfgang Denk
@ 2019-07-19 8:35 ` AKASHI Takahiro
2019-07-19 13:14 ` Wolfgang Denk
0 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-19 8:35 UTC (permalink / raw)
To: u-boot
On Fri, Jul 19, 2019 at 10:21:12AM +0200, Wolfgang Denk wrote:
> Dear AKASHI Takahiro,
>
> In message <20190717082525.891-6-takahiro.akashi@linaro.org> you wrote:
> > Please note that the aim of this patch is to illustrate how we can
> > extend the existing backing storage drivers for env interfaces to
> > support U-Boot environment context.
> >
> > We will be able to support more devices as well as more contexts
> > in a similar way. Existing drivers still work exactly in the same
> > way as before while they are not extended yet.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> > env/fat.c | 103 ++++++++++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 84 insertions(+), 19 deletions(-)
> >
> > diff --git a/env/fat.c b/env/fat.c
> > index 7f74c64dfe7e..e4a672d2730a 100644
> > --- a/env/fat.c
> > +++ b/env/fat.c
> > @@ -30,23 +30,44 @@
> > # endif
> > #endif
> >
> > +static struct evn_fat_context {
> > + char *interface;
> > + char *dev_and_part;
> > + char *file;
> > +} fat_context[ENVCTX_COUNT] = {
> > +#if defined(CONFIG_ENV_FAT_INTERFACE) && \
> > + defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE)
> > + [ENVCTX_UBOOT] = {
> > + CONFIG_ENV_FAT_INTERFACE,
> > + CONFIG_ENV_FAT_DEVICE_AND_PART,
> > + CONFIG_ENV_FAT_FILE,
> > + },
> > +#endif
> > +};
>
> Does it make sense to define this in a FAT specific way? I guess we
> could come up with a common structure that covers all supported
> storage devices and use this here.
But a different driver has different sets of configurations required.
How can we *generalize* them?
> Also, the "#if defined" looks really dangerous to me, as missing
> #defines will go unnoticed, and in the end you are accessing
> uninitialized data...
Yes, I have made this mistake before.
But I think that all the drivers must be verified in any case
before any changes are applied to the upstream. No?
> Did you review the patchset for memory leaks?
No.
-Takahiro Akashi
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> If you can't explain it to a six year old, you don't understand it
> yourself. - Albert Einstein
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 05/16] env: fat: add U-Boot environment context support
2019-07-19 8:35 ` AKASHI Takahiro
@ 2019-07-19 13:14 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 13:14 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190719083548.GU21948@linaro.org> you wrote:
>
> > > +#if defined(CONFIG_ENV_FAT_INTERFACE) && \
> > > + defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE)
> > > + [ENVCTX_UBOOT] = {
> > > + CONFIG_ENV_FAT_INTERFACE,
> > > + CONFIG_ENV_FAT_DEVICE_AND_PART,
> > > + CONFIG_ENV_FAT_FILE,
> > > + },
> > > +#endif
> > > +};
> >
> > Does it make sense to define this in a FAT specific way? I guess we
> > could come up with a common structure that covers all supported
> > storage devices and use this here.
>
> But a different driver has different sets of configurations required.
> How can we *generalize* them?
Does it? At least all block devices probably share the properties
of interface, device, partition and file name in one way or another.
Or offset instead of file name when accessing the raw device.
I think it should be possible to at least support all file systems
with exactly the sme code - ther eis nothing special about FAT here,
or am I missing something?
> > Also, the "#if defined" looks really dangerous to me, as missing
> > #defines will go unnoticed, and in the end you are accessing
> > uninitialized data...
>
> Yes, I have made this mistake before.
> But I think that all the drivers must be verified in any case
> before any changes are applied to the upstream. No?
Indeed, testing will be needed.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If it happens once, it's a bug.
If it happens twice, it's a feature.
If it happens more than twice, it's a design philosophy.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 06/16] env: add variable storage attribute support
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (4 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 05/16] env: fat: " AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:35 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 07/16] env: add/expose attribute helper functions for hashtable AKASHI Takahiro
` (11 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
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(-)
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__ */
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 06/16] env: add variable storage attribute support
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 06/16] env: add variable storage attribute support AKASHI Takahiro
@ 2019-07-19 8:35 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:35 UTC (permalink / raw)
To: u-boot
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
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
No journaling file system can recover your data if the disk dies.
- Steve Rago in <D4Cw1p.L9E@plc.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 07/16] env: add/expose attribute helper functions for hashtable
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (5 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 06/16] env: add variable storage attribute support AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:36 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 08/16] hashtable: import/export entries with flags AKASHI Takahiro
` (10 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
Those functions will be used for hashtable routines to support attributes
in importing/exporting entries.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
env/flags.c | 19 +++++++++++++++++--
include/env_flags.h | 10 ++++++++++
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/env/flags.c b/env/flags.c
index c1db891a2343..1a1872aa2fc7 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -238,6 +238,21 @@ enum env_flags_varstorage env_flags_parse_varstorage_from_binflags(int binflags)
return env_flags_varstorage_non_volatile;
}
+void env_flags_encode_attr_from_binflags(char *attr, int binflags)
+{
+ enum env_flags_vartype type;
+ enum env_flags_varaccess access;
+ enum env_flags_varstorage storage;
+
+ type = (enum env_flags_vartype)(binflags & ENV_FLAGS_VARTYPE_BIN_MASK);
+ access = env_flags_parse_varaccess_from_binflags(binflags);
+ storage = env_flags_parse_varstorage_from_binflags(binflags);
+
+ attr[ENV_FLAGS_VARTYPE_LOC] = env_flags_vartype_rep[type];
+ attr[ENV_FLAGS_VARACCESS_LOC] = env_flags_varaccess_rep[access];
+ attr[ENV_FLAGS_VARSTORAGE_LOC] = env_flags_varstorage_rep[storage];
+}
+
static inline int is_hex_prefix(const char *value)
{
return value[0] == '0' && (value[1] == 'x' || value[1] == 'X');
@@ -488,10 +503,10 @@ int env_flags_validate_env_set_params(char *name, char * const val[], int count)
#else /* !USE_HOSTCC - Functions only used from lib/hashtable.c */
/*
- * Parse the flag charachters from the .flags attribute list into the binary
+ * Parse the flag characters from the .flags attribute list into the binary
* form to be stored in the environment entry->flags field.
*/
-static int env_parse_flags_to_bin(const char *flags)
+int env_parse_flags_to_bin(const char *flags)
{
int binflags = 0;
diff --git a/include/env_flags.h b/include/env_flags.h
index f46528691889..15391a28de77 100644
--- a/include/env_flags.h
+++ b/include/env_flags.h
@@ -132,6 +132,10 @@ enum env_flags_varaccess env_flags_parse_varaccess_from_binflags(int binflags);
*/
enum env_flags_varstorage
env_flags_parse_varstorage_from_binflags(int binflags);
+/*
+ * Create a string representation from the binary flags
+ */
+void env_flags_encode_attr_from_binflags(char *attr, int binflags);
#ifdef CONFIG_CMD_NET
/*
@@ -175,6 +179,12 @@ int env_flags_validate_env_set_params(char *name, char *const val[], int count);
#else /* !USE_HOSTCC */
+/*
+ * Parse the flag characters from the .flags attribute list into the binary
+ * form to be stored in the environment entry->flags field.
+ */
+int env_parse_flags_to_bin(const char *flags);
+
#include <search.h>
/*
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 07/16] env: add/expose attribute helper functions for hashtable
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 07/16] env: add/expose attribute helper functions for hashtable AKASHI Takahiro
@ 2019-07-19 8:36 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:36 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190717082525.891-8-takahiro.akashi@linaro.org> you wrote:
> Those functions will be used for hashtable routines to support attributes
> in importing/exporting entries.
Guess this all goes away after fixing the flag handling?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"You know, after a woman's raised a family and so on, she wants to
start living her own life." "Whose life she's _been_ living, then?"
- Terry Pratchett, _Witches Abroad_
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 08/16] hashtable: import/export entries with flags
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (6 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 07/16] env: add/expose attribute helper functions for hashtable AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:38 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 09/16] hashtable: extend hdelete_ext for autosave AKASHI Takahiro
` (9 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
'flags' value of all the entries in hashtable should be preserved across
save/load of U-Boot environment context.
To hold such information in an exported file, its text format is now
expanded as follows:
name:attr=value<sp>
...
\0
where "attr" must be a fixed-length(ENV_FLAGS_ATTR_MAX_LEN) string which
complies with a existing format of ".flags" variable and used by
env_attr_lookup().
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
include/search.h | 1 +
lib/hashtable.c | 61 ++++++++++++++++++++++++++++++++++++++++++++----
2 files changed, 57 insertions(+), 5 deletions(-)
diff --git a/include/search.h b/include/search.h
index 9cece695d726..ab0d7ccb5f8c 100644
--- a/include/search.h
+++ b/include/search.h
@@ -133,6 +133,7 @@ extern int hwalk_r(struct hsearch_data *__htab, int (*callback)(ENTRY *));
#define H_MATCH_REGEX (1 << 8) /* search for regular expression matches */
#define H_MATCH_METHOD (H_MATCH_IDENT | H_MATCH_SUBSTR | H_MATCH_REGEX)
#define H_PROGRAMMATIC (1 << 9) /* indicate that an import is from env_set() */
+#define H_MATCH_FLAGS (1 << 10) /* search/grep key = variable flags */
#define H_ORIGIN_FLAGS (H_INTERACTIVE | H_PROGRAMMATIC)
#endif /* _SEARCH_H_ */
diff --git a/lib/hashtable.c b/lib/hashtable.c
index d6975d9cafc6..3fe1d38c827e 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -248,6 +248,15 @@ static inline int _compare_and_overwrite_entry(ENTRY item, ACTION action,
/* Overwrite existing value? */
if ((action == ENTER) && (item.data != NULL)) {
+ /* attributes cannot be changed */
+ if (item.flags != htab->table[idx].entry.flags) {
+ debug("attributes cannot be changed %s\n",
+ item.key);
+ __set_errno(EINVAL);
+ *retval = NULL;
+ return 0;
+ }
+
/* check for permission */
if (htab->change_ok != NULL && htab->change_ok(
&htab->table[idx].entry, item.data,
@@ -389,6 +398,8 @@ int hsearch_ext(ENTRY item, ACTION action, ENTRY ** retval,
htab->table[idx].used = hval;
htab->table[idx].entry.context = item.context;
htab->table[idx].entry.key = strdup(item.key);
+ /* TODO: can be overwritten by env_flags_init() */
+ htab->table[idx].entry.flags = item.flags;
htab->table[idx].entry.data = strdup(item.data);
if (!htab->table[idx].entry.key ||
!htab->table[idx].entry.data) {
@@ -402,7 +413,9 @@ int hsearch_ext(ENTRY item, ACTION action, ENTRY ** retval,
/* This is a new entry, so look up a possible callback */
env_callback_init(&htab->table[idx].entry);
/* Also look for flags */
- env_flags_init(&htab->table[idx].entry);
+ /* TODO: or we may want to add another argument? */
+ if (!item.flags)
+ env_flags_init(&htab->table[idx].entry);
/* check for permission */
if (htab->change_ok != NULL && htab->change_ok(
@@ -596,6 +609,21 @@ static int match_string(int flag, const char *str, const char *pat, void *priv)
return 0;
}
+/* TODO: we always need some kind of regex matching */
+static int match_attr(int flag, const char *attr, const char *pat, void *priv)
+{
+ int found, i;
+
+ for (found = 1, i = 0; i < ENV_FLAGS_ATTR_MAX_LEN; i++) {
+ if (pat[i] != '.' && attr[i] != pat[i]) {
+ found = 0;
+ break;
+ }
+ }
+
+ return found;
+}
+
static int match_entry(ENTRY *ep, int flag,
int argc, char * const argv[])
{
@@ -617,6 +645,14 @@ static int match_entry(ENTRY *ep, int flag,
if (match_string(flag, ep->key, argv[arg], priv))
return 1;
}
+ if (flag & H_MATCH_FLAGS) {
+ char attr[ENV_FLAGS_ATTR_MAX_LEN + 1];
+
+ env_flags_encode_attr_from_binflags(attr, ep->flags);
+ attr[ENV_FLAGS_ATTR_MAX_LEN] = '\0';
+ if (match_attr(flag, attr, argv[arg], priv))
+ return 1;
+ }
if (flag & H_MATCH_DATA) {
if (match_string(flag, ep->data, argv[arg], priv))
return 1;
@@ -680,6 +716,7 @@ ssize_t hexport_ext(struct hsearch_data *htab, unsigned int ctx,
++s;
}
}
+ totlen += ENV_FLAGS_ATTR_MAX_LEN + 1; /* for :xxx */
totlen += 2; /* for '=' and 'sep' char */
}
}
@@ -731,6 +768,9 @@ ssize_t hexport_ext(struct hsearch_data *htab, unsigned int ctx,
s = list[i]->key;
while (*s)
*p++ = *s++;
+ *p++ = ':';
+ env_flags_encode_attr_from_binflags(p, list[i]->flags);
+ p += ENV_FLAGS_ATTR_MAX_LEN;
*p++ = '=';
s = list[i]->data;
@@ -829,9 +869,9 @@ int himport_ext(struct hsearch_data *htab, unsigned int ctx,
const char *env, size_t size, const char sep, int flag,
int crlf_is_lf, int nvars, char * const vars[])
{
- char *data, *sp, *dp, *name, *value;
+ char *data, *sp, *dp, *name, *value, *attr;
char *localvars[nvars];
- int i;
+ int flags, i;
/* Test for correct arguments. */
if (htab == NULL) {
@@ -927,8 +967,10 @@ int himport_ext(struct hsearch_data *htab, unsigned int ctx,
}
/* parse name */
- for (name = dp; *dp != '=' && *dp && *dp != sep; ++dp)
- ;
+ for (name = dp, attr = NULL; *dp != '=' && *dp && *dp != sep;
+ ++dp)
+ if (*dp == ':')
+ attr = dp;
/* deal with "name" and "name=" entries (delete var) */
if (*dp == '\0' || *(dp + 1) == '\0' ||
@@ -936,6 +978,8 @@ int himport_ext(struct hsearch_data *htab, unsigned int ctx,
if (*dp == '=')
*dp++ = '\0';
*dp++ = '\0'; /* terminate name */
+ if (attr)
+ *attr = '\0';
debug("DELETE CANDIDATE: \"%s\"\n", name);
if (!drop_var_from_set(name, nvars, localvars))
@@ -947,6 +991,12 @@ int himport_ext(struct hsearch_data *htab, unsigned int ctx,
continue;
}
*dp++ = '\0'; /* terminate name */
+ if (attr) {
+ *attr = '\0';
+ flags = env_parse_flags_to_bin(++attr);
+ } else {
+ flags = 0; /* TODO: default? */
+ }
/* parse value; deal with escapes */
for (value = sp = dp; *dp && (*dp != sep); ++dp) {
@@ -971,6 +1021,7 @@ int himport_ext(struct hsearch_data *htab, unsigned int ctx,
/* enter into hash table */
e.context = ctx;
e.key = name;
+ e.flags = flags;
e.data = value;
hsearch_ext(e, ENTER, &rv, htab, flag);
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 08/16] hashtable: import/export entries with flags
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 08/16] hashtable: import/export entries with flags AKASHI Takahiro
@ 2019-07-19 8:38 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:38 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190717082525.891-9-takahiro.akashi@linaro.org> you wrote:
> 'flags' value of all the entries in hashtable should be preserved across
> save/load of U-Boot environment context.
> To hold such information in an exported file, its text format is now
> expanded as follows:
> name:attr=value<sp>
> ...
> \0
>
> where "attr" must be a fixed-length(ENV_FLAGS_ATTR_MAX_LEN) string which
> complies with a existing format of ".flags" variable and used by
> env_attr_lookup().
Full NAK here. This breaks compatibility with exiting code.
The colon is a legal character in variable names, so you cannot use
it to introduce new meanings.
Please extend existing flag handling in a compatible way instead.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Anything that is worth doing at all is worth doing well.
-- Philip Earl of Chesterfield
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 09/16] hashtable: extend hdelete_ext for autosave
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (7 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 08/16] hashtable: import/export entries with flags AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:41 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 10/16] env: save non-volatile variables only AKASHI Takahiro
` (8 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
With this patch, an extended interface of hdelete_ext() will return
variable's attribute. The feature will be used in a successive patch
to determine if the context should be 'autosaved' or not even in case
of deletion.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
include/search.h | 2 +-
lib/hashtable.c | 8 +++++---
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/search.h b/include/search.h
index ab0d7ccb5f8c..cf145fa79f8b 100644
--- a/include/search.h
+++ b/include/search.h
@@ -94,7 +94,7 @@ extern int hmatch_ext(const char *__match, int __last_idx, ENTRY ** __retval,
extern int hdelete_r(const char *__key, struct hsearch_data *__htab,
int __flag);
extern int hdelete_ext(const char *__key, struct hsearch_data *__htab,
- unsigned int ctx, int __flag);
+ unsigned int ctx, uint32_t *flags, int __flag);
extern ssize_t hexport_r(struct hsearch_data *__htab,
const char __sep, int __flag, char **__resp, size_t __size,
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 3fe1d38c827e..4a2541ecf59d 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -485,7 +485,7 @@ static void _hdelete(const char *key, struct hsearch_data *htab, ENTRY *ep,
}
int hdelete_ext(const char *key, struct hsearch_data *htab, unsigned int ctx,
- int flag)
+ uint32_t *flags, int flag)
{
ENTRY e, *ep;
int idx;
@@ -519,6 +519,8 @@ int hdelete_ext(const char *key, struct hsearch_data *htab, unsigned int ctx,
return 0;
}
+ if (flags)
+ *flags = ep->flags;
_hdelete(key, htab, ep, idx);
return 1;
@@ -526,7 +528,7 @@ int hdelete_ext(const char *key, struct hsearch_data *htab, unsigned int ctx,
int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
{
- return hdelete_ext(key, htab, 0, flag);
+ return hdelete_ext(key, htab, 0, NULL, flag);
}
#if !(defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_SAVEENV))
@@ -985,7 +987,7 @@ int himport_ext(struct hsearch_data *htab, unsigned int ctx,
if (!drop_var_from_set(name, nvars, localvars))
continue;
- if (hdelete_ext(name, htab, ctx, flag) == 0)
+ if (hdelete_ext(name, htab, ctx, NULL, flag) == 0)
debug("DELETE ERROR ##############################\n");
continue;
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 09/16] hashtable: extend hdelete_ext for autosave
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 09/16] hashtable: extend hdelete_ext for autosave AKASHI Takahiro
@ 2019-07-19 8:41 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:41 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190717082525.891-10-takahiro.akashi@linaro.org> you wrote:
> With this patch, an extended interface of hdelete_ext() will return
> variable's attribute. The feature will be used in a successive patch
> to determine if the context should be 'autosaved' or not even in case
> of deletion.
Why would this be added to the hdelete() code?
hdelete() is used to DELETE entries from the hash table. Something
that has been deleted does not have any attributes any more.
This looks all wrong to me.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Brain fried - Core dumped
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 10/16] env: save non-volatile variables only
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (8 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 09/16] hashtable: extend hdelete_ext for autosave AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:45 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 11/16] env: save a context immediately if 'autosave' variable is changed AKASHI Takahiro
` (7 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
Once variable storage attribute is introduced, only NON_VOLATILE or
NON_VOLATILE_AUTOSAVE variables should be saved to backing storage.
In addition, NON_VOLATILE_AUTOSAVE variables are saved immediately
at env_set[_ext]().
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
env/common.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/env/common.c b/env/common.c
index c2a2d9f22feb..832a54e9faa2 100644
--- a/env/common.c
+++ b/env/common.c
@@ -230,6 +230,7 @@ int env_export_ext(env_hdr_t **env_out, enum env_context ctx)
size_t size;
ssize_t len;
env_hdr_t *envp;
+ char * const match[] = {"..n", "..a",};
if (*env_out) {
data = (*env_out)->data;
@@ -238,8 +239,9 @@ int env_export_ext(env_hdr_t **env_out, enum env_context ctx)
data = NULL;
size = 0;
}
- len = hexport_ext(&env_htab, ctx, '\0', 0, (char **)&data, size,
- 0, NULL);
+ /* TODO: H_MATCH_REGEX */
+ len = hexport_ext(&env_htab, ctx, '\0', H_MATCH_FLAGS | H_MATCH_IDENT,
+ (char **)&data, size, 2, match);
if (len < 0) {
pr_err("Cannot export environment: errno = %d\n",
errno);
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 10/16] env: save non-volatile variables only
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 10/16] env: save non-volatile variables only AKASHI Takahiro
@ 2019-07-19 8:45 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:45 UTC (permalink / raw)
To: u-boot
Dear AKASHI Takahiro,
In message <20190717082525.891-11-takahiro.akashi@linaro.org> you wrote:
> Once variable storage attribute is introduced, only NON_VOLATILE or
> NON_VOLATILE_AUTOSAVE variables should be saved to backing storage.
This logis is wrong.
It should read:
Variables which have the "volatile" flag set will be skipped
when savong the environment.
> In addition, NON_VOLATILE_AUTOSAVE variables are saved immediately
> at env_set[_ext]().
This is completely independent, and should eventually even be a
separate commit:
Variables which have the 'autosave" flag set are saved with
each (successful) env_set().
Yes, I made up my mind:
Adding "volatile" and "autosave" is two independent features, this
should be relfected in two separate commits / patch series.
Probably even independent from and preceeding the context series.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Es ist nicht genug zu wissen, man muß auch anwenden; es ist nicht ge-
nug zu wollen, man muß auch tun. -- Goethe, Maximen und Reflexionen
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 11/16] env: save a context immediately if 'autosave' variable is changed
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (9 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 10/16] env: save non-volatile variables only AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:48 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 12/16] env: extend interfaces to get/set attributes AKASHI Takahiro
` (6 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
By definition, when any variable with VARSTORAGE_NON_VOLATILE_AUTOSAVE
is modified with env_save_ext(), the associated context should be written
back to storage immediately.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
cmd/nvedit.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index cc80ba712767..9896a4141a2a 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -271,8 +271,27 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag,
/* Delete only ? */
if (argc < 3 || argv[2] == NULL) {
- int rc = hdelete_r(name, &env_htab, env_flag);
- return !rc;
+ uint32_t flags;
+
+ int rc = hdelete_ext(name, &env_htab, ctx, &flags, env_flag);
+ if (!rc) {
+ debug("Failed to delete from hash table\n");
+ return 1;
+ }
+
+ if (env_flags_parse_varstorage_from_binflags(flags)
+ == env_flags_varstorage_non_volatile_autosave) {
+ int ret;
+
+ ret = env_save_ext(ctx);
+ if (ret) {
+ printf("## Error saving variables, ret = %d\n",
+ ret);
+ return 1;
+ }
+ }
+
+ return 0;
}
/*
@@ -307,6 +326,17 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag,
return 1;
}
+ if (env_flags_parse_varstorage_from_binflags(ep->flags)
+ == env_flags_varstorage_non_volatile_autosave) {
+ int ret;
+
+ ret = env_save_ext(ep->context);
+ if (ret) {
+ printf("## Error saving variables, ret = %d\n", ret);
+ return 1;
+ }
+ }
+
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 11/16] env: save a context immediately if 'autosave' variable is changed
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 11/16] env: save a context immediately if 'autosave' variable is changed AKASHI Takahiro
@ 2019-07-19 8:48 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:48 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190717082525.891-12-takahiro.akashi@linaro.org> you wrote:
> By definition, when any variable with VARSTORAGE_NON_VOLATILE_AUTOSAVE
> is modified with env_save_ext(), the associated context should be written
> back to storage immediately.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> cmd/nvedit.c | 34 ++++++++++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index cc80ba712767..9896a4141a2a 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -271,8 +271,27 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag,
>
> /* Delete only ? */
> if (argc < 3 || argv[2] == NULL) {
> - int rc = hdelete_r(name, &env_htab, env_flag);
> - return !rc;
> + uint32_t flags;
> +
> + int rc = hdelete_ext(name, &env_htab, ctx, &flags, env_flag);
> + if (!rc) {
> + debug("Failed to delete from hash table\n");
> + return 1;
> + }
> +
> + if (env_flags_parse_varstorage_from_binflags(flags)
> + == env_flags_varstorage_non_volatile_autosave) {
> + int ret;
> +
> + ret = env_save_ext(ctx);
> + if (ret) {
> + printf("## Error saving variables, ret = %d\n",
> + ret);
> + return 1;
> + }
> + }
> +
> + return 0;
I think your logic of using hdelete() here to provide flag
information is not correct. The existing code stores flag
information independent of the actual existence of the variable.
Your code breaks this.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Quantum Mechanics is God's version of "Trust me."
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 12/16] env: extend interfaces to get/set attributes
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (10 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 11/16] env: save a context immediately if 'autosave' variable is changed AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 8:50 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 13/16] cmd: env: show variable storage attribute in "env flags" command AKASHI Takahiro
` (5 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
With this patch, the following interfaces are extended to accept
an additional argument, flags, to get/set attributes of variables.
This feature will be used to implement UEFI variables' semantics,
in particular, that any value change must be reflected to backing
storage immediately.
Meanwhile, existing "env print," "env set" commands are not
extended for now as there is no strong need for that.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
cmd/nvedit.c | 59 ++++++++++++++++++++++++++++++-----------------
include/exports.h | 5 ++--
2 files changed, 41 insertions(+), 23 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 9896a4141a2a..70fbb5dd8f2f 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -89,7 +89,7 @@ int get_env_id(void)
*/
static int env_print_ext(enum env_context ctx, char *name, int flag)
{
- char *res = NULL;
+ char *res = NULL, attr[ENV_FLAGS_ATTR_MAX_LEN + 1];
ssize_t len;
if (name) { /* print a single name */
@@ -98,10 +98,12 @@ static int env_print_ext(enum env_context ctx, char *name, int flag)
e.key = name;
e.context = ctx;
e.data = NULL;
- hsearch_r(e, FIND, &ep, &env_htab, flag);
+ hsearch_ext(e, FIND, &ep, &env_htab, flag);
if (ep == NULL)
return 0;
- len = printf("%s=%s\n", ep->key, ep->data);
+ env_flags_encode_attr_from_binflags(attr, ep->flags);
+ attr[ENV_FLAGS_ATTR_MAX_LEN] = '\0';
+ len = printf("%s:%s=%s\n", ep->key, attr, ep->data);
return len;
}
@@ -233,7 +235,7 @@ DONE:
* or replace or delete an existing one.
*/
static int _do_env_set(int flag, int argc, char * const argv[], int env_flag,
- enum env_context ctx)
+ enum env_context ctx, int flags)
{
int i, len;
char *name, *value, *s;
@@ -241,7 +243,7 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag,
debug("Initial value for argc=%d\n", argc);
- if (ctx == ENVCTX_UEFI)
+ if (ctx == ENVCTX_UEFI && (env_flag & H_INTERACTIVE))
return do_env_set_efi(NULL, flag, argc, argv);
while (argc > 1 && **(argv + 1) == '-') {
@@ -317,8 +319,9 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag,
e.key = name;
e.context = ctx;
+ e.flags = flags;
e.data = value;
- hsearch_r(e, ENTER, &ep, &env_htab, env_flag);
+ hsearch_ext(e, ENTER, &ep, &env_htab, env_flag);
free(value);
if (!ep) {
printf("## Error inserting \"%s\" variable, errno=%d\n",
@@ -341,7 +344,7 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag,
}
int env_set_ext(const enum env_context ctx,
- const char *varname, const char *varvalue)
+ const char *varname, int flags, const char *varvalue)
{
const char * const argv[4] = { "setenv", varname, varvalue, NULL };
@@ -351,15 +354,15 @@ int env_set_ext(const enum env_context ctx,
if (varvalue == NULL || varvalue[0] == '\0')
return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC,
- ctx);
+ ctx, flags);
else
return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC,
- ctx);
+ ctx, flags);
}
int env_set(const char *varname, const char *varvalue)
{
- return env_set_ext(ENVCTX_UBOOT, varname, varvalue);
+ return env_set_ext(ENVCTX_UBOOT, varname, 0, varvalue);
}
/**
@@ -446,11 +449,11 @@ static int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
return _do_env_set(flag, --argc, ++argv, H_INTERACTIVE,
- ENVCTX_UEFI);
+ ENVCTX_UEFI, 0);
else
#endif
- return _do_env_set(flag, argc, argv, H_INTERACTIVE, ENVCTX_UBOOT);
+ return _do_env_set(flag, argc, argv, H_INTERACTIVE, ENVCTX_UBOOT, 0);
}
/*
@@ -528,7 +531,8 @@ int do_env_ask(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
}
/* Continue calling setenv code */
- return _do_env_set(flag, len, local_args, H_INTERACTIVE, ENVCTX_UBOOT);
+ return _do_env_set(flag, len, local_args, H_INTERACTIVE, ENVCTX_UBOOT,
+ 0);
}
#endif
@@ -712,13 +716,13 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc,
const char * const _argv[3] = { "setenv", argv[1], NULL };
return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE,
- ENVCTX_UBOOT);
+ ENVCTX_UBOOT, 0);
} else {
const char * const _argv[4] = { "setenv", argv[1], buffer,
NULL };
return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE,
- ENVCTX_UBOOT);
+ ENVCTX_UBOOT, 0);
}
}
#endif /* CONFIG_CMD_EDITENV */
@@ -729,7 +733,7 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc,
* return address of storage for that variable,
* or NULL if not found
*/
-char *env_get_ext(const enum env_context ctx, const char *name)
+char *env_get_ext(const enum env_context ctx, const char *name, int *flags)
{
if (gd->flags & GD_FLG_ENV_READY) { /* after import into hashtable */
ENTRY e, *ep;
@@ -739,21 +743,34 @@ char *env_get_ext(const enum env_context ctx, const char *name)
e.key = name;
e.context = ctx;
e.data = NULL;
- hsearch_r(e, FIND, &ep, &env_htab, 0);
+ hsearch_ext(e, FIND, &ep, &env_htab, 0);
+
+ if (ep) {
+ if (flags)
+ *flags = ep->flags;
+
+ return ep->data;
+ }
- return ep ? ep->data : NULL;
+ return NULL;
}
/* restricted capabilities before import */
- if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
+ if (env_get_f(name, (char *)gd->env_buf, sizeof(gd->env_buf)) > 0) {
+ if (flags)
+ *flags = 0;
+
return (char *)(gd->env_buf);
+ }
return NULL;
}
char *env_get(const char *name)
{
- return env_get_ext(ENVCTX_UBOOT, name);
+ int flags;
+
+ return env_get_ext(ENVCTX_UBOOT, name, &flags);
}
/*
@@ -1240,7 +1257,7 @@ static int do_env_exists(cmd_tbl_t *cmdtp, int flag, int argc,
e.key = argv[1];
e.context = ENVCTX_UBOOT;
e.data = NULL;
- hsearch_r(e, FIND, &ep, &env_htab, 0);
+ hsearch_ext(e, FIND, &ep, &env_htab, 0);
return (ep == NULL) ? 1 : 0;
}
diff --git a/include/exports.h b/include/exports.h
index 0c39d9f16f3d..8eb31cb33b89 100644
--- a/include/exports.h
+++ b/include/exports.h
@@ -28,9 +28,10 @@ unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base);
int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
enum env_context; /* defined in environment.h */
char *env_get(const char *name);
-char *env_get_ext(enum env_context, const char *name);
+char *env_get_ext(enum env_context, const char *name, int *flags);
int env_set(const char *varname, const char *value);
-int env_set_ext(enum env_context, const char *varname, const char *value);
+int env_set_ext(enum env_context, const char *varname, int flags,
+ const char *value);
long simple_strtol(const char *cp, char **endp, unsigned int base);
int strcmp(const char *cs, const char *ct);
unsigned long ustrtoul(const char *cp, char **endp, unsigned int base);
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 12/16] env: extend interfaces to get/set attributes
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 12/16] env: extend interfaces to get/set attributes AKASHI Takahiro
@ 2019-07-19 8:50 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 8:50 UTC (permalink / raw)
To: u-boot
Dear AKASHI Takahiro,
In message <20190717082525.891-13-takahiro.akashi@linaro.org> you wrote:
> With this patch, the following interfaces are extended to accept
> an additional argument, flags, to get/set attributes of variables.
> This feature will be used to implement UEFI variables' semantics,
> in particular, that any value change must be reflected to backing
> storage immediately.
NAK, see previous comments.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The use of COBOL cripples the mind; its teaching should, therefore,
be regarded as a criminal offence.
-- Edsger W. Dijkstra, SIGPLAN Notices, Volume 17, Number 5
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 13/16] cmd: env: show variable storage attribute in "env flags" command
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (11 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 12/16] env: extend interfaces to get/set attributes AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 9:05 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC,PATCH v4 14/16] env: fat: support UEFI context AKASHI Takahiro
` (4 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
With this patch, "env flags" commands shows information about
storage attribute as well as other attributes.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
cmd/nvedit.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 70fbb5dd8f2f..77e8846b601a 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -618,10 +618,12 @@ static int print_static_flags(const char *var_name, const char *flags,
{
enum env_flags_vartype type = env_flags_parse_vartype(flags);
enum env_flags_varaccess access = env_flags_parse_varaccess(flags);
+ enum env_flags_varstorage storage = env_flags_parse_varstorage(flags);
- printf("\t%-20s %-20s %-20s\n", var_name,
+ printf("\t%-20s %-20s %-20s %-20s\n", var_name,
env_flags_get_vartype_name(type),
- env_flags_get_varaccess_name(access));
+ env_flags_get_varaccess_name(access),
+ env_flags_get_varstorage_name(storage));
return 0;
}
@@ -630,16 +632,25 @@ static int print_active_flags(ENTRY *entry)
{
enum env_flags_vartype type;
enum env_flags_varaccess access;
+ enum env_flags_varstorage storage;
- if (entry->flags == 0)
+#ifdef CONFIG_EFI_LOADER
+ if (entry->context == ENVCTX_UEFI &&
+ entry->flags == ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE)
+ return 0;
+#endif
+ /* ENVCTX_UBOOT */
+ if (entry->flags == ENV_FLAGS_VARSTORAGE_NON_VOLATILE)
return 0;
type = (enum env_flags_vartype)
(entry->flags & ENV_FLAGS_VARTYPE_BIN_MASK);
access = env_flags_parse_varaccess_from_binflags(entry->flags);
- printf("\t%-20s %-20s %-20s\n", entry->key,
+ storage = env_flags_parse_varstorage_from_binflags(entry->flags);
+ printf("\t%-20s %-20s %-20s %-20s\n", entry->key,
env_flags_get_vartype_name(type),
- env_flags_get_varaccess_name(access));
+ env_flags_get_varaccess_name(access),
+ env_flags_get_varstorage_name(storage));
return 0;
}
@@ -667,19 +678,19 @@ int do_env_flags(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
/* Print the static flags that may exist */
puts("Static flags:\n");
- printf("\t%-20s %-20s %-20s\n", "Variable Name", "Variable Type",
- "Variable Access");
- printf("\t%-20s %-20s %-20s\n", "-------------", "-------------",
- "---------------");
+ printf("\t%-20s %-20s %-20s %-20s\n", "Variable Name", "Variable Type",
+ "Variable Access", "Variable Storage");
+ printf("\t%-20s %-20s %-20s %-20s\n", "-------------", "-------------",
+ "---------------", "-------------");
env_attr_walk(ENV_FLAGS_LIST_STATIC, print_static_flags, NULL);
puts("\n");
/* walk through each variable and print the flags if non-default */
puts("Active flags:\n");
- printf("\t%-20s %-20s %-20s\n", "Variable Name", "Variable Type",
- "Variable Access");
- printf("\t%-20s %-20s %-20s\n", "-------------", "-------------",
- "---------------");
+ printf("\t%-20s %-20s %-20s %-20s\n", "Variable Name", "Variable Type",
+ "Variable Access", "Variable Storage");
+ printf("\t%-20s %-20s %-20s %-20s\n", "-------------", "-------------",
+ "---------------", "-------------");
hwalk_r(&env_htab, print_active_flags);
return 0;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 13/16] cmd: env: show variable storage attribute in "env flags" command
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 13/16] cmd: env: show variable storage attribute in "env flags" command AKASHI Takahiro
@ 2019-07-19 9:05 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 9:05 UTC (permalink / raw)
To: u-boot
Dear AKASHI Takahiro,
In message <20190717082525.891-14-takahiro.akashi@linaro.org> you wrote:
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 70fbb5dd8f2f..77e8846b601a 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -618,10 +618,12 @@ static int print_static_flags(const char *var_name, const char *flags,
> {
> enum env_flags_vartype type = env_flags_parse_vartype(flags);
> enum env_flags_varaccess access = env_flags_parse_varaccess(flags);
> + enum env_flags_varstorage storage = env_flags_parse_varstorage(flags);
>
> - printf("\t%-20s %-20s %-20s\n", var_name,
> + printf("\t%-20s %-20s %-20s %-20s\n", var_name,
> env_flags_get_vartype_name(type),
> - env_flags_get_varaccess_name(access));
> + env_flags_get_varaccess_name(access),
> + env_flags_get_varstorage_name(storage));
This needs changing, see before.
>
> - if (entry->flags == 0)
> +#ifdef CONFIG_EFI_LOADER
> + if (entry->context == ENVCTX_UEFI &&
> + entry->flags == ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE)
> + return 0;
> +#endif
> + /* ENVCTX_UBOOT */
> + if (entry->flags == ENV_FLAGS_VARSTORAGE_NON_VOLATILE)
> return 0;
No; we should not have such #ifdefery here. Please get rid of this.
We have contexts, we should not handle these through #ifdef.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There are three ways to get something done: do it yourself, hire
someone, or forbid your kids to do it.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC,PATCH v4 14/16] env: fat: support UEFI context
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (12 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 13/16] cmd: env: show variable storage attribute in "env flags" command AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-19 9:08 ` Wolfgang Denk
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 15/16] env, efi_loader: define flags for well-known global UEFI variables AKASHI Takahiro
` (3 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
With this patch, UEFI will be allowed to save/load variables to
fat filesystem. The following configurations should be defined.
CONFIG_EFI_ENV_FAT
CONFIG_EFI_ENV_FAT_INTERFACE
CONFIG_EFI_ENV_FAT_DEVICE_AND_PART
CONFIG_EFI_ENV_FAT_FILE
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
env/Kconfig | 42 +++++++++++++++++++++++++++++++++++++++++-
env/fat.c | 11 ++++++++++-
include/environment.h | 4 ++++
3 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/env/Kconfig b/env/Kconfig
index b9439171fd56..23e450a3e5bd 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -402,7 +402,7 @@ config ENV_FAT_INTERFACE
Define this to a string that is the name of the block device.
config ENV_FAT_DEVICE_AND_PART
- string "Device and partition for where to store the environemt in FAT"
+ string "Device and partition for where to store the environment in FAT"
depends on ENV_IS_IN_FAT
default "0:1" if TI_COMMON_CMD_OPTIONS
default "0:auto" if ARCH_ZYNQMP
@@ -432,6 +432,46 @@ config ENV_FAT_FILE
It's a string of the FAT file name. This file use to store the
environment.
+config ENV_EFI_FAT
+ bool "Enable UEFI variable context on a FAT filesystem"
+ depends on ENV_IS_IN_FAT
+ depends on EFI_VARIABLE_USE_ENV
+ help
+ Enable this option if you want to load/save UEFI variables
+ from/to fat file system.
+
+if ENV_EFI_FAT
+config ENV_EFI_FAT_INTERFACE
+ string "UEFI: Name of the block device for the environment"
+ help
+ Define this to a string that is the name of the block device.
+
+config ENV_EFI_FAT_DEVICE_AND_PART
+ string "UEFI: Device and partition for where to store the environemt in FAT"
+ help
+ Define this to a string to specify the partition of the device. It can
+ be as following:
+
+ "D:P", "D:0", "D", "D:" or "D:auto" (D, P are integers. And P >= 1)
+ - "D:P": device D partition P. Error occurs if device D has no
+ partition table.
+ - "D:0": device D.
+ - "D" or "D:": device D partition 1 if device D has partition
+ table, or the whole device D if has no partition
+ table.
+ - "D:auto": first partition in device D with bootable flag set.
+ If none, first valid partition in device D. If no
+ partition table then means device D.
+
+config ENV_EFI_FAT_FILE
+ string "UEFI: Name of the FAT file to use for the environment"
+ default "uboot_efi.env"
+ help
+ It's a string of the FAT file name. This file use to store the
+ environment.
+
+endif
+
config ENV_EXT4_INTERFACE
string "Name of the block device for the environment"
depends on ENV_IS_IN_EXT4
diff --git a/env/fat.c b/env/fat.c
index e4a672d2730a..963ddaf8e9f6 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -30,7 +30,7 @@
# endif
#endif
-static struct evn_fat_context {
+static struct env_fat_context {
char *interface;
char *dev_and_part;
char *file;
@@ -43,6 +43,15 @@ static struct evn_fat_context {
CONFIG_ENV_FAT_FILE,
},
#endif
+#if defined(CONFIG_ENV_EFI_FAT_INTERFACE) && \
+ defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && \
+ defined(CONFIG_ENV_EFI_FAT_FILE)
+ [ENVCTX_UEFI] = {
+ CONFIG_ENV_EFI_FAT_INTERFACE,
+ CONFIG_ENV_EFI_FAT_DEVICE_AND_PART,
+ CONFIG_ENV_EFI_FAT_FILE,
+ },
+#endif
};
#ifdef CMD_SAVEENV
diff --git a/include/environment.h b/include/environment.h
index 9fa085a9b728..12b562823eb0 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -214,6 +214,10 @@ enum env_operation {
enum env_context {
ENVCTX_UBOOT,
+#ifdef CONFIG_EFI_LOADER
+ /* Even if !EFI_VARIABLE_USE_ENV, "env -e" should work */
+ ENVCTX_UEFI,
+#endif
ENVCTX_COUNT,
};
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC,PATCH v4 14/16] env: fat: support UEFI context
2019-07-17 8:25 ` [U-Boot] [RFC,PATCH v4 14/16] env: fat: support UEFI context AKASHI Takahiro
@ 2019-07-19 9:08 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 9:08 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190717082525.891-15-takahiro.akashi@linaro.org> you wrote:
> With this patch, UEFI will be allowed to save/load variables to
> fat filesystem. The following configurations should be defined.
...
> +#if defined(CONFIG_ENV_EFI_FAT_INTERFACE) && \
> + defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && \
> + defined(CONFIG_ENV_EFI_FAT_FILE)
> + [ENVCTX_UEFI] = {
> + CONFIG_ENV_EFI_FAT_INTERFACE,
> + CONFIG_ENV_EFI_FAT_DEVICE_AND_PART,
> + CONFIG_ENV_EFI_FAT_FILE,
> + },
> +#endif
This looks bad to me, as you will not get any error when one of the
defintions is missing.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Systems programmers are the high priests of a low cult.
-- R.S. Barton
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 15/16] env, efi_loader: define flags for well-known global UEFI variables
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (13 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC,PATCH v4 14/16] env: fat: support UEFI context AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 16/16] efi_loader: variable: rework with new extended env interfaces AKASHI Takahiro
` (2 subsequent siblings)
17 siblings, 0 replies; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
Those global UEFI variables are well defined in UEFI specification,
and their attributes in U-Boot environment should be enforced with
specific values.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
include/env_flags.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/include/env_flags.h b/include/env_flags.h
index 15391a28de77..184a42a2990a 100644
--- a/include/env_flags.h
+++ b/include/env_flags.h
@@ -78,10 +78,27 @@ enum env_flags_varstorage {
#define SERIAL_FLAGS ""
#endif
+#ifdef CONFIG_EFI_VARIABLE_USE_ENV
+ /* All the values are stringified in hex representation */
+#define UEFI_FLAGS \
+ "efi_Boot[0-9A-F]*_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:san," \
+ "efi_BootCurrent_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:sav," \
+ "efi_BootNext_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:san," \
+ "efi_BootOrder_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:san," \
+ "efi_OsIndications_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:san," \
+ "efi_OsIndicationsSupported_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:sav," \
+ "efi_PlatfromLang_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:san," \
+ "efi_PlatfromLangCodes_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:sav," \
+ "efi_RuntimeServicesSupported_8be4df61-93ca-11d2-aa-0d-00-e0-98-03-2b-8c:srv,"
+#else
+#define UEFI_FLAGS ""
+#endif
+
#define ENV_FLAGS_LIST_STATIC \
ETHADDR_FLAGS \
NET_FLAGS \
SERIAL_FLAGS \
+ UEFI_FLAGS \
CONFIG_ENV_FLAGS_LIST_STATIC
#ifdef CONFIG_CMD_ENV_FLAGS
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 16/16] efi_loader: variable: rework with new extended env interfaces
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (14 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 15/16] env, efi_loader: define flags for well-known global UEFI variables AKASHI Takahiro
@ 2019-07-17 8:25 ` AKASHI Takahiro
2019-07-17 18:53 ` Heinrich Schuchardt
2019-07-17 19:05 ` [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support Heinrich Schuchardt
2019-07-19 6:50 ` Wolfgang Denk
17 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-17 8:25 UTC (permalink / raw)
To: u-boot
In UEFI variable implementation, ENVCTX_UEFI is used for context
and VARSTORAGE_VOLATILE and VARSTORAGE_NON_VOLATILE_AUTOSAVE are
allowed for variable storage attribute.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
include/efi_loader.h | 1 +
lib/efi_loader/Kconfig | 12 +++++++++
lib/efi_loader/Makefile | 2 +-
lib/efi_loader/efi_setup.c | 5 ++++
lib/efi_loader/efi_variable.c | 50 ++++++++++++++++++++++++++---------
5 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index b07155cecb7c..e119d2d2a802 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -617,6 +617,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
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 efi_variable_init(void);
/*
* See section 3.1.3 in the v2.7 UEFI spec for more details on
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index cd5436c576b1..60085a81144d 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -18,6 +18,18 @@ config EFI_LOADER
if EFI_LOADER
+choice
+ prompt "Select variables storage"
+ default EFI_VARIABLE_USE_ENV
+ help
+ With this option, UEFI variables are implemented using
+ U-Boot environment variables.
+
+config EFI_VARIABLE_USE_ENV
+ bool "Dedicated context in U-Boot environment"
+
+endchoice
+
config EFI_GET_TIME
bool "GetTime() runtime service"
depends on DM_RTC
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 01769ea58ba6..a1fb0f112e5c 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -31,7 +31,7 @@ obj-y += efi_root_node.o
obj-y += efi_runtime.o
obj-y += efi_setup.o
obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
-obj-y += efi_variable.o
+obj-$(CONFIG_EFI_VARIABLE_USE_ENV) += efi_variable.o
obj-y += efi_watchdog.o
obj-$(CONFIG_LCD) += efi_gop.o
obj-$(CONFIG_DM_VIDEO) += efi_gop.o
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index bfb57836fa9f..e96c92474467 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -102,6 +102,11 @@ efi_status_t efi_init_obj_list(void)
/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
switch_to_non_secure_mode();
+ /* Initialize UEFI variables */
+ ret = efi_variable_init();
+ if (ret != EFI_SUCCESS)
+ goto out;
+
/* Define supported languages */
ret = efi_init_platform_lang();
if (ret != EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index d6b75ca02e45..115a1f593058 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -8,6 +8,7 @@
#include <malloc.h>
#include <charset.h>
#include <efi_loader.h>
+#include <exports.h>
#include <hexdump.h>
#include <environment.h>
#include <search.h>
@@ -184,7 +185,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
EFI_PRINT("get '%s'\n", native_name);
- val = env_get(native_name);
+ val = env_get_ext(ENVCTX_UEFI, native_name, NULL);
free(native_name);
if (!val)
return EFI_EXIT(EFI_NOT_FOUND);
@@ -272,7 +273,7 @@ static efi_status_t parse_uboot_variable(char *variable,
const efi_guid_t *vendor,
u32 *attributes)
{
- char *guid, *name, *end, c;
+ char *guid, *name, *env_attr, *end, c;
unsigned long name_len;
u16 *p;
@@ -284,11 +285,14 @@ static efi_status_t parse_uboot_variable(char *variable,
if (!name)
return EFI_INVALID_PARAMETER;
name++;
- end = strchr(name, '=');
+ env_attr = strchr(name, ':');
+ if (!env_attr)
+ return EFI_INVALID_PARAMETER;
+ end = strchr(env_attr, '=');
if (!end)
return EFI_INVALID_PARAMETER;
- name_len = end - name;
+ name_len = env_attr - name;
if (*variable_name_size < (name_len + 1)) {
*variable_name_size = name_len + 1;
return EFI_BUFFER_TOO_SMALL;
@@ -360,7 +364,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
name_len = strlen(native_name);
for (variable = efi_variables_list; variable && *variable;) {
if (!strncmp(variable, native_name, name_len) &&
- variable[name_len] == '=')
+ variable[name_len] == ':')
break;
variable = strchr(variable, '\n');
@@ -387,9 +391,9 @@ 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',
- H_MATCH_REGEX | H_MATCH_KEY,
- &efi_variables_list, 0, 1, regexlist);
+ list_len = hexport_ext(&env_htab, ENVCTX_UEFI, '\n',
+ H_MATCH_REGEX | H_MATCH_KEY,
+ &efi_variables_list, 0, 1, regexlist);
/* 1 indicates that no match was found */
if (list_len <= 1)
return EFI_EXIT(EFI_NOT_FOUND);
@@ -424,7 +428,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
{
char *native_name = NULL, *val = NULL, *s;
efi_status_t ret = EFI_SUCCESS;
- u32 attr;
+ u32 attr, flags;
EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
data_size, data);
@@ -446,12 +450,12 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
/* delete the variable: */
- env_set(native_name, NULL);
+ env_set_ext(ENVCTX_UEFI, native_name, 0, NULL);
ret = EFI_SUCCESS;
goto out;
}
- val = env_get(native_name);
+ val = env_get_ext(ENVCTX_UEFI, native_name, NULL);
if (val) {
parse_attr(val, &attr);
@@ -484,6 +488,8 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
* store attributes
* TODO: several attributes are not supported
*/
+ flags = (attributes & EFI_VARIABLE_NON_VOLATILE) ?
+ ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE : 0;
attributes &= (EFI_VARIABLE_NON_VOLATILE |
EFI_VARIABLE_BOOTSERVICE_ACCESS |
EFI_VARIABLE_RUNTIME_ACCESS);
@@ -511,7 +517,8 @@ 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))
+ /* TODO: type: string, access: READ/WRITE/CREATE/DELETE */
+ if (env_set_ext(ENVCTX_UEFI, native_name, flags, val))
ret = EFI_DEVICE_ERROR;
out:
@@ -520,3 +527,22 @@ out:
return EFI_EXIT(ret);
}
+
+efi_status_t efi_variable_init(void)
+{
+ int ret;
+
+ ret = env_load_ext(ENVCTX_UEFI);
+ if (ret)
+#if 1
+ /*
+ * FIXME:
+ * It is valid even if initial file in FAT doesn't exist
+ */
+ return EFI_SUCCESS;
+#else
+ return EFI_DEVICE_ERROR;
+#endif
+
+ return EFI_SUCCESS;
+}
--
2.21.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 16/16] efi_loader: variable: rework with new extended env interfaces
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 16/16] efi_loader: variable: rework with new extended env interfaces AKASHI Takahiro
@ 2019-07-17 18:53 ` Heinrich Schuchardt
2019-07-18 0:13 ` AKASHI Takahiro
0 siblings, 1 reply; 48+ messages in thread
From: Heinrich Schuchardt @ 2019-07-17 18:53 UTC (permalink / raw)
To: u-boot
On 7/17/19 10:25 AM, AKASHI Takahiro wrote:
> In UEFI variable implementation, ENVCTX_UEFI is used for context
> and VARSTORAGE_VOLATILE and VARSTORAGE_NON_VOLATILE_AUTOSAVE are
> allowed for variable storage attribute.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> include/efi_loader.h | 1 +
> lib/efi_loader/Kconfig | 12 +++++++++
> lib/efi_loader/Makefile | 2 +-
> lib/efi_loader/efi_setup.c | 5 ++++
> lib/efi_loader/efi_variable.c | 50 ++++++++++++++++++++++++++---------
> 5 files changed, 57 insertions(+), 13 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index b07155cecb7c..e119d2d2a802 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -617,6 +617,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> 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 efi_variable_init(void);
This initialization function is already defined in origin/master as
efi_status_t efi_init_variables();
>
> /*
> * See section 3.1.3 in the v2.7 UEFI spec for more details on
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index cd5436c576b1..60085a81144d 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -18,6 +18,18 @@ config EFI_LOADER
>
> if EFI_LOADER
>
> +choice
> + prompt "Select variables storage"
> + default EFI_VARIABLE_USE_ENV
> + help
> + With this option, UEFI variables are implemented using
> + U-Boot environment variables.
> +
> +config EFI_VARIABLE_USE_ENV
> + bool "Dedicated context in U-Boot environment"
I could not find any place in the code where EFI_VARIABLE_USE_ENV is
set. Why do you introduce it?
> +
> +endchoice
> +
> config EFI_GET_TIME
> bool "GetTime() runtime service"
> depends on DM_RTC
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 01769ea58ba6..a1fb0f112e5c 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -31,7 +31,7 @@ obj-y += efi_root_node.o
> obj-y += efi_runtime.o
> obj-y += efi_setup.o
> obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> -obj-y += efi_variable.o
> +obj-$(CONFIG_EFI_VARIABLE_USE_ENV) += efi_variable.o
> obj-y += efi_watchdog.o
> obj-$(CONFIG_LCD) += efi_gop.o
> obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index bfb57836fa9f..e96c92474467 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -102,6 +102,11 @@ efi_status_t efi_init_obj_list(void)
> /* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
> switch_to_non_secure_mode();
>
> + /* Initialize UEFI variables */
> + ret = efi_variable_init();
We already call efi_init_variables() here.
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> /* Define supported languages */
> ret = efi_init_platform_lang();
> if (ret != EFI_SUCCESS)
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index d6b75ca02e45..115a1f593058 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -8,6 +8,7 @@
> #include <malloc.h>
> #include <charset.h>
> #include <efi_loader.h>
> +#include <exports.h>
> #include <hexdump.h>
> #include <environment.h>
> #include <search.h>
> @@ -184,7 +185,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
>
> EFI_PRINT("get '%s'\n", native_name);
>
> - val = env_get(native_name);
> + val = env_get_ext(ENVCTX_UEFI, native_name, NULL);
> free(native_name);
> if (!val)
> return EFI_EXIT(EFI_NOT_FOUND);
> @@ -272,7 +273,7 @@ static efi_status_t parse_uboot_variable(char *variable,
> const efi_guid_t *vendor,
> u32 *attributes)
> {
> - char *guid, *name, *end, c;
> + char *guid, *name, *env_attr, *end, c;
> unsigned long name_len;
> u16 *p;
>
> @@ -284,11 +285,14 @@ static efi_status_t parse_uboot_variable(char *variable,
> if (!name)
> return EFI_INVALID_PARAMETER;
> name++;
> - end = strchr(name, '=');
> + env_attr = strchr(name, ':');
> + if (!env_attr)
> + return EFI_INVALID_PARAMETER;
> + end = strchr(env_attr, '=');
> if (!end)
> return EFI_INVALID_PARAMETER;
>
> - name_len = end - name;
> + name_len = env_attr - name;
> if (*variable_name_size < (name_len + 1)) {
> *variable_name_size = name_len + 1;
> return EFI_BUFFER_TOO_SMALL;
> @@ -360,7 +364,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> name_len = strlen(native_name);
> for (variable = efi_variables_list; variable && *variable;) {
> if (!strncmp(variable, native_name, name_len) &&
> - variable[name_len] == '=')
> + variable[name_len] == ':')
> break;
>
> variable = strchr(variable, '\n');
> @@ -387,9 +391,9 @@ 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',
> - H_MATCH_REGEX | H_MATCH_KEY,
> - &efi_variables_list, 0, 1, regexlist);
> + list_len = hexport_ext(&env_htab, ENVCTX_UEFI, '\n',
> + H_MATCH_REGEX | H_MATCH_KEY,
> + &efi_variables_list, 0, 1, regexlist);
> /* 1 indicates that no match was found */
> if (list_len <= 1)
> return EFI_EXIT(EFI_NOT_FOUND);
> @@ -424,7 +428,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> {
> char *native_name = NULL, *val = NULL, *s;
> efi_status_t ret = EFI_SUCCESS;
> - u32 attr;
> + u32 attr, flags;
>
> EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> data_size, data);
> @@ -446,12 +450,12 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
>
> if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> /* delete the variable: */
> - env_set(native_name, NULL);
> + env_set_ext(ENVCTX_UEFI, native_name, 0, NULL);
> ret = EFI_SUCCESS;
> goto out;
> }
>
> - val = env_get(native_name);
> + val = env_get_ext(ENVCTX_UEFI, native_name, NULL);
> if (val) {
> parse_attr(val, &attr);
>
> @@ -484,6 +488,8 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> * store attributes
> * TODO: several attributes are not supported
> */
> + flags = (attributes & EFI_VARIABLE_NON_VOLATILE) ?
> + ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE : 0;
> attributes &= (EFI_VARIABLE_NON_VOLATILE |
> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> EFI_VARIABLE_RUNTIME_ACCESS);
> @@ -511,7 +517,8 @@ 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))
> + /* TODO: type: string, access: READ/WRITE/CREATE/DELETE */
> + if (env_set_ext(ENVCTX_UEFI, native_name, flags, val))
> ret = EFI_DEVICE_ERROR;
>
> out:
> @@ -520,3 +527,22 @@ out:
>
> return EFI_EXIT(ret);
> }
> +
> +efi_status_t efi_variable_init(void)
Please, rebase your patch on the existing coding.
> +{
> + int ret;
> +
> + ret = env_load_ext(ENVCTX_UEFI);
> + if (ret)
> +#if 1
There is no need for the #if. Please, remove the unused code.
> + /*
> + * FIXME:
> + * It is valid even if initial file in FAT doesn't exist
What will the correct handling look like?
Best regards
Heinrich
> + */
> + return EFI_SUCCESS;
> +#else
> + return EFI_DEVICE_ERROR;
> +#endif
> +
> + return EFI_SUCCESS;
> +}
>
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 16/16] efi_loader: variable: rework with new extended env interfaces
2019-07-17 18:53 ` Heinrich Schuchardt
@ 2019-07-18 0:13 ` AKASHI Takahiro
0 siblings, 0 replies; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-18 0:13 UTC (permalink / raw)
To: u-boot
Heinrich,
On Wed, Jul 17, 2019 at 08:53:53PM +0200, Heinrich Schuchardt wrote:
> On 7/17/19 10:25 AM, AKASHI Takahiro wrote:
> >In UEFI variable implementation, ENVCTX_UEFI is used for context
> >and VARSTORAGE_VOLATILE and VARSTORAGE_NON_VOLATILE_AUTOSAVE are
> >allowed for variable storage attribute.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> > include/efi_loader.h | 1 +
> > lib/efi_loader/Kconfig | 12 +++++++++
> > lib/efi_loader/Makefile | 2 +-
> > lib/efi_loader/efi_setup.c | 5 ++++
> > lib/efi_loader/efi_variable.c | 50 ++++++++++++++++++++++++++---------
> > 5 files changed, 57 insertions(+), 13 deletions(-)
> >
> >diff --git a/include/efi_loader.h b/include/efi_loader.h
> >index b07155cecb7c..e119d2d2a802 100644
> >--- a/include/efi_loader.h
> >+++ b/include/efi_loader.h
> >@@ -617,6 +617,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> > 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 efi_variable_init(void);
>
> This initialization function is already defined in origin/master as
>
> efi_status_t efi_init_variables();
As I mentioned in the previous reply, the current patch set is
based on v2019.07.
> >
> > /*
> > * See section 3.1.3 in the v2.7 UEFI spec for more details on
> >diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >index cd5436c576b1..60085a81144d 100644
> >--- a/lib/efi_loader/Kconfig
> >+++ b/lib/efi_loader/Kconfig
> >@@ -18,6 +18,18 @@ config EFI_LOADER
> >
> > if EFI_LOADER
> >
> >+choice
> >+ prompt "Select variables storage"
> >+ default EFI_VARIABLE_USE_ENV
> >+ help
> >+ With this option, UEFI variables are implemented using
> >+ U-Boot environment variables.
> >+
> >+config EFI_VARIABLE_USE_ENV
> >+ bool "Dedicated context in U-Boot environment"
>
> I could not find any place in the code where EFI_VARIABLE_USE_ENV is
> set. Why do you introduce it?
Set by default in "choice."
Once another implementation like StMM be added, you will see choices.
> >+
> >+endchoice
> >+
> > config EFI_GET_TIME
> > bool "GetTime() runtime service"
> > depends on DM_RTC
> >diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> >index 01769ea58ba6..a1fb0f112e5c 100644
> >--- a/lib/efi_loader/Makefile
> >+++ b/lib/efi_loader/Makefile
> >@@ -31,7 +31,7 @@ obj-y += efi_root_node.o
> > obj-y += efi_runtime.o
> > obj-y += efi_setup.o
> > obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> >-obj-y += efi_variable.o
> >+obj-$(CONFIG_EFI_VARIABLE_USE_ENV) += efi_variable.o
> > obj-y += efi_watchdog.o
> > obj-$(CONFIG_LCD) += efi_gop.o
> > obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> >diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> >index bfb57836fa9f..e96c92474467 100644
> >--- a/lib/efi_loader/efi_setup.c
> >+++ b/lib/efi_loader/efi_setup.c
> >@@ -102,6 +102,11 @@ efi_status_t efi_init_obj_list(void)
> > /* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
> > switch_to_non_secure_mode();
> >
> >+ /* Initialize UEFI variables */
> >+ ret = efi_variable_init();
>
> We already call efi_init_variables() here.
>
> >+ if (ret != EFI_SUCCESS)
> >+ goto out;
> >+
> > /* Define supported languages */
> > ret = efi_init_platform_lang();
> > if (ret != EFI_SUCCESS)
> >diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >index d6b75ca02e45..115a1f593058 100644
> >--- a/lib/efi_loader/efi_variable.c
> >+++ b/lib/efi_loader/efi_variable.c
> >@@ -8,6 +8,7 @@
> > #include <malloc.h>
> > #include <charset.h>
> > #include <efi_loader.h>
> >+#include <exports.h>
> > #include <hexdump.h>
> > #include <environment.h>
> > #include <search.h>
> >@@ -184,7 +185,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> >
> > EFI_PRINT("get '%s'\n", native_name);
> >
> >- val = env_get(native_name);
> >+ val = env_get_ext(ENVCTX_UEFI, native_name, NULL);
> > free(native_name);
> > if (!val)
> > return EFI_EXIT(EFI_NOT_FOUND);
> >@@ -272,7 +273,7 @@ static efi_status_t parse_uboot_variable(char *variable,
> > const efi_guid_t *vendor,
> > u32 *attributes)
> > {
> >- char *guid, *name, *end, c;
> >+ char *guid, *name, *env_attr, *end, c;
> > unsigned long name_len;
> > u16 *p;
> >
> >@@ -284,11 +285,14 @@ static efi_status_t parse_uboot_variable(char *variable,
> > if (!name)
> > return EFI_INVALID_PARAMETER;
> > name++;
> >- end = strchr(name, '=');
> >+ env_attr = strchr(name, ':');
> >+ if (!env_attr)
> >+ return EFI_INVALID_PARAMETER;
> >+ end = strchr(env_attr, '=');
> > if (!end)
> > return EFI_INVALID_PARAMETER;
> >
> >- name_len = end - name;
> >+ name_len = env_attr - name;
> > if (*variable_name_size < (name_len + 1)) {
> > *variable_name_size = name_len + 1;
> > return EFI_BUFFER_TOO_SMALL;
> >@@ -360,7 +364,7 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> > name_len = strlen(native_name);
> > for (variable = efi_variables_list; variable && *variable;) {
> > if (!strncmp(variable, native_name, name_len) &&
> >- variable[name_len] == '=')
> >+ variable[name_len] == ':')
> > break;
> >
> > variable = strchr(variable, '\n');
> >@@ -387,9 +391,9 @@ 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',
> >- H_MATCH_REGEX | H_MATCH_KEY,
> >- &efi_variables_list, 0, 1, regexlist);
> >+ list_len = hexport_ext(&env_htab, ENVCTX_UEFI, '\n',
> >+ H_MATCH_REGEX | H_MATCH_KEY,
> >+ &efi_variables_list, 0, 1, regexlist);
> > /* 1 indicates that no match was found */
> > if (list_len <= 1)
> > return EFI_EXIT(EFI_NOT_FOUND);
> >@@ -424,7 +428,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> > {
> > char *native_name = NULL, *val = NULL, *s;
> > efi_status_t ret = EFI_SUCCESS;
> >- u32 attr;
> >+ u32 attr, flags;
> >
> > EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> > data_size, data);
> >@@ -446,12 +450,12 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> >
> > if ((data_size == 0) || !(attributes & ACCESS_ATTR)) {
> > /* delete the variable: */
> >- env_set(native_name, NULL);
> >+ env_set_ext(ENVCTX_UEFI, native_name, 0, NULL);
> > ret = EFI_SUCCESS;
> > goto out;
> > }
> >
> >- val = env_get(native_name);
> >+ val = env_get_ext(ENVCTX_UEFI, native_name, NULL);
> > if (val) {
> > parse_attr(val, &attr);
> >
> >@@ -484,6 +488,8 @@ efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> > * store attributes
> > * TODO: several attributes are not supported
> > */
> >+ flags = (attributes & EFI_VARIABLE_NON_VOLATILE) ?
> >+ ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE : 0;
> > attributes &= (EFI_VARIABLE_NON_VOLATILE |
> > EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > EFI_VARIABLE_RUNTIME_ACCESS);
> >@@ -511,7 +517,8 @@ 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))
> >+ /* TODO: type: string, access: READ/WRITE/CREATE/DELETE */
> >+ if (env_set_ext(ENVCTX_UEFI, native_name, flags, val))
> > ret = EFI_DEVICE_ERROR;
> >
> > out:
> >@@ -520,3 +527,22 @@ out:
> >
> > return EFI_EXIT(ret);
> > }
> >+
> >+efi_status_t efi_variable_init(void)
>
> Please, rebase your patch on the existing coding.
>
>
> >+{
> >+ int ret;
> >+
> >+ ret = env_load_ext(ENVCTX_UEFI);
> >+ if (ret)
> >+#if 1
>
> There is no need for the #if. Please, remove the unused code.
I know. This is just a reminder of the "FIXME" below.
> >+ /*
> >+ * FIXME:
> >+ * It is valid even if initial file in FAT doesn't exist
>
> What will the correct handling look like?
Probably env_load() should be modified consistently across all drivers
to return, say, ENOENT? so that we can distinguish and handle the case
properly.
Thanks,
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> >+ */
> >+ return EFI_SUCCESS;
> >+#else
> >+ return EFI_DEVICE_ERROR;
> >+#endif
> >+
> >+ return EFI_SUCCESS;
> >+}
> >
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (15 preceding siblings ...)
2019-07-17 8:25 ` [U-Boot] [RFC, PATCH v4 16/16] efi_loader: variable: rework with new extended env interfaces AKASHI Takahiro
@ 2019-07-17 19:05 ` Heinrich Schuchardt
2019-07-18 0:04 ` AKASHI Takahiro
2019-07-19 6:50 ` Wolfgang Denk
17 siblings, 1 reply; 48+ messages in thread
From: Heinrich Schuchardt @ 2019-07-17 19:05 UTC (permalink / raw)
To: u-boot
On 7/17/19 10:25 AM, AKASHI Takahiro wrote:
> # In version 4 of this patch set, the implementation is changed to meet
> # Wolfgang's requirements.
> # I believe that this is NOT intrusive, and that my approach here is NOT
> # selfish at all. If Wolfgang doesn't accept this approach, however,
> # I would like to go for "Plan B" for UEFI variables implementation.
>
> This patch set is an attempt to implement non-volatile attribute for
> UEFI variables. Under the current implementation,
> * SetVariable API doesn't recognize non-volatile attribute
> * While some variables are defined non-volatile in UEFI specification,
> they are NOT marked as non-volatile in the code.
> * env_save() (or "env save" command) allows us to save all the variables
> into persistent storage, but it may cause volatile UEFI variables,
> along with irrelevant U-Boot variables, to be saved unconditionally.
>
> Those observation rationalizes that the implementation of UEFI variables
> should be revamped utilizing dedicated storage for them.
>
> Basic ideas:
> * Each variable may be labelled with a "context." Variables with
> different contexts will be loaded/saved in different storages.
> Currently, we define two contexts:
>
> ENVCTX_UBOOT: U-Boot environment variables
> ENVCTX_UEFI: UEFI variables
>
> * Each variable may have a third attribute, "variable storage."
> There are three new flags defined:
> ('flags' are a bit-wise representation of attributes.)
>
> ENV_FLAGS_VARSTORAGE_VOLATILE: A variable only exists during runtime.
> ENV_FLAGS_VARSTORAGE_NON_VOLATILE: A variable is persistent, but explicit
> "load" and "save" command is required.
> ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE: A variable is persistent,
> any change will be written back to storage immediately.
>
> * Following those extensions, new interfaces are introduced:
> for hashtable,
> hsearch_r -> hsearch_ext
> hmatch_r -> hmatch_ext
> hdelete_r -> hdelete_ext
> himport_r -> himport_ext
> hexport_r -> hexport_ext
> for env,
> env_save -> env_save_ext
> env_load -> env_load_ext
> env_import -> env_import_ext
> env_export -> env_export_ext
> They will take one or two additional arguments, context and flags,
> comparing with the existing corresponding interfaces.
>
> * Even with those changes, existing interfaces will maintain
> its semantics, as if all the U-Boot environment variables were with
> UEFI_UBOOT context and VARSTORAGE_NON_VOLATILE attribute.
> (The only exception is "env_load()." See below.)
>
> * There is one thing visible to users; *Exported* variables now have
> attribute information, which is appended to variable's name
> in a format of ":xxx"
> For example,
> => printenv
> arch:san=arm
> baudrate:san=115200
> board:san=qemu-arm
> ...
> This information is necessary to preserve attributes across reboot
> (save/load).
>
> * Each environment storage driver must be modified so as to be aware
> of contexts. (See flash and fat in my patch set.)
>
> Known issues/restriction/TODO:
> * Existing interfaces may be marked obsolute or deleted in the future.
>
> * Currently, only flash and fat drivers are modified to support contexts.
> * Currently, only fat driver is modified to support UEFI context.
>
> -> Applying changes to the other drivers is straightforward.
>
> * ENV_IS_NOWHERE doesn't work if we want to disable U-Boot environment,
> but still want to enable UEFI (hence UEFI variables).
>
> -> One solution would be to add *intermediate* configurations
> to set ENV_IS_NOWHERE properly in such a case.
>
> * Any context won't have VARSTORAGE_NON_VOLATILE and
> VARSTORAGE_NON_VOLATILE_AUTOSAVE entries at the same time. This is
> because it will make "save" operation ugly and unnecessarily complicated
> as we discussed in ML.
>
> -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
> to a context itself.
>
> * env_load() may lose some of compatibility; It now always imports variables
> *without* clearing existing entries (i.e. H_NOCLEAR specified) in order
> to support multiple contexts simultaneously.
>
> -> I have no other good idea to solve this issue.
>
> * Unfortunately, some of existing U-Boot variables seem to be
> "volatile" in nature. For example, we will see errors at boot time:
> ...
> Loading Environment from Flash... OK
> ## Error inserting "fdtcontroladdr" variable, errno=22
> In: pl011 at 9000000
> Out: pl011 at 9000000
> Err: pl011 at 9000000
> ## Error inserting "stdin" variable, errno=22
> ## Error inserting "stdout" variable, errno=22
> ## Error inserting "stderr" variable, errno=22
>
> -> We will have to changes their attributes one-by-one.
> (Those can also be set through ENV_FLAGS_LIST_STATIC in env_flags.h).
>
> * As described above, attribute information is visible to users.
> I'm afraid that it might lead to any incompatibility somewhere.
> (I don't notice any issues though.)
>
> * The whole area of storage will be saved at *every* update of
> one UEFI variable. It should be optimized if possible.
>
> * An error during "save" operation may cause inconsistency between cache
> (hash table) and the storage.
>
> -> This is not UEFI specific though.
>
> * Patch#15 may be unnecessary.
>
> * Add tests if necessary.
>
> Note:
> If we need "secure storage" for UEFI variables, efi_get_variable/
> efi_get_next_variable_name/efi_set_variable() should be completely replaced
> with stub functions to communicate with secure world.
> This patch won't affect such an implementation.
>
> Usage:
> To enable this feature, the following configs must be enabled:
> CONFIG_ENV_IS_IN_FAT
> CONFIG_ENV_FAT_INTERFACE
> CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
> CONFIG_ENV_EFI_FAT_FILE
>
> You can also define a non-volatile variable from command interface:
> => setenv -e -nv FOO baa
>
> Patch#1 to #5 are to add 'context' support to env interfaces.
> Patch#6 to #13 are to add 'variable storage' attribute to env interfaces.
> Patch#14 to #16 are to modify UEFI variable implementation to utilize
> new env interfaces.
>
> Changes in v4 (July 17, 2019)
> * remove already-merged patches
> * revamp after Wolfgang's suggestion
>
> Changes in v3 (June 4, 2019)
> * remove already-merged patches
> * revamp the code again
> * introduce CONFIG_EFI_VARIABLE_USE_ENV for this configuration.
> Once another backing storage, i.e. StMM services for secure boot,
> is supported, another option will be added.
>
> Changes in v2 (Apr 24, 2019)
> * rebased on efi-2019-07
> * revamp the implementation
>
> v1 (Nov 28, 2018)
> * initial
Thanks for all the effort you put into getting this implemented.
Unfortunately the code does not compile after applying patches 1-15 for
sandbox_defconfig:
In file included from include/asm-generic/global_data.h:23,
from ./arch/sandbox/include/asm/global_data.h:18,
from include/dm/of.h:11,
from include/dm/ofnode.h:12,
from include/dm/device.h:13,
from include/dm.h:9,
from drivers/net/mdio_sandbox.c:7:
include/environment.h:158:19: error: ‘CONFIG_ENV_SIZE’ undeclared here
(not in a function); did you mean ‘CONFIG_OF_LIVE’?
#define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
^~~~~~~~~~~~~~~
include/environment.h:162:21: note: in expansion of macro ‘ENV_SIZE’
unsigned char data[ENV_SIZE]; /* Environment data */
^~~~~~~~
CC drivers/power/domain/sandbox-power-domain.o
Here is another error:
CC cmd/nvedit.o
cmd/nvedit.c: In function ‘do_env_print_ext’:
cmd/nvedit.c:129:13: error: ‘ENVCTX_UEFI’ undeclared (first use in this
function); did you mean ‘ENVCTX_UBOOT’?
if (ctx == ENVCTX_UEFI)
^~~~~~~~~~~
ENVCTX_UBOOT
cmd/nvedit.c:129:13: note: each undeclared identifier is reported only
once for each function it appears in
Please, run Travis CI before resubmitting.
Patch 16 is not based on origin/master and cannot be applied.
I found no adjustments to test/env/*. How will your changes be tested?
Best regards
Heinrich
>
> AKASHI Takahiro (16):
> hashtable: extend interfaces to handle entries with context
> env: extend interfaces to import/export U-Boot environment per context
> env: extend interfaces to label variable with context
> env: flash: add U-Boot environment context support
> env: fat: add U-Boot environment context support
> env: add variable storage attribute support
> env: add/expose attribute helper functions for hashtable
> hashtable: import/export entries with flags
> hashtable: extend hdelete_ext for autosave
> env: save non-volatile variables only
> env: save a context immediately if 'autosave' variable is changed
> env: extend interfaces to get/set attributes
> cmd: env: show variable storage attribute in "env flags" command
> env: fat: support UEFI context
> env,efi_loader: define flags for well-known global UEFI variables
> efi_loader: variable: rework with new extended env interfaces
>
> cmd/nvedit.c | 186 ++++++++++++++++++++++--------
> env/Kconfig | 42 ++++++-
> env/common.c | 82 ++++++++++---
> env/env.c | 92 +++++++++++----
> env/fat.c | 112 +++++++++++++++---
> env/flags.c | 149 +++++++++++++++++++++++-
> env/flash.c | 28 +++--
> include/asm-generic/global_data.h | 7 +-
> include/efi_loader.h | 1 +
> include/env_default.h | 1 +
> include/env_flags.h | 63 +++++++++-
> include/environment.h | 89 ++++++++++++--
> include/exports.h | 4 +
> include/search.h | 16 +++
> lib/efi_loader/Kconfig | 12 ++
> lib/efi_loader/Makefile | 2 +-
> lib/efi_loader/efi_setup.c | 5 +
> lib/efi_loader/efi_variable.c | 50 ++++++--
> lib/hashtable.c | 143 ++++++++++++++++++++---
> 19 files changed, 925 insertions(+), 159 deletions(-)
>
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support
2019-07-17 19:05 ` [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support Heinrich Schuchardt
@ 2019-07-18 0:04 ` AKASHI Takahiro
0 siblings, 0 replies; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-18 0:04 UTC (permalink / raw)
To: u-boot
Heinrich,
On Wed, Jul 17, 2019 at 09:05:55PM +0200, Heinrich Schuchardt wrote:
> On 7/17/19 10:25 AM, AKASHI Takahiro wrote:
> ># In version 4 of this patch set, the implementation is changed to meet
> ># Wolfgang's requirements.
> ># I believe that this is NOT intrusive, and that my approach here is NOT
> ># selfish at all. If Wolfgang doesn't accept this approach, however,
> ># I would like to go for "Plan B" for UEFI variables implementation.
> >
> >This patch set is an attempt to implement non-volatile attribute for
> >UEFI variables. Under the current implementation,
> >* SetVariable API doesn't recognize non-volatile attribute
> >* While some variables are defined non-volatile in UEFI specification,
> > they are NOT marked as non-volatile in the code.
> >* env_save() (or "env save" command) allows us to save all the variables
> > into persistent storage, but it may cause volatile UEFI variables,
> > along with irrelevant U-Boot variables, to be saved unconditionally.
> >
> >Those observation rationalizes that the implementation of UEFI variables
> >should be revamped utilizing dedicated storage for them.
> >
> >Basic ideas:
> >* Each variable may be labelled with a "context." Variables with
> > different contexts will be loaded/saved in different storages.
> > Currently, we define two contexts:
> >
> > ENVCTX_UBOOT: U-Boot environment variables
> > ENVCTX_UEFI: UEFI variables
> >
> >* Each variable may have a third attribute, "variable storage."
> > There are three new flags defined:
> > ('flags' are a bit-wise representation of attributes.)
> >
> > ENV_FLAGS_VARSTORAGE_VOLATILE: A variable only exists during runtime.
> > ENV_FLAGS_VARSTORAGE_NON_VOLATILE: A variable is persistent, but explicit
> > "load" and "save" command is required.
> > ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE: A variable is persistent,
> > any change will be written back to storage immediately.
> >
> >* Following those extensions, new interfaces are introduced:
> > for hashtable,
> > hsearch_r -> hsearch_ext
> > hmatch_r -> hmatch_ext
> > hdelete_r -> hdelete_ext
> > himport_r -> himport_ext
> > hexport_r -> hexport_ext
> > for env,
> > env_save -> env_save_ext
> > env_load -> env_load_ext
> > env_import -> env_import_ext
> > env_export -> env_export_ext
> > They will take one or two additional arguments, context and flags,
> > comparing with the existing corresponding interfaces.
> >
> >* Even with those changes, existing interfaces will maintain
> > its semantics, as if all the U-Boot environment variables were with
> > UEFI_UBOOT context and VARSTORAGE_NON_VOLATILE attribute.
> > (The only exception is "env_load()." See below.)
> >
> >* There is one thing visible to users; *Exported* variables now have
> > attribute information, which is appended to variable's name
> > in a format of ":xxx"
> > For example,
> > => printenv
> > arch:san=arm
> > baudrate:san=115200
> > board:san=qemu-arm
> > ...
> > This information is necessary to preserve attributes across reboot
> > (save/load).
> >
> >* Each environment storage driver must be modified so as to be aware
> > of contexts. (See flash and fat in my patch set.)
> >
> >Known issues/restriction/TODO:
> >* Existing interfaces may be marked obsolute or deleted in the future.
> >
> >* Currently, only flash and fat drivers are modified to support contexts.
> >* Currently, only fat driver is modified to support UEFI context.
> >
> > -> Applying changes to the other drivers is straightforward.
> >
> >* ENV_IS_NOWHERE doesn't work if we want to disable U-Boot environment,
> > but still want to enable UEFI (hence UEFI variables).
> >
> > -> One solution would be to add *intermediate* configurations
> > to set ENV_IS_NOWHERE properly in such a case.
> >
> >* Any context won't have VARSTORAGE_NON_VOLATILE and
> > VARSTORAGE_NON_VOLATILE_AUTOSAVE entries at the same time. This is
> > because it will make "save" operation ugly and unnecessarily complicated
> > as we discussed in ML.
> >
> > -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
> > to a context itself.
> >
> >* env_load() may lose some of compatibility; It now always imports variables
> > *without* clearing existing entries (i.e. H_NOCLEAR specified) in order
> > to support multiple contexts simultaneously.
> >
> > -> I have no other good idea to solve this issue.
> >
> >* Unfortunately, some of existing U-Boot variables seem to be
> > "volatile" in nature. For example, we will see errors at boot time:
> > ...
> > Loading Environment from Flash... OK
> > ## Error inserting "fdtcontroladdr" variable, errno=22
> > In: pl011 at 9000000
> > Out: pl011 at 9000000
> > Err: pl011 at 9000000
> > ## Error inserting "stdin" variable, errno=22
> > ## Error inserting "stdout" variable, errno=22
> > ## Error inserting "stderr" variable, errno=22
> >
> > -> We will have to changes their attributes one-by-one.
> > (Those can also be set through ENV_FLAGS_LIST_STATIC in env_flags.h).
> >
> >* As described above, attribute information is visible to users.
> > I'm afraid that it might lead to any incompatibility somewhere.
> > (I don't notice any issues though.)
> >
> >* The whole area of storage will be saved at *every* update of
> > one UEFI variable. It should be optimized if possible.
> >
> >* An error during "save" operation may cause inconsistency between cache
> > (hash table) and the storage.
> >
> > -> This is not UEFI specific though.
> >
> >* Patch#15 may be unnecessary.
> >
> >* Add tests if necessary.
> >
> >Note:
> >If we need "secure storage" for UEFI variables, efi_get_variable/
> >efi_get_next_variable_name/efi_set_variable() should be completely replaced
> >with stub functions to communicate with secure world.
> >This patch won't affect such an implementation.
> >
> >Usage:
> >To enable this feature, the following configs must be enabled:
> > CONFIG_ENV_IS_IN_FAT
> > CONFIG_ENV_FAT_INTERFACE
> > CONFIG_ENV_EFI_FAT_DEVICE_AND_PART
> > CONFIG_ENV_EFI_FAT_FILE
> >
> >You can also define a non-volatile variable from command interface:
> >=> setenv -e -nv FOO baa
> >
> >Patch#1 to #5 are to add 'context' support to env interfaces.
> >Patch#6 to #13 are to add 'variable storage' attribute to env interfaces.
> >Patch#14 to #16 are to modify UEFI variable implementation to utilize
> > new env interfaces.
> >
> >Changes in v4 (July 17, 2019)
> >* remove already-merged patches
> >* revamp after Wolfgang's suggestion
> >
> >Changes in v3 (June 4, 2019)
> >* remove already-merged patches
> >* revamp the code again
> >* introduce CONFIG_EFI_VARIABLE_USE_ENV for this configuration.
> > Once another backing storage, i.e. StMM services for secure boot,
> > is supported, another option will be added.
> >
> >Changes in v2 (Apr 24, 2019)
> >* rebased on efi-2019-07
> >* revamp the implementation
> >
> >v1 (Nov 28, 2018)
> >* initial
>
> Thanks for all the effort you put into getting this implemented.
>
> Unfortunately the code does not compile after applying patches 1-15 for
> sandbox_defconfig:
>
> In file included from include/asm-generic/global_data.h:23,
> from ./arch/sandbox/include/asm/global_data.h:18,
> from include/dm/of.h:11,
> from include/dm/ofnode.h:12,
> from include/dm/device.h:13,
> from include/dm.h:9,
> from drivers/net/mdio_sandbox.c:7:
> include/environment.h:158:19: error: ‘CONFIG_ENV_SIZE’ undeclared here
> (not in a function); did you mean ‘CONFIG_OF_LIVE’?
> #define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
> ^~~~~~~~~~~~~~~
> include/environment.h:162:21: note: in expansion of macro ‘ENV_SIZE’
> unsigned char data[ENV_SIZE]; /* Environment data */
> ^~~~~~~~
> CC drivers/power/domain/sandbox-power-domain.o
>
> Here is another error:
>
> CC cmd/nvedit.o
> cmd/nvedit.c: In function ‘do_env_print_ext’:
> cmd/nvedit.c:129:13: error: ‘ENVCTX_UEFI’ undeclared (first use in this
> function); did you mean ‘ENVCTX_UBOOT’?
> if (ctx == ENVCTX_UEFI)
> ^~~~~~~~~~~
> ENVCTX_UBOOT
> cmd/nvedit.c:129:13: note: each undeclared identifier is reported only
> once for each function it appears in
>
> Please, run Travis CI before resubmitting.
I will fix, but please note that this patch set is more or less RFC,
and that the primary aim is to see if Wolfgang agrees with basic ideas
and the approach here.
> Patch 16 is not based on origin/master and cannot be applied.
Currently, the patch is based on v2019.07.
> I found no adjustments to test/env/*. How will your changes be tested?
Future work, but "ut env" and "bootefi selftest" have been passed in
relevant test cases.
Please note, again, that even without my patch, the current "ut env" is
far from perfect in terms of coverage of available features.
Thanks,
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> >
> >AKASHI Takahiro (16):
> > hashtable: extend interfaces to handle entries with context
> > env: extend interfaces to import/export U-Boot environment per context
> > env: extend interfaces to label variable with context
> > env: flash: add U-Boot environment context support
> > env: fat: add U-Boot environment context support
> > env: add variable storage attribute support
> > env: add/expose attribute helper functions for hashtable
> > hashtable: import/export entries with flags
> > hashtable: extend hdelete_ext for autosave
> > env: save non-volatile variables only
> > env: save a context immediately if 'autosave' variable is changed
> > env: extend interfaces to get/set attributes
> > cmd: env: show variable storage attribute in "env flags" command
> > env: fat: support UEFI context
> > env,efi_loader: define flags for well-known global UEFI variables
> > efi_loader: variable: rework with new extended env interfaces
> >
> > cmd/nvedit.c | 186 ++++++++++++++++++++++--------
> > env/Kconfig | 42 ++++++-
> > env/common.c | 82 ++++++++++---
> > env/env.c | 92 +++++++++++----
> > env/fat.c | 112 +++++++++++++++---
> > env/flags.c | 149 +++++++++++++++++++++++-
> > env/flash.c | 28 +++--
> > include/asm-generic/global_data.h | 7 +-
> > include/efi_loader.h | 1 +
> > include/env_default.h | 1 +
> > include/env_flags.h | 63 +++++++++-
> > include/environment.h | 89 ++++++++++++--
> > include/exports.h | 4 +
> > include/search.h | 16 +++
> > lib/efi_loader/Kconfig | 12 ++
> > lib/efi_loader/Makefile | 2 +-
> > lib/efi_loader/efi_setup.c | 5 +
> > lib/efi_loader/efi_variable.c | 50 ++++++--
> > lib/hashtable.c | 143 ++++++++++++++++++++---
> > 19 files changed, 925 insertions(+), 159 deletions(-)
> >
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support
2019-07-17 8:25 [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support AKASHI Takahiro
` (16 preceding siblings ...)
2019-07-17 19:05 ` [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support Heinrich Schuchardt
@ 2019-07-19 6:50 ` Wolfgang Denk
2019-07-19 7:36 ` AKASHI Takahiro
17 siblings, 1 reply; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 6:50 UTC (permalink / raw)
To: u-boot
Dear Takahiro,
In message <20190717082525.891-1-takahiro.akashi@linaro.org> you wrote:
> # In version 4 of this patch set, the implementation is changed to meet
> # Wolfgang's requirements.
Thanks.
> * Each variable may have a third attribute, "variable storage."
> There are three new flags defined:
> ('flags' are a bit-wise representation of attributes.)
>
> ENV_FLAGS_VARSTORAGE_VOLATILE: A variable only exists during runtime.
> ENV_FLAGS_VARSTORAGE_NON_VOLATILE: A variable is persistent, but explicit
> "load" and "save" command is required.
> ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE: A variable is persistent,
> any change will be written back to storage immediately.
Urgh... ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE is way too long,
and unnecessary complicated. And we don't need 3 new flags, as ENV_FLAGS_VARSTORAGE_NON_VOLATILE
is the default anyway.
Please just define
ENV_FLAGS_VOLATILE
and
ENV_FLAGS_AUTOSAVE
> * Following those extensions, new interfaces are introduced:
> for hashtable,
> hsearch_r -> hsearch_ext
> hmatch_r -> hmatch_ext
> hdelete_r -> hdelete_ext
> himport_r -> himport_ext
> hexport_r -> hexport_ext
> for env,
> env_save -> env_save_ext
> env_load -> env_load_ext
> env_import -> env_import_ext
> env_export -> env_export_ext
Why do we need all these _ext names? Please don't. Just give the
newly added functions new names.
> * There is one thing visible to users; *Exported* variables now have
> attribute information, which is appended to variable's name
> in a format of ":xxx"
> For example,
> => printenv
> arch:san=arm
> baudrate:san=115200
> board:san=qemu-arm
> ...
> This information is necessary to preserve attributes across reboot
> (save/load).
NAK. ':' is a legal character in a variable name, so you cannot use
it here for new purposes. For example:
=> setenv baudrate:san foo
=> printenv baudrate:san
baudrate:san=foo
> * Each environment storage driver must be modified so as to be aware
> of contexts. (See flash and fat in my patch set.)
I dislike this. Can we not do all the different handling in a
central place, and basically leave the storage handlers unchanged?
They do not need to know about contexts; they just do the I/O.
> Known issues/restriction/TODO:
> * Existing interfaces may be marked obsolute or deleted in the future.
Which "existing interfaces" are you talking about?
> * ENV_IS_NOWHERE doesn't work if we want to disable U-Boot environment,
> but still want to enable UEFI (hence UEFI variables).
>
> -> One solution would be to add *intermediate* configurations
> to set ENV_IS_NOWHERE properly in such a case.
Please explain what you have in ind here. What I'm guessing from
this short description sounds not so good to me, but you may have
better ideas...
> * Any context won't have VARSTORAGE_NON_VOLATILE and
> VARSTORAGE_NON_VOLATILE_AUTOSAVE entries at the same time. This is
> because it will make "save" operation ugly and unnecessarily complicated
> as we discussed in ML.
NAK. Both the VOLATILE and the AUTOSAVE properties are interesting
features, and should be available to normale U-Boot environment
variables. So the U-Boot context must support all variantes of
"normal" (persistent, non-autosave), volatile and autosave variables
at the same time. If you don't need this for UEFI, ok. But we want
to have this for U-Boot.
> -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
> to a context itself.
No. This is NOT what we need.
> * env_load() may lose some of compatibility; It now always imports variables
> *without* clearing existing entries (i.e. H_NOCLEAR specified) in order
> to support multiple contexts simultaneously.
NAK. This must ne fixed.
> * Unfortunately, some of existing U-Boot variables seem to be
> "volatile" in nature. For example, we will see errors at boot time:
> ...
> Loading Environment from Flash... OK
> ## Error inserting "fdtcontroladdr" variable, errno=22
> In: pl011 at 9000000
> Out: pl011 at 9000000
> Err: pl011 at 9000000
> ## Error inserting "stdin" variable, errno=22
> ## Error inserting "stdout" variable, errno=22
> ## Error inserting "stderr" variable, errno=22
>
> -> We will have to changes their attributes one-by-one.
> (Those can also be set through ENV_FLAGS_LIST_STATIC in env_flags.h).
I think your approach to fix this is wrong. The problem should not
happen in the first place. And no, stdin/stdout/stderr are _not_
volatile.
> * As described above, attribute information is visible to users.
> I'm afraid that it might lead to any incompatibility somewhere.
> (I don't notice any issues though.)
Correct, the current code is incompatible. This must be solved
differently. Colon is a legal character in a variable name.
> * The whole area of storage will be saved at *every* update of
> one UEFI variable. It should be optimized if possible.
This is only true for UEFI storage, right?
> You can also define a non-volatile variable from command interface:
> => setenv -e -nv FOO baa
This makes no sense. Being non-volatile is the default. This must
remain as is. Only "volatile" and "autosave" are new attributes.
More comments with the patches...
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It usually takes more than three weeks to prepare a good impromptu
speech. - Mark Twain
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support
2019-07-19 6:50 ` Wolfgang Denk
@ 2019-07-19 7:36 ` AKASHI Takahiro
2019-07-19 9:41 ` Wolfgang Denk
0 siblings, 1 reply; 48+ messages in thread
From: AKASHI Takahiro @ 2019-07-19 7:36 UTC (permalink / raw)
To: u-boot
On Fri, Jul 19, 2019 at 08:50:18AM +0200, Wolfgang Denk wrote:
> Dear Takahiro,
>
> In message <20190717082525.891-1-takahiro.akashi@linaro.org> you wrote:
> > # In version 4 of this patch set, the implementation is changed to meet
> > # Wolfgang's requirements.
>
> Thanks.
>
> > * Each variable may have a third attribute, "variable storage."
> > There are three new flags defined:
> > ('flags' are a bit-wise representation of attributes.)
> >
> > ENV_FLAGS_VARSTORAGE_VOLATILE: A variable only exists during runtime.
> > ENV_FLAGS_VARSTORAGE_NON_VOLATILE: A variable is persistent, but explicit
> > "load" and "save" command is required.
> > ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE: A variable is persistent,
> > any change will be written back to storage immediately.
>
> Urgh... ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE is way too long,
> and unnecessary complicated.
But this rule *does* conform with other flags' naming rule.
Aren't other name, like
env_flags_varaccess_changedefault in env_flags_varaccess, or
ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR
long?
What is the upper limit that you think?
> And we don't need 3 new flags, as ENV_FLAGS_VARSTORAGE_NON_VOLATILE
> is the default anyway.
"default" may be different on different contexts.
It cannot be the reason.
> Please just define
>
> ENV_FLAGS_VOLATILE
> and
> ENV_FLAGS_AUTOSAVE
Disagree.
>
> > * Following those extensions, new interfaces are introduced:
> > for hashtable,
> > hsearch_r -> hsearch_ext
> > hmatch_r -> hmatch_ext
> > hdelete_r -> hdelete_ext
> > himport_r -> himport_ext
> > hexport_r -> hexport_ext
> > for env,
> > env_save -> env_save_ext
> > env_load -> env_load_ext
> > env_import -> env_import_ext
> > env_export -> env_export_ext
>
> Why do we need all these _ext names? Please don't. Just give the
> newly added functions new names.
You seem to misunderstand my patch. Please read my code first.
I have not renamed the existing interfaces, which still remain
for compatibility, and added new functions with "extended" names.
> > * There is one thing visible to users; *Exported* variables now have
> > attribute information, which is appended to variable's name
> > in a format of ":xxx"
> > For example,
> > => printenv
> > arch:san=arm
> > baudrate:san=115200
> > board:san=qemu-arm
> > ...
> > This information is necessary to preserve attributes across reboot
> > (save/load).
>
> NAK. ':' is a legal character in a variable name, so you cannot use
> it here for new purposes. For example:
What is your recommendation?
I didn't find what are the definition of "illegal" characters.
> => setenv baudrate:san foo
> => printenv baudrate:san
> baudrate:san=foo
>
> > * Each environment storage driver must be modified so as to be aware
> > of contexts. (See flash and fat in my patch set.)
>
> I dislike this. Can we not do all the different handling in a
> central place, and basically leave the storage handlers unchanged?
> They do not need to know about contexts; they just do the I/O.
How? I don't get your point.
Did you read patch (#4 and) #5 and #14?
Context-specific code are quite isolated and yet we need to modify
drivers a bit.
> > Known issues/restriction/TODO:
> > * Existing interfaces may be marked obsolute or deleted in the future.
>
> Which "existing interfaces" are you talking about?
h*_r(), env_import/export(), env_save/load() and etc.
Once all the occurrences in the repository have been udpated
using new interfaces, we will be able to delete them, won't you?
> > * ENV_IS_NOWHERE doesn't work if we want to disable U-Boot environment,
> > but still want to enable UEFI (hence UEFI variables).
> >
> > -> One solution would be to add *intermediate* configurations
> > to set ENV_IS_NOWHERE properly in such a case.
>
> Please explain what you have in ind here. What I'm guessing from
> this short description sounds not so good to me, but you may have
> better ideas...
For example,
if we want to use FAT for UEFI variables, but don't want to enable
U-Boot envrionment for some reason (don't ask me why now),
we cannot simply enable ENV_IS_NOWHERE, then U-Boot envrionment
won't work as expected in some places.
>
> > * Any context won't have VARSTORAGE_NON_VOLATILE and
> > VARSTORAGE_NON_VOLATILE_AUTOSAVE entries at the same time. This is
> > because it will make "save" operation ugly and unnecessarily complicated
> > as we discussed in ML.
>
> NAK. Both the VOLATILE and the AUTOSAVE properties are interesting
> features, and should be available to normale U-Boot environment
> variables. So the U-Boot context must support all variantes of
> "normal" (persistent, non-autosave), volatile and autosave variables
> at the same time. If you don't need this for UEFI, ok. But we want
> to have this for U-Boot.
NAK. Do you remember our discussion in the past?
Allowing such a feature may end up with ugly and complicated
implementation. I believe that we agreed on this point before.
>
> > -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
> > to a context itself.
>
> No. This is NOT what we need.
Why?
> > * env_load() may lose some of compatibility; It now always imports variables
> > *without* clearing existing entries (i.e. H_NOCLEAR specified) in order
> > to support multiple contexts simultaneously.
>
> NAK. This must ne fixed.
NAK.
This is a natural result of supporting multiple contexts
in U-Boot environment (plus UEFI variables in one hash table.)
> > * Unfortunately, some of existing U-Boot variables seem to be
> > "volatile" in nature. For example, we will see errors at boot time:
> > ...
> > Loading Environment from Flash... OK
> > ## Error inserting "fdtcontroladdr" variable, errno=22
> > In: pl011 at 9000000
> > Out: pl011 at 9000000
> > Err: pl011 at 9000000
> > ## Error inserting "stdin" variable, errno=22
> > ## Error inserting "stdout" variable, errno=22
> > ## Error inserting "stderr" variable, errno=22
> >
> > -> We will have to changes their attributes one-by-one.
> > (Those can also be set through ENV_FLAGS_LIST_STATIC in env_flags.h).
>
> I think your approach to fix this is wrong. The problem should not
> happen in the first place. And no, stdin/stdout/stderr are _not_
> volatile.
I can't understand why we need to save stdin/stdout/stderr as
non-volatile variables. It should be initialized at every boot.
> > * As described above, attribute information is visible to users.
> > I'm afraid that it might lead to any incompatibility somewhere.
> > (I don't notice any issues though.)
>
> Correct, the current code is incompatible. This must be solved
> differently. Colon is a legal character in a variable name.
How?
You denied, in the past discussions in ML, that key or value be
'structured' as an opaque object.
>
> > * The whole area of storage will be saved at *every* update of
> > one UEFI variable. It should be optimized if possible.
>
> This is only true for UEFI storage, right?
Yes, so?
> > You can also define a non-volatile variable from command interface:
> > => setenv -e -nv FOO baa
>
> This makes no sense. Being non-volatile is the default. This must
> remain as is. Only "volatile" and "autosave" are new attributes.
'-nv' option is already merged in the upstream, and
it is only valid for UEFI variables.
And again, there is no *default* attribute for UEFI variables.
Thanks you for your comments,
-Takahiro Akashi
> More comments with the patches...
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> It usually takes more than three weeks to prepare a good impromptu
> speech. - Mark Twain
^ permalink raw reply [flat|nested] 48+ messages in thread* [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support
2019-07-19 7:36 ` AKASHI Takahiro
@ 2019-07-19 9:41 ` Wolfgang Denk
0 siblings, 0 replies; 48+ messages in thread
From: Wolfgang Denk @ 2019-07-19 9:41 UTC (permalink / raw)
To: u-boot
Dear AKASHI Takahiro,
In message <20190719073606.GP21948@linaro.org> you wrote:
>
> > Urgh... ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE is way too long,
> > and unnecessary complicated.
>
> But this rule *does* conform with other flags' naming rule.
> Aren't other name, like
> env_flags_varaccess_changedefault in env_flags_varaccess, or
> ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR
Yes, you are right. I missed to review these patches in time.
It would have been much niver if these flags had simple, speaking
names like
ENV_FLAG_READONLY
ENV_FLAG_WRITEONCE
ENV_FLAG_CHANGEDEFAULT
> long?
> What is the upper limit that you think?
> > Please just define
> >
> > ENV_FLAGS_VOLATILE
> > and
> > ENV_FLAGS_AUTOSAVE
>
> Disagree.
Please explain why?
This is all that's needed. There is no more magic behind.
It makes no sense to invent ariticial "groups" like "access" or
"storage" when it just adds tons of new functions and many, many
lines of code.
> > Why do we need all these _ext names? Please don't. Just give the
> > newly added functions new names.
>
> You seem to misunderstand my patch. Please read my code first.
> I have not renamed the existing interfaces, which still remain
> for compatibility, and added new functions with "extended" names.
I have read your code. We don't want to have such aton of
duplicated funtions. use the existing ones, and add context
handling.
> > > => printenv
> > > arch:san=arm
> > > baudrate:san=115200
> > > board:san=qemu-arm
> > > ...
> > > This information is necessary to preserve attributes across reboot
> > > (save/load).
> >
> > NAK. ':' is a legal character in a variable name, so you cannot use
> > it here for new purposes. For example:
>
> What is your recommendation?
As is, flags are stored in the ".flags" variable. The reason is
that there was no backward compatible way to add such a property.
> I didn't find what are the definition of "illegal" characters.
The only characters which cannot be used in a variable name is '='
(which is the separator character in external storrage format, and
'\0', which is the terminator.
You cannot encode wadditional information in the external storage,
except by defining new special variables - here, we use ".flags".
> > > * Each environment storage driver must be modified so as to be aware
> > > of contexts. (See flash and fat in my patch set.)
> >
> > I dislike this. Can we not do all the different handling in a
> > central place, and basically leave the storage handlers unchanged?
> > They do not need to know about contexts; they just do the I/O.
>
> How? I don't get your point.
> Did you read patch (#4 and) #5 and #14?
> Context-specific code are quite isolated and yet we need to modify
> drivers a bit.
Yes, I have read this. I find the implentation not elegant. We
should keep such details out of the handler code to the extent
possible. The data / parameters should prepared in a central place,
and just passed to the drivers. It makes no sense to add "context"
knowhow to the drivers - these should just do I/O and not know about
such details.
> > > * Existing interfaces may be marked obsolute or deleted in the future.
> >
> > Which "existing interfaces" are you talking about?
>
> h*_r(), env_import/export(), env_save/load() and etc.
> Once all the occurrences in the repository have been udpated
> using new interfaces, we will be able to delete them, won't you?
No. We should just change the existing interfaces to support
contexts, that's all. not defining new ones (with longer names) and
then switching.
> > > -> One solution would be to add *intermediate* configurations
> > > to set ENV_IS_NOWHERE properly in such a case.
> >
> > Please explain what you have in ind here. What I'm guessing from
> > this short description sounds not so good to me, but you may have
> > better ideas...
>
> For example,
> if we want to use FAT for UEFI variables, but don't want to enable
> U-Boot envrionment for some reason (don't ask me why now),
> we cannot simply enable ENV_IS_NOWHERE, then U-Boot envrionment
> won't work as expected in some places.
ENV_IS_NOWHERE means that there is no storage defined for the U-Boot
environment. When implementing contexts properly, this becomes a
property of the U-Boot context only, while other contexts can have
other storage devices defined.
Yes, this needs fixing in a few places which assume that ther eis
only a single context - so far.
> > NAK. Both the VOLATILE and the AUTOSAVE properties are interesting
> > features, and should be available to normale U-Boot environment
> > variables. So the U-Boot context must support all variantes of
> > "normal" (persistent, non-autosave), volatile and autosave variables
> > at the same time. If you don't need this for UEFI, ok. But we want
> > to have this for U-Boot.
>
> NAK. Do you remember our discussion in the past?
> Allowing such a feature may end up with ugly and complicated
> implementation. I believe that we agreed on this point before.
No we did not. And there is absolutely no complicated
implementation needed. All ne need is two new flags, and code to
test these flags in env_set() (for ENV_FLAGS_AUTOSAVE) and in
env_save() (for ENV_FLAGS_VOLATILE).
> > > -> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed
> > > to a context itself.
> >
> > No. This is NOT what we need.
>
> Why?
Because it is a property of individual variables. We do not have a
concept for context-wide flags, andif you want to define one, it
must not mess with the existing implementation of variable-specific
flags.
> > > * env_load() may lose some of compatibility; It now always imports variables
> > > *without* clearing existing entries (i.e. H_NOCLEAR specified) in order
> > > to support multiple contexts simultaneously.
> >
> > NAK. This must ne fixed.
>
> NAK.
> This is a natural result of supporting multiple contexts
> in U-Boot environment (plus UEFI variables in one hash table.)
"Natural result"? I don't think so. It's a problem of your
implementation, and must be fixed, as it introduces incompatible
changes in behaviour. I won't accept that.
> I can't understand why we need to save stdin/stdout/stderr as
> non-volatile variables. It should be initialized at every boot.
No. You may, for example, want to permanently set std* to
alternative devices, say to netconsole. this only works if the
variables are saved, as is the case now.
Again, this must not be changed as it would break
backward-compatibility.
[Also I cannot see a problem with these specific variables. Why do
you see such errors?]
> > Correct, the current code is incompatible. This must be solved
> > differently. Colon is a legal character in a variable name.
>
> How?
> You denied, in the past discussions in ML, that key or value be
> 'structured' as an opaque object.
See above. The definition of the external storage format does not
allow any such additions. Yes, this is very unfortunate, but that
code was written 19 years ago and such a need was just not foreseen.
As is, we have the ".flags" variable. We can extend this, and
eventually define similar things like ".context[U-Boot]",
".context[UEFI}" etc...
> > > * The whole area of storage will be saved at *every* update of
> > > one UEFI variable. It should be optimized if possible.
> >
> > This is only true for UEFI storage, right?
>
> Yes, so?
Just to make sure I undernd your intentions / code...
> > This makes no sense. Being non-volatile is the default. This must
> > remain as is. Only "volatile" and "autosave" are new attributes.
>
> '-nv' option is already merged in the upstream, and
> it is only valid for UEFI variables.
Well, I guess we can get rid of this in the context of this patch
series.
> And again, there is no *default* attribute for UEFI variables.
You see, a variable can be _either_ volatile or _not_. So a single
flasg is all that's needed. U-Boot has non-volatile as default, so
it would make sense to adapt this notation for the UEFI
implementation in U-Boot as well. Then all contexts will just mark
volatile vars by setting the corresponding flag. This does not
change behaviour for UEFI. Yes, it changes the interface, but that
is probably easy, as you say there is no default.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There is wisdom in turning as often as possible from the familiar to
the unfamiliar: it keeps the mind nimble, it kills prejudice, and it
fosters humor. - George Santayana
^ permalink raw reply [flat|nested] 48+ messages in thread