From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Denk Date: Tue, 29 Oct 2019 14:28:19 +0100 Subject: [U-Boot] [PATCH v5 00/19] efi_loader: non-volatile variables support In-Reply-To: <20191028011435.GP10448@linaro.org> 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> <20191028011435.GP10448@linaro.org> Message-ID: <20191029132819.35759240034@gemini.denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Dear Takahiro, In message <20191028011435.GP10448@linaro.org> you wrote: > > > 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. OK. > 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. I have said so uin lcear and unmistakable words when discussion was to add me as maintainer for environment code. I cannot commit to such a position as I don't have enough time available. > > > > > > > * 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. I would appreciate if you could actually answer my questions. there are two of them: You write: "Non-volatile feature is not implemented in a general form and must be implemented by users in their sub-systems." Q1: Why did you not implement this feature in a generic way, so it is automatically available to all sub-systems that support contexts? Q2: What exactly needs to be done, which code needs to be writtenm, to implement this feature - say - for U-Boot context? > 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=== This is even worse, as instead of adding a single argument to a function call you are adding two full fucntion calls. But why would that be needed? U-Boot is strictly single tasking. You always know what the current context is, and nobody will change it behind your back. And UEFI functions are not being called in random places, they are limited to a small set of UEFI commands, right? So why do you not just enter UEFI context when you start executing UEFI code, and restore the rpevious state when returning to non-UEFI U-Boot? Or am I missing something? > > > > 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. The reason of this problem must be understood and the bug fixed before it can go into mainline. > > 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). We need to test if the new context code works (and does not cause problems) on systems where UEFI is not enabled. 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 Don't you know anything? I should have thought anyone knows that who knows anything about anything... - Terry Pratchett, _Soul Music_