Re: [PATCH 3/5] MdeModulePkg/UefiBootManagerLib: report EDKII_OS_LOADER_DETAIL status code

[Laszlo Ersek <lersek@redhat.com>]

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 <ruiyu.ni@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Doran, Mark <mark.doran@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