From owner-dev-commits-src-all@freebsd.org Fri Feb 5 17:24:42 2021 Return-Path: Delivered-To: dev-commits-src-all@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 D6D7B54D5BD for ; Fri, 5 Feb 2021 17:24:42 +0000 (UTC) (envelope-from tsoome@me.com) Received: from pv50p00im-ztbu10011701.me.com (pv50p00im-ztbu10011701.me.com [17.58.6.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4DXMjk4tWfz3ntg for ; Fri, 5 Feb 2021 17:24:42 +0000 (UTC) (envelope-from tsoome@me.com) Received: from nazgul.lan (148-52-235-80.sta.estpak.ee [80.235.52.148]) by pv50p00im-ztbu10011701.me.com (Postfix) with ESMTPSA id A33468A05D5; Fri, 5 Feb 2021 17:24:37 +0000 (UTC) From: Toomas Soome Message-Id: <97F5C09F-7AE3-4763-AD32-BFEA25101CE5@me.com> Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.60.0.2.21\)) Subject: Re: git: 0c839497c174 - stable/13 - loader.efi: There are systems without ConOut, also use ConOutDev Date: Fri, 5 Feb 2021 19:24:35 +0200 In-Reply-To: Cc: Toomas Soome , src-committers , dev-commits-src-all@freebsd.org, dev-commits-src-branches@freebsd.org To: Warner Losh References: X-Mailer: Apple Mail (2.3654.60.0.2.21) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369, 18.0.737 definitions=2021-02-05_10:2021-02-05, 2021-02-05 signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 mlxscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-2006250000 definitions=main-2102050110 X-Rspamd-Queue-Id: 4DXMjk4tWfz3ntg X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] 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-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Feb 2021 17:24:42 -0000 > On 5. Feb 2021, at 18:44, Warner Losh wrote: >=20 >=20 >=20 > On Thu, Feb 4, 2021 at 11:38 PM Toomas Soome > wrote: >=20 >=20 >> On 5. Feb 2021, at 01:56, Warner Losh > wrote: >>=20 >> =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... >>=20 >> Warner=20 >>=20 >=20 > 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. >=20 > 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. >=20 > If it would appear, there are systems with unusable devices listed in = ConOutDev, then we need to think how to handle such case. >=20 > 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.=E2=80=9D 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. >=20 > 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. >=20 > 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. >=20 > Warner Because the issues is reported on 13. well, I guess You are right there, = could have waited a bit more. thanks, toomas >=20 > =20 > Thanks, > Toomas >=20 >> On Thu, Feb 4, 2021, 2:34 PM Toomas Soome > wrote: >> The branch stable/13 has been updated by tsoome: >>=20 >> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D0c839497c174e961fc71f7d3329d05b1= 0ec5525b = >>=20 >> commit 0c839497c174e961fc71f7d3329d05b10ec5525b >> Author: Toomas Soome >> AuthorDate: 2021-02-04 20:49:02 +0000 >> Commit: Toomas Soome >> CommitDate: 2021-02-04 21:33:15 +0000 >>=20 >> loader.efi: There are systems without ConOut, also use ConOutDev >>=20 >> 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. >>=20 >> PR: 253253 >>=20 >> (cherry picked from commit = 2bd4ff2d8911009283e4e615ca4aad35a845f48b) >> --- >> stand/efi/loader/main.c | 2 ++ >> 1 file changed, 2 insertions(+) >>=20 >> 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;