Date: Fri, 5 Feb 2021 13:26:35 -0700 From: Warner Losh <imp@bsdimp.com> To: Toomas Soome <tsoome@me.com> Cc: Toomas Soome <tsoome@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-branches@freebsd.org Subject: Re: git: 0c839497c174 - stable/13 - loader.efi: There are systems without ConOut, also use ConOutDev Message-ID: <CANCZdfrnOB5RS5BUzOSM2_cQ_dp5%2BgrA%2BocZFbFXQNOxxDcj_g@mail.gmail.com> In-Reply-To: <CANCZdfo-pRUQhw_7=MN6VPOcsgvRoP47PUC44yjbZbzopdtJSw@mail.gmail.com> References: <CANCZdfqXU7Syo4wvxJ17Gt%2B0E2Kw8yOU_nrTNA%2Beu=z-ZwTKow@mail.gmail.com> <CF59D855-228A-4BC1-AC4B-0C54417EF3BF@me.com> <CANCZdfrdpMXTxJbzY6w2NE_DtwZ1eBuMm%2B1rr6d0a22R2QKaCQ@mail.gmail.com> <97F5C09F-7AE3-4763-AD32-BFEA25101CE5@me.com> <CANCZdfpf6KAyPCnxJnxPAg2K4POK7okkp1m3O28VvAcrK1tzag@mail.gmail.com> <014891C8-7B1F-4908-9495-2ED1A5FAABCF@me.com> <CANCZdfo-pRUQhw_7=MN6VPOcsgvRoP47PUC44yjbZbzopdtJSw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Feb 5, 2021 at 1:21 PM Warner Losh <imp@bsdimp.com> wrote: > > > On Fri, Feb 5, 2021 at 1:09 PM Toomas Soome <tsoome@me.com> wrote: > >> >> >> On 5. Feb 2021, at 21:43, Warner Losh <imp@bsdimp.com> wrote: >> >> >> >> On Fri, Feb 5, 2021 at 10:24 AM Toomas Soome <tsoome@me.com> wrote: >> >>> >>> >>> On 5. Feb 2021, at 18:44, Warner Losh <imp@bsdimp.com> wrote: >>> >>> >>> >>> On Thu, Feb 4, 2021 at 11:38 PM Toomas Soome <tsoome@me.com> wrote: >>> >>>> >>>> >>>> On 5. Feb 2021, at 01:56, Warner Losh <imp@bsdimp.com> wrote: >>>> >>>> =EF=BB=BF >>>> 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 i= n >>>> this case was totally wrong choice), we can try the possible devices l= ist. >>>> 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 ca= n >>> 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 s= et >>> dynamically on every boot. ConIn, ConOut, and ErrOut are always proper >>> subsets of ConInDev, ConOutDev, and ErrOutDev.*=E2=80=9D* >>> >> >> 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 par= t >> 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 ma= y >>> 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 wher= e >> 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. Bu= t > this is not standards compliant, so how can we be sure that others will b= e > 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 t= hat >>> 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@freebsd.org> wrote: >>>> >>>>> The branch stable/13 has been updated by tsoome: >>>>> >>>>> URL: >>>>> https://cgit.FreeBSD.org/src/commit/?id=3D0c839497c174e961fc71f7d3329= d05b10ec5525b >>>>> <https://cgit.freebsd.org/src/commit/?id=3D0c839497c174e961fc71f7d332= 9d05b10ec5525b> >>>>> >>>>> commit 0c839497c174e961fc71f7d3329d05b10ec5525b >>>>> Author: Toomas Soome <tsoome@FreeBSD.org> >>>>> AuthorDate: 2021-02-04 20:49:02 +0000 >>>>> Commit: Toomas Soome <tsoome@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 ca= n >>>>> 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 =3D 0; >>>>> sz =3D sizeof(buf); >>>>> rv =3D efi_global_getenv("ConOut", buf, &sz); >>>>> + if (rv !=3D EFI_SUCCESS) >>>>> + rv =3D efi_global_getenv("ConOutDev", buf, &sz); >>>>> if (rv !=3D EFI_SUCCESS) { >>>>> /* If we don't have any ConOut default to serial */ >>>>> how =3D RB_SERIAL; >>>> >>>> >>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrnOB5RS5BUzOSM2_cQ_dp5%2BgrA%2BocZFbFXQNOxxDcj_g>