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

[Warner Losh]

On Fri, Feb 5, 2021 at 1:21 PM Warner Losh <imp at bsdimp.com> wrote:

>
>
> On Fri, Feb 5, 2021 at 1:09 PM Toomas Soome <tsoome at me.com> wrote:
>
>>
>>
>> On 5. Feb 2021, at 21:43, Warner Losh <imp at bsdimp.com> wrote:
>>
>>
>>
>> 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.
>>
>>
>> Well, we can argue if empty set is or is not subset of (any) other set.
>> But, we do have specification. And we should not arbitrary pick what part
>> we are going to follow and what not.
>>
>
> The empty set isn't a proper subset. It is a subset, but not a proper
> subset, by definition. Therefore, it's not standards complaint.
>

Hmmm, I just looked up 'proper subset' and it's the other end: ConOut must
be smaller than ConOutDev according to this wording. However, that makes no
sense here.

>
>>
>>> 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.
>>
>>
>> There is almost always ConOut as well. Except when there is not. And in
>> this case, there is not.
>>
>> I still do not get what it is we are arguing about. Do we have case where
>> ConOut is not set and ConOutDev does have garbage?
>>
>
> We have one sighting of 'ConOut' being missing. Any extrapolation from
> there is tricky. We know in this one case the info appears to be good. But
> this is not standards compliant, so how can we be sure that others will be
> similar?
>
> Warner
>
>
>> rgds,
>> toomas
>>
>>
>> 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;
>>>>
>>>>
>>