From owner-dev-commits-src-all@freebsd.org Fri Feb 5 20:26:47 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 DCFAE5295AF for ; Fri, 5 Feb 2021 20:26:47 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qv1-xf30.google.com (mail-qv1-xf30.google.com [IPv6:2607:f8b0:4864:20::f30]) (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 4DXRlq1Hnrz4VSc for ; Fri, 5 Feb 2021 20:26:46 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qv1-xf30.google.com with SMTP id w11so4083819qvz.12 for ; Fri, 05 Feb 2021 12:26:46 -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=mDi1cuBJfAjZD6twYIxn+1LpY1MeNrzofV545yAOJMc=; b=byh4yTE3bV+MFaD9F4rsSq5zeJ0PYRw7ffapDtSS2c3/g25LdeJSMMWtu+oe6mfcI5 FBugx09+TljN2e1O7Ihvm6QqnVHCT4bNfVMNr7x3NOWYDXIjXELhItBNO+YmJB5NxQf4 IZZBgxPvb4M794/Vj5rztSBgwCTlayohloXuB+FG3+LjO4Eo8YjobnF9QmmDM339Z2Pi Xw5djy8FuWAfCSn2EaJgBNs4SJz2e6+/01gA0PNvvkg9Kr0bg5H9dxL+08cvW9bnnJgx 6wYVzPOm2czLr6NEpd3g89JanRCZfJqj51yU2b8mVYVvyKLx2ee3pEl+EWQeEqWdJiVX VsyQ== 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=mDi1cuBJfAjZD6twYIxn+1LpY1MeNrzofV545yAOJMc=; b=Zr0YIg+1GC37lpjIL1WgfO6oygpYJxSRncAPu0QWo9NdmyM85ZlcZQsh7WfP24iMhS SFfHWlFr/C3a/7lCPZ7pdZ8bx8Pd+LrD14daTJYaUuLxbBilw/i1Wws+XQp9DcOroVhp lY2rOpfWMphoc2inLXFp/UGdglaYIB+1vNr59ENfRNTEZRHhchq6UMpOLRfVnJqcffvs 4tjTDPEzZOEcviLyq7syor++9/beE/j3uhAJRTAYR6jk0S3KdKBUYMptvG++LPctI72V Z0AAobwSrA09S8OVd1kH0LfcVONN2oPI/cRlmNMmuMRp0I6FScV7y3ynydtIszfHJPZV IgAg== X-Gm-Message-State: AOAM533N7iiyRfW0zv6op9+TOVUhJMHn8bZ6eFLFcrRtQGUp+cx2l6x0 JDHqxB02DTYue8Py7CunVAaC8Uv6hHQBI0fNrcT8ww== X-Google-Smtp-Source: ABdhPJystaQ4SKUGGFMzBQoXE0TBCYcXv5xtnlClv3bW/HimE6naHkQXXIQE9QX2Bn7IphHgmHe4lfuKRpXG2BlawLo= X-Received: by 2002:a0c:eb4c:: with SMTP id c12mr5969951qvq.62.1612556806222; Fri, 05 Feb 2021 12:26:46 -0800 (PST) MIME-Version: 1.0 References: <97F5C09F-7AE3-4763-AD32-BFEA25101CE5@me.com> <014891C8-7B1F-4908-9495-2ED1A5FAABCF@me.com> In-Reply-To: From: Warner Losh Date: Fri, 5 Feb 2021 13:26:35 -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: 4DXRlq1Hnrz4VSc X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=byh4yTE3; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::f30) 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-all@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; RCPT_COUNT_FIVE(0.00)[5]; SPAMHAUS_ZRD(0.00)[2607:f8b0:4864:20::f30: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::f30: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::f30: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-all]; 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-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 20:26:47 -0000 On Fri, Feb 5, 2021 at 1:21 PM Warner Losh wrote: > > > 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 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 wrote: >>>> >>>>> The branch stable/13 has been updated by tsoome: >>>>> >>>>> URL: >>>>> https://cgit.FreeBSD.org/src/commit/?id=3D0c839497c174e961fc71f7d3329= d05b10ec5525b >>>>> >>>>> >>>>> 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 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; >>>> >>>> >>