| Message ID | 20190717082525.891-3-takahiro.akashi@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: non-volatile variables support | expand |
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
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@denx.de > panic: kernel trap (ignored)
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
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_ */
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(-)