From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Mon, 28 Oct 2019 10:14:36 +0900 Subject: [U-Boot] [PATCH v5 00/19] efi_loader: non-volatile variables support In-Reply-To: <20191025132523.A8CA7240064@gemini.denx.de> References: <20190905082133.18996-1-takahiro.akashi@linaro.org> <20191001062829.GA18778@linaro.org> <20191023065332.GE10448@linaro.org> <20191025070644.4F34D2400D6@gemini.denx.de> <20191025075645.GJ10448@linaro.org> <20191025132523.A8CA7240064@gemini.denx.de> Message-ID: <20191028011435.GP10448@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 Wolfgang, On Fri, Oct 25, 2019 at 03:25:23PM +0200, Wolfgang Denk wrote: > Dear Takahiro, > > In message <20191025075645.GJ10448@linaro.org> you wrote: > > > > I won't and cannot make replies on every comment that you gave me > > below because we are very different at some basic points and > > other comments are just details. > > Can you please at least comment on the size impact? How much does > the code size grow on everage, especially for SPL? Well, from the next version as I will make a drastic change as far as I follow your suggestion below. > > > So first I wanted to know if you agree to my *basic* approach or not, > > not details, in order to go further, but still don't see > > yes or no below. > > To be honest: when I saw your monster patch series which basically > touches every piece of code in U-Boot, I felt a strong temptation to > just send a NAK and be done with it. But I know this would not be > fair. But to be able to say yes I would need days to review the > code and probably run some tests myself, and I don;t have that time. Okay, I see but I hope that you should have said so much earlier. My patch (v5) has been stranded almost two months without any comments. > So I can neither say yes or no, sorry. > > > > My biggest concern is that such a highly invasive change cannot be > > > simply rubberstamped in a code review - I think this also needs > > > runtime testing on at least a significant number of the affected > > > boards. We should try to get help from at least some board > > > maintainers - maybe you should ask for help for such testing n the > > > board maintainers mailing list? > > This is a point which is important to me. We need at least a few > "Tested-by" credits... > > > > > > > * Non-volatile feature is not implemented in a general form and must be > > > > > > implemented by users in their sub-systems. > > > > > > I don't understand what this means, or why such a decision was made. > > > Which sub-systems do you have in mind here? > > > > UEFI sub-system/library. > > What needs to be done to have this - say - for U-Boot context? > > > > What prevented you from implementing a solution to works for all of > > > us? > > ? At least for me or UEFI, nothing more, other than a *context*, is needed to meet the requirement. If some one really want to have such(?) a feature, he or she can on top of my patch. I believe that my attitude here is fair enough. > > > > > > > Known issues/restriction/TODO: > > > > > > * The current form of patchset is not 'bisect'able. > > > > > > Not to break 'bisect,' all the patches in this patch set must be > > > > > > put into a single commit when merging. > > > > > > (This can be mitigated by modifying/splitting Patch#18/#19 though.) > > > > > > OK, so you are aware of this problem. > > > > > > I must admit that I really hate this. If you could avoid all the API > > > changes, this would solve this problem, wouldn't it? > > > > "Avoid all the API changes" is an approach that I took in all my > > previous versions, but you *denied* it. > > > > That is: I proposed an approach in which the existing interfaces, > > env_get/set(), were maintained for existing users/sub-systems. > > Only new users who want to enjoy merits from new "context" feature may > > use new *extended* interfaces, env_[get|set]_ext(), in my case UEFI. > > As you *denied* it, this version (v5) is an inevitable result. > > > > Don't take me wrong, but I think that you made inconsistent comments. > > I think you misunderstand. If we just need the same pointer in all > functions dealing with the environment, there are at least two ways > to implement this: we can add it as an argument to each and every > function call; this will blow up code size and also impact execution > speed. Or we can add it to some (private or public) data structure > that is visible everywhere. In the simplest case we could add such > a pointer to the global data (GD) structure as I suggested in my > previous mail. this would allow you to use basically the same code > as now, but without needing to change all the argument lists. In > the result, you could drop your modifications of all common and > board specififc files. The code changes would be concentrated on > the environment code, and it should be anle to submit bisectable > patches again. > > Yes, global variables have disadvantages, too, but does it not make > sense here? I think we will have only one active environment > context at any time in U-Boot, so this seems to be at least a lesser > evil than the zillion of changes of all call arguments. I have thought of an idea as you mentioned above before but immediately gave it up as it seems to be an ugly implementation and I simply dislike it. But if you and Tom (or whoever can say ACK to my patch) think that it is the *only* solution that you can accept, I will have to change my mind. So do you always admit the following coding? ===8<=== (in some UEFI function) ... old_ctx = env_set_context(ctx_uefi); env_set(var, value); env_set_context(old_ctx); ... ===>8=== > > > > > > > * Unfortunately, this code fails to read U-Boot environment from flash > > > > > > at boot time due to incomprehensible memory corruption. > > > > > > See murky workaround, which is marked as FIXME, in env/flash.c. > > > > > > Argh. This is a killing point, isn't it? > > > > > > You don't seriously expect to have patches which cause > > > "incomprehensible memory corruption" to be included into mainline? > > > > It will be just a matter of time for debugging. > > It might be difficult to find willing testers under such > circumstances. I would not want to run code with serious known > bugs. I believe that the issue is independent from the basic approach that we are now discussing. > > > > > > * An error during "save" operation may cause inconsistency between > > > > > > cache (hash table) and the storage. > > > > > > -> This is not UEFI specific though. > > > > > > Is this a new problem, or do you just mention this here for > > > completeness? We always had this issue, didn't we? > > > > As I said, "this is not UEFI specific." > > This does not answer my question. Are you just refering to the > general problem that a write to the persistent storage area might > fail, which has ever been present and which is inherently > unfixxable, or does your new code add any additional problems of > this kind? A general problem in your term. > > > How much testing can be done on boards that don't use FAT to store > > > the environment? > > > > As I said, > > > > > > In this version, only FAT file system and flash devices are supported, > > > > > > but it is quite straightforward to modify other drivers. > > > > If you don't think my patch is not qualified for a "PATCH" for some reason, > > I will sent one as "RFC" from the next version. I don't care. > > I think this is a new feature that needs to be tested. You focus on > UEFI + FAT and I don't know if UEFI + non-FAT makes sense, but at > least (1) non-UEFI + FAT and (2) non-UEFI + non-FAT are > configurations which must be tested - absolute minimum is at least > one example implementation. My suggestion would be to use something > that is not file system based, but using plain storage, say a board > which has the environment in SPI NOR flash. What do you mean by "non-UEFI"? Disabling UEFI, or testing U-Boot environment variables on no-FAT device? If the latter, I have tested it with flash (on qemu). Thanks, -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 > Our OS who art in CPU, UNIX be thy name. > Thy programs run, thy syscalls done, > In kernel as it is in user!