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

[Laszlo Ersek <lersek@redhat.com>]

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.)


(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?

> 
> 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.

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