From owner-dev-commits-src-branches@freebsd.org Fri Feb 5 20:21:51 2021 Return-Path: Delivered-To: dev-commits-src-branches@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id AB93A529509 for ; Fri, 5 Feb 2021 20:21:51 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qv1-xf36.google.com (mail-qv1-xf36.google.com [IPv6:2607:f8b0:4864:20::f36]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DXRf61gwfz4VLX for ; Fri, 5 Feb 2021 20:21:50 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qv1-xf36.google.com with SMTP id h21so4089304qvb.8 for ; Fri, 05 Feb 2021 12:21:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ktaCHAHoDKFDrx5XZH9UWMchOoQVyW2RernLwvgZELM=; b=q1P2KxcpGwS6L64Xr0gmxm4/r36DgqfJVw1ih2P/NM2Jom/Rsq5KQPm1IKr2Odcb4M nvCSNyOUUO2GsCQnj1G8XflGlBQffIZ8zzgsOsfrVFxHBUIlFM3ar/uGgAH9d9BgLON4 ATZuWghnKsleP3r9Utw8kNpIohly+DJMMBN6Yn7g6bO5YSUEkcN3GStir7AKWpMIueJk 9hqFAqdHeHC4mE2STU1VHTZaSFodGuDj4cJ33ZBNwYAyZq59tKsZMZFhS4vq32uXHN+J KxfZ+/xno/0c/Hy4ZP55WTTGN1N8kx3WIfWxGeiYjoK+2qTTSX6jipPjN16WWZtFAvlq 9VOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ktaCHAHoDKFDrx5XZH9UWMchOoQVyW2RernLwvgZELM=; b=unwbTzc9ez/EjZPS1Cx1csaILZDSX0QjbAXm1EFNN4v4bpyna2gDXv6AWllC4lBUag MLqbu3+NXrPtgitnUNLfRvP6ui+QK55wpl2nTa7SBjWc8fxz10nnSkfQaueUJ9pRzLlO SKWkInpxLWNE7BrZmVOX6+3JgiGW+LUZ6TFA2grQZqZupnGJvvZo+RtDyCfJPGyjYD50 zUecqjTrT3GvrIIMx68B4mAvVZmGJCRZtBK+TEooHcoS66GUpR86sWO52HCyKKGWdy/p axo+kxphcNvVsTozEiG+owfLSu9tD+IC9XvFY+Q3ua2WsLLb6ec8MzE+A57ad4/4KQoJ emCA== X-Gm-Message-State: AOAM531noLJfY8N3j9JZ9SXaSnqFnjt7R5kLBCBIj5rqnoYD4931D9Y5 wdDg9a8mTA3z6fRZo4iUYYlhMaUzgMjEQuEdqjQVQg== X-Google-Smtp-Source: ABdhPJzwhPZT50fr4LJ/RbuLv+Dywwt+/hi8dK8HrbcaNGh5SV0H5ecQ18XpwnOEodKZ74C6O0RuF+TCoBUUa7N9PSY= X-Received: by 2002:a05:6214:14e2:: with SMTP id k2mr5986069qvw.24.1612556509269; Fri, 05 Feb 2021 12:21:49 -0800 (PST) MIME-Version: 1.0 References: <97F5C09F-7AE3-4763-AD32-BFEA25101CE5@me.com> <014891C8-7B1F-4908-9495-2ED1A5FAABCF@me.com> In-Reply-To: <014891C8-7B1F-4908-9495-2ED1A5FAABCF@me.com> From: Warner Losh Date: Fri, 5 Feb 2021 13:21:38 -0700 Message-ID: Subject: Re: git: 0c839497c174 - stable/13 - loader.efi: There are systems without ConOut, also use ConOutDev To: Toomas Soome Cc: Toomas Soome , src-committers , "" , dev-commits-src-branches@freebsd.org X-Rspamd-Queue-Id: 4DXRf61gwfz4VLX X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=q1P2Kxcp; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::f36) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-3.00 / 15.00]; RCVD_TLS_ALL(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[dev-commits-src-branches@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; RCPT_COUNT_FIVE(0.00)[5]; SPAMHAUS_ZRD(0.00)[2607:f8b0:4864:20::f36:from:127.0.2.255]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; NEURAL_HAM_SHORT(-1.00)[-1.000]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::f36:from]; R_SPF_NA(0.00)[no SPF record]; FREEMAIL_TO(0.00)[me.com]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; MIME_TRACE(0.00)[0:+,1:+,2:~]; RBL_DBL_DONT_QUERY_IPS(0.00)[2607:f8b0:4864:20::f36:from]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; MAILMAN_DEST(0.00)[dev-commits-src-branches]; RCVD_COUNT_TWO(0.00)[2] Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.34 X-BeenThere: dev-commits-src-branches@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commits to the stable branches of the FreeBSD src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Feb 2021 20:21:51 -0000 On Fri, Feb 5, 2021 at 1:09 PM Toomas Soome wrote: > > > On 5. Feb 2021, at 21:43, Warner Losh wrote: > > > > On Fri, Feb 5, 2021 at 10:24 AM Toomas Soome wrote: > >> >> >> On 5. Feb 2021, at 18:44, Warner Losh wrote: >> >> >> >> On Thu, Feb 4, 2021 at 11:38 PM Toomas Soome wrote: >> >>> >>> >>> On 5. Feb 2021, at 01:56, Warner Losh 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 in >>> this case was totally wrong choice), we can try the possible devices li= st. >>> 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 b= e >> 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 se= t >> 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 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. > > >> 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 th= e > 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 th= at >> 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 wrote: >>> >>>> The branch stable/13 has been updated by tsoome: >>>> >>>> URL: >>>> https://cgit.FreeBSD.org/src/commit/?id=3D0c839497c174e961fc71f7d3329d= 05b10ec5525b >>>> >>>> >>>> commit 0c839497c174e961fc71f7d3329d05b10ec5525b >>>> Author: Toomas Soome >>>> AuthorDate: 2021-02-04 20:49:02 +0000 >>>> Commit: Toomas Soome >>>> 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 =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; >>> >>> >