On 11/23/17 15:53, Ni, Ruiyu wrote:
> Comments below.
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Thursday, November 23, 2017 10:03 PM
>> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Justen, Jordan L
>> <jordan.l.jus...@intel.com>; Dong, Eric <eric.d...@intel.com>; Zeng, Star
>> <star.z...@intel.com>; Doran, Mark <mark.do...@intel.com>
>> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
