From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Fri, 19 Jul 2019 16:36:07 +0900 Subject: [U-Boot] [RFC, PATCH v4 00/16] efi_loader: non-volatile variables support In-Reply-To: <20190719065018.968BE240049@gemini.denx.de> References: <20190717082525.891-1-takahiro.akashi@linaro.org> <20190719065018.968BE240049@gemini.denx.de> Message-ID: <20190719073606.GP21948@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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