From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 C4B412034C5F4 for ; Thu, 23 Nov 2017 14:56:03 -0800 (PST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Nov 2017 15:00:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,443,1505804400"; d="scan'208,217";a="6306317" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga001.fm.intel.com with ESMTP; 23 Nov 2017 15:00:20 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 23 Nov 2017 15:00:20 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 23 Nov 2017 15:00:19 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.152]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.159]) with mapi id 14.03.0319.002; Fri, 24 Nov 2017 07:00:18 +0800 From: "Ni, Ruiyu" To: Laszlo Ersek CC: edk2-devel-01 , Ard Biesheuvel , "Justen, Jordan L" , "Dong, Eric" , "Zeng, Star" , "Doran, Mark" Thread-Topic: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code Thread-Index: AQHTY+3iRUVHeCXtuU+IX8hF+88Z1qMhUt7ggAAmvYCAAJCCIP//o18AgADoUGE= Date: Thu, 23 Nov 2017 23:00:17 +0000 Message-ID: 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>, <4f73213f-ea25-a21a-c2b7-644152e5aefc@redhat.com> In-Reply-To: <4f73213f-ea25-a21a-c2b7-644152e5aefc@redhat.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 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 22:56:04 -0000 Content-Language: zh-CN Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable Sent from a small-screen device =1B$B:_=1B(B 2017=1B$BG/=1B(B11=1B$B7n=1B(B24=1B$BF|!$>e8a=1B(B1:08=1B$B!$= =1B(BLaszlo Ersek > =1B$B>; edk2-devel-0= 1 > Cc: Ard Biesheuvel >; Justen, Jordan L >; Dong, Eric <= eric.dong@intel.com>; 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 writ= es.) 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, no= t synchronous. Handlers are registered at various task priority levels, and i= f the reporting occurs at the same or higher TPL, then the delivery will be delay= ed. (According to the PI spec, it is even possible to report several status cod= es 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 statu= s 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; f= or 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 referen= ces 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 handl= er 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.) TPL_HIGH is just an indicator to tell status router to call the handler in = blocking or synchronized way. So handler actually works in the status code = reporter=1B$B!G=1B(Bs TPL, which is TPL_APPLICATION. 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 =3D gBS->LoadImage ( TRUE, gImageHandle, FilePath, FileBuffer, FileSize, &ImageHandle ); } if (FileBuffer !=3D NULL) { FreePool (FileBuffer); } if (FilePath !=3D NULL) { FreePool (FilePath); } if (EFI_ERROR (Status)) { // // Report Status Code to indicate that the failure to load boot optio= n // 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, i= t 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 statu= s 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 tha= t; 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 confi= dential - -, but if you look up the paragraph starting with "Best practice", Mark bas= ically 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 invite= d 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, an= d push *live* for the change that you want, you have no chance at getting stu= ff into the specs. I mean, in his email (b), Mark suggests asking a "proxy" for representing t= he 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 understan= d the problem space, and you maintain the corresponding reference code in edk= 2. 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 prac= tice, if they like it. Once it is wide-spread practice, it can be more easily cod= ified 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 h= as 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