From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Fri, 1 Nov 2019 15:04:36 +0900 Subject: [U-Boot] [PATCH v5 00/19] efi_loader: non-volatile variables support In-Reply-To: <20191029132819.35759240034@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> <20191028011435.GP10448@linaro.org> <20191029132819.35759240034@gemini.denx.de> Message-ID: <20191101060434.GV10448@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, I would like to focus on the critical and most arguable point in this email. On Tue, Oct 29, 2019 at 02:28:19PM +0100, Wolfgang Denk wrote: > 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? Can you elaborate What you mean by "entering UEFI context"? What I'm thinking of here is that we would add one more member field, which is a pointer to my "env_context," in GD and change it in env_set_context(). This can be defined even as an inline function. (My other patches, at least patch#1 to #6, can be used almost as they are.) I don't see why you deny such a simple solution with lots of flexibility. -Takahiro Akashi > 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_