From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1AA29220C1C2A for ; Thu, 23 Nov 2017 09:04:34 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8CAFD624DF; Thu, 23 Nov 2017 17:08:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-221.rdu2.redhat.com [10.10.120.221]) by smtp.corp.redhat.com (Postfix) with ESMTP id BD896620A2; Thu, 23 Nov 2017 17:08:49 +0000 (UTC) To: "Ni, Ruiyu" , edk2-devel-01 Cc: Ard Biesheuvel , "Justen, Jordan L" , "Dong, Eric" , "Zeng, Star" , "Doran, Mark" References: <20171122235849.4177-1-lersek@redhat.com> <20171122235849.4177-4-lersek@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BACE29B@SHSMSX104.ccr.corp.intel.com> <8b8b9430-f117-a9fb-a80c-cb3d034025ec@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BACEB2A@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <4f73213f-ea25-a21a-c2b7-644152e5aefc@redhat.com> Date: Thu, 23 Nov 2017 18:08:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BACEB2A@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 23 Nov 2017 17:08:51 +0000 (UTC) Subject: Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Nov 2017 17:04:35 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/23/17 15:53, Ni, Ruiyu wrote: > Comments below. > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, November 23, 2017 10:03 PM >> To: Ni, Ruiyu ; edk2-devel-01 >> Cc: Ard Biesheuvel ; Justen, Jordan L >> ; Dong, Eric ; Zeng, Star >> ; Doran, Mark >> Subject: Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report >> EDKII_OS_LOADER_DETAIL status code >> >> Hello Ray, >> >> (CC Mark Doran for the last part of my email) >> >> On 11/23/17 06:21, Ni, Ruiyu wrote: >>> Laszlo, >>> When booting a boot option, UefiBootManagerBoot() sets a Boot#### >>> variable and saves #### to BootCurrent variable. >>> So all the details (except the EDKII_OS_LOADER_DETAIL.Status) can be >>> retrieved from reading Boot#### variable. >> >> I have two counter-arguments: >> >> (1) I would like to minimize the number of accesses to non-volatile UEFI >> variables. Dependent on the virtualization platform and the (virtual) flash chip >> structure (for example, flash block size), flash access can be very slow. >> >> I'm not 100% sure whether this performance problem affects flash *reads* as >> well, or if it affects flash writes only. (It definitely affects flash writes.) > > NV *read* should have no performance impact since EDKII variable driver's > implementation caches the flash in memory. I am not sure about that > in OVMF. Yeah it should work the same. Thanks! > But I think a good implementation of variable driver should > consider such *read* optimization. > Performance needs to be considered, but simpler/easy-maintain > code takes higher priority. > >> >> >> (2) The delivery of status codes to registered handlers is asynchronous, not >> synchronous. Handlers are registered at various task priority levels, and if the >> reporting occurs at the same or higher TPL, then the delivery will be delayed. >> (According to the PI spec, it is even possible to report several status codes >> before the first will be delivered -- basically the codes are queued until the TPL is >> reduced sufficiently.) This is why all data has to be embedded in the status code >> structure itself. >> >> Like in any other notify function's case, it is prudent to register status code >> handlers at the least strict TPL that can work for the actual processing. (The >> internals of the handler function can even put an upper limit on the TPL; for >> example using Simple Text Output prevents us from going higher than >> TPL_NOTIFY.) For this reason I chose TPL_CALLBACK for the OVMF status code >> handler. >> >> Now, if we can *guarantee* that these status codes are only reported at the >> TPL_APPLICATION level, then a TPL_CALLBACK status code handler will be >> synchronous in effect, and then the status code structure can carry references >> to other (external) things in the system too, such as UEFI variables. >> >> Can we guarantee that EfiBootManagerBoot() is never invoked above >> TPL_APPLICATION? > Yes. It should be. Because gBS->LoadImage()/StartImage() per UEFI spec TPL > restriction rule, should be run at TPL_APPLICATION. > And if there is no special reason (handler runs too slow), the status handler > is registered at TPL_HIGH so that the handler is called synchronously. OK, the TPL_APPLICATION promise will work for me. I missed the LoadImage / StartImage implications; good point! (I can't register the handler at TPL_HIGH, because the handler calls AsciiPrint, which internally uses gST->ConOut, and that (i.e., SimpleTextOut) cannot be used above TPL_NOTIFY. But the TPL_CALLBACK handler will work fine with the TPL_APPLICATION report.) > >> >>> >>> 4 more comments below. >> >> [snip] >> >>>> + BmReportOsLoaderDetail ( >>>> + OsLoaderDetail, >>>> + OsLoaderDetailSize, >>>> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD, >>>> + 0 // DetailStatus -- unused here >>>> + ); >>>> + >>> >>> 1. With the BootCurrent, this change is not necessary because others >>> can hook PcdProgressCodeOsLoaderLoad to get the current loading boot >>> option. >> >> As long as we can clear my points (1) and (2) above, I'd be fine with this >> approach. >> >>> >>>> Status = gBS->LoadImage ( >>>> TRUE, >>>> gImageHandle, >>>> FilePath, >>>> FileBuffer, >>>> FileSize, >>>> &ImageHandle >>>> ); >>>> } >>>> if (FileBuffer != NULL) { >>>> FreePool (FileBuffer); >>>> } >>>> if (FilePath != NULL) { >>>> FreePool (FilePath); >>>> } >>>> >>>> if (EFI_ERROR (Status)) { >>>> // >>>> // Report Status Code to indicate that the failure to load boot option >>>> // >>>> REPORT_STATUS_CODE ( >>>> EFI_ERROR_CODE | EFI_ERROR_MINOR, >>>> (EFI_SOFTWARE_DXE_BS_DRIVER | >>>> EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) >>>> ); >>>> + BmReportOsLoaderDetail ( >>>> + OsLoaderDetail, >>>> + OsLoaderDetailSize, >>>> + EDKII_OS_LOADER_DETAIL_TYPE_LOAD_ERROR, >>>> + Status >>>> + ); >>>> + >>> >>> 2. I think firstly, the OsLoaderDetail is not needed. >>> Secondly, I prefer to submit a PI spec change to include extended data >>> for EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR status code. >>> Instead of inventing a new status code. >> >> I understand your point, and in theory I agree with it, but in practice, I cannot. > > If there is no similar status code for a certain purpose, I agree to invent a new status > code. And when the new status code is proven by time to be mature enough, it can go > to PI spec. > But for this case, since PI spec already has such status code, I prefer to at least try > to change the spec. If PIWG doesn't agree, we can go on using the new status code. Oh I'm not worried about PIWG disagreement. Getting strong disagreement *quickly* is actually a very good outcome, because it tells us how to proceed. What I'm worried about is a drawn-out process that takes forever (and requires me to spend a disproportionate amount of time on the phone, and at bad times). Unless there is some kind of promise that I can work with the PIWG in a timely manner *without* the phone, it doesn't make sense for me to start the process. I'll ask on the PIWG list first. Thanks, Laszlo > >> >> My experience with Mantis tickets is not good: >> >> (a) First, I would have to write up the proposal. That's fine, I can do that; have >> done it before. >> >> >> (b) Second, I'd have to *call* the meetings. Please locate the email titled >> >> a reminder about MANTIS usage >> >> from Mark, (date: 6 Sep 2017), cross-posted to the PIWG/USWG/ASWG lists. >> >> I'm not allowed to quote the email verbatim -- all of these lists are confidential - >> -, but if you look up the paragraph starting with "Best practice", Mark basically >> implied, "file the the Mantis ticket, then dial in to the next meeting and sell your >> proposal to the WG *verbally*". >> >> I *cannot* do that. The WG meetings always take place early afternoon in >> Pacific (US West Coast) timezone, which is around *midnight* in my timezone. >> >> In addition, I don't even have time to attend the confcalls that I'm invited to >> within Red Hat! >> >> I don't understand why a carefully written Mantis ticket is not sufficient for >> discussion, but apparently it isn't. :/ >> >> >> (c) Case in point, please see Mantis ticket #1736, titled >> >> lacking specification of EFI_BOOT_SCRIPT_WIDTH in PI 1.5 / Vol 5 / >> 8.7.1 Save State Write >> >> which I had originally filed in December 2016. >> >> In February 2017, I was asked for more details. I said, "fair enough", and I spent >> half a day writing up the requested change in *full* detail. >> I gave editing instructions of the form >> >> change this to that >> >> and >> >> add/delete the following >> >> as recommended in Mark's email (b). >> >> You can see my write-up in note 0005044. After that note, there have been >> *zero* updates to the ticket; since February 2017. >> >> >> So the fact is, unless you have time to call the WG meetings every week, and >> push *live* for the change that you want, you have no chance at getting stuff >> into the specs. >> >> >> I mean, in his email (b), Mark suggests asking a "proxy" for representing the >> change request, to anyone that cannot dial in. Well, can *you* be my proxy on >> the PIWG meeting, if I write up the change request? You certainly understand >> the problem space, and you maintain the corresponding reference code in edk2. >> I just doubt you have time for conference calls, same as I. >> >> UefiBootManagerLib already reports an extended status code, with >> EDKII_SET_VARIABLE_STATUS payload. Introducing another edk2-specific status >> code requires no slow negotiation, and it can be adopted by others, in practice, >> if they like it. Once it is wide-spread practice, it can be more easily codified in >> the spec. The specifics for the first implementation can be -- should be -- >> discussed on this open list; we had better not standardize something that has >> zero implementations just yet. >> >> Anyway, I'm willing to go through the standardization process, but only if it >> doesn't require me to pick up the phone every week (esp. at midnight). >> >> Thanks >> Laszlo