git: 0c839497c174 - stable/13 - loader.efi: There are systems without ConOut, also use ConOutDev

[Warner Losh]

On Fri, Feb 5, 2021 at 10:24 AM Toomas Soome <tsoome at me.com> wrote:

>
>
> On 5. Feb 2021, at 18:44, Warner Losh <imp at bsdimp.com> wrote:
>
>
>
> On Thu, Feb 4, 2021 at 11:38 PM Toomas Soome <tsoome at me.com> wrote:
>
>>
>>
>> On 5. Feb 2021, at 01:56, Warner Losh <imp at bsdimp.com> wrote:
>>
>> 
>> And why the instaMFC? Changes are supposed to cook force days before
>> merging... I have questions about the wisdom of this change...
>>
>> Warner
>>
>>
>> Reason is in PR. There is someone with the system without ConOut but
>> ConOutDev is set. Instead of falling back to arbitrary device (which in
>> this case was totally wrong choice), we can try the possible devices list.
>> We do not change the ConOut parsing.
>>
>
> We could have the same effect defaulting to Video. This bug should have
> been discussed / reviewed before it was committed.
>
>
> How is is different from defaulting to serial, it is just as bad? we can
> not guess there, thats why we do have ConOutDev list.
>
>
>
> If it would appear, there are systems with unusable devices listed in
>> ConOutDev, then we need to think how to handle such case.
>>
>
> Yes. We fall back to the arbitrary device... It's just a flag that can be
> overridden. We can easily fall back to video too.
>
>
> We *should not* fall back on arbitrary devices when there is source for
> alternate options. And that option is from specification:
>
> "The ConInDev, ConOutDev, and ErrOutDev variables each contain an EFI_DEVICE_PATH_PROTOCOL
> descriptor that defines all the possible default devices to use on boot.
> These variables are volatile, and are set dynamically on every boot. ConIn,
> ConOut, and ErrOut are always proper subsets of ConInDev, ConOutDev, and
> ErrOutDev.*”*
>

Right. Except they aren't a proper subset in this case. Since they aren't a
proper subset, can you count on them having any meaningful meaning? In the
cases you found they do, but it's just as arbitrary.


> UEFI Spec 2.8A, Page 82.
>
> There may or may not be Video (or Serial) device listed. If not, there are
> good chance we will be in trouble because the firmware internals may not be
> set up properly and we can just as well end up in hung system.
>

For x86, there's almost always Video. For !x86 it gets more troublesome.

The kernel shouldn't hang when we give it the wrong console because if the
device isn't there, it won't cninit won't work.

> Please seek more review is the point I'd hoped to make in the private
> email. This could easily have been reviewed. There was no urgent rush that
> required it to go in w/o review or even discussion.
>
>
> However, you didn't answer my question: Why the instant MFC? There's a 3
> day minimum for changes in head... And there's nothing so urgent that
> requires a short-circuit.
>
> Warner
>
>
>
> Because the issues is reported on 13. well, I guess You are right there,
> could have waited a bit more.
>

Right. Exactly my point....

Warner


> thanks,
> toomas
>
>
>
>
>
>
>> Thanks,
>> Toomas
>>
>> On Thu, Feb 4, 2021, 2:34 PM Toomas Soome <tsoome at freebsd.org> wrote:
>>
>>> The branch stable/13 has been updated by tsoome:
>>>
>>> URL:
>>> https://cgit.FreeBSD.org/src/commit/?id=0c839497c174e961fc71f7d3329d05b10ec5525b
>>> <https://cgit.freebsd.org/src/commit/?id=0c839497c174e961fc71f7d3329d05b10ec5525b>
>>>
>>> commit 0c839497c174e961fc71f7d3329d05b10ec5525b
>>> Author:     Toomas Soome <tsoome at FreeBSD.org>
>>> AuthorDate: 2021-02-04 20:49:02 +0000
>>> Commit:     Toomas Soome <tsoome at FreeBSD.org>
>>> CommitDate: 2021-02-04 21:33:15 +0000
>>>
>>>     loader.efi: There are systems without ConOut, also use ConOutDev
>>>
>>>     Conout does contian the default output device name.
>>>     ConOutDev does contain all possible output device names, so we can
>>>     use it as fallback, when there is no ConOut.
>>>
>>>     PR: 253253
>>>
>>>     (cherry picked from commit 2bd4ff2d8911009283e4e615ca4aad35a845f48b)
>>> ---
>>>  stand/efi/loader/main.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/stand/efi/loader/main.c b/stand/efi/loader/main.c
>>> index ca41cd4a2610..32b278950745 100644
>>> --- a/stand/efi/loader/main.c
>>> +++ b/stand/efi/loader/main.c
>>> @@ -735,6 +735,8 @@ parse_uefi_con_out(void)
>>>         how = 0;
>>>         sz = sizeof(buf);
>>>         rv = efi_global_getenv("ConOut", buf, &sz);
>>> +       if (rv != EFI_SUCCESS)
>>> +               rv = efi_global_getenv("ConOutDev", buf, &sz);
>>>         if (rv != EFI_SUCCESS) {
>>>                 /* If we don't have any ConOut default to serial */
>>>                 how = RB_SERIAL;
>>
>>
>