From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::244; helo=mail-it0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x244.google.com (mail-it0-x244.google.com [IPv6:2607:f8b0:4001:c0b::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AEBE4203BA505 for ; Mon, 14 May 2018 03:28:49 -0700 (PDT) Received: by mail-it0-x244.google.com with SMTP id c3-v6so9984703itj.4 for ; Mon, 14 May 2018 03:28:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=pGHOXRO5V5eLtDXKZu3tDdKaciM8i5Bekk4H4glxqJo=; b=G6tjxPLRgHZq0YmATavRpN9ewkw6ickNe0zdnjpMQ+JhSGHGFiKx7fUMHaAiT20Zpq KHLwL2HXF4C0836RTVqH6eo++koWpoUwtnY5csOxlaDgKuxu8qVqo2hinpmW8l9NBh47 KsW1ImTNS8G7l/ivfd018UCmMrCvDgbHBsxrI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=pGHOXRO5V5eLtDXKZu3tDdKaciM8i5Bekk4H4glxqJo=; b=QHiNiT8y+BE3muhC8zw/LkdLPy+Rdg6Um3uhJ1CJGJ/VYXGuvHGP+8aIQQqBmj3UHE H2ggu9AlRe0kT9NfswSailvd4sQNLazH6viM9KbwiPyetlCxBa6fwB0EEApagMzShZFH abGEmjls2/BohgvXtB1LM/bNFYqvV86sx3TagMu3UwCfj2dkxr4zaj0waRyncvk+xWss JZAeYaHPbTFE1x/jKwIpoCFFqwS2HMfh8buRquetTmZGLiAqkMHo1j3o9LtHWE8fwwYG LtVjP7z3dRJsTTNclIPAORf6qWmA2O4G3RVTvTTF5ogYr9zQKQB7wWp3GqLwnQOw1S3f K0dQ== X-Gm-Message-State: ALKqPwdVmN3NhALH+IgpNvvkOcE+32PDQJ5d+Z0L0AaHrBJcDcY/hGFK +TrYekSryLtNpKXZa68pXx26PNmTZBS5gg+pR639JQ== X-Google-Smtp-Source: AB8JxZpJ5uKh1jsnyiifXOzqBDqu3uYEBPxtymuDFpSIv1TxWtPPYASoBhG+Coil/ZHbuMZKY/4s5v54ppa77vl79FU= X-Received: by 2002:a24:af45:: with SMTP id l5-v6mr8868645iti.106.1526293728177; Mon, 14 May 2018 03:28:48 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.187.134 with HTTP; Mon, 14 May 2018 03:28:47 -0700 (PDT) In-Reply-To: <20180512225514.30760-1-lersek@redhat.com> References: <20180512225514.30760-1-lersek@redhat.com> From: Ard Biesheuvel Date: Mon, 14 May 2018 12:28:47 +0200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen Subject: Re: [PATCH] OvmfPkg/PlatformBootManagerLib: connect consoles unconditionally X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 May 2018 10:28:50 -0000 Content-Type: text/plain; charset="UTF-8" On 13 May 2018 at 00:55, Laszlo Ersek wrote: > If both ConIn and ConOut exist, but ConIn references none of the PS/2 > keyboard, the USB wild-card keyboard, and any serial ports, then > PlatformInitializeConsole() currently allows the boot to proceed without > any input devices at all. This makes for a bad user experience -- the > firmware menu could only be entered through OsIndications, set by a guest > OS. > > Do what ArmVirtQemu does already, namely connect the consoles, and add > them to ConIn / ConOut / ErrOut, unconditionally. (The underlying > EfiBootManagerUpdateConsoleVariable() function checks for duplicates.) > > The issue used to be masked by the EfiBootManagerConnectAll() call that > got conditionalized in commit 245c643cc8b7. > > This patch is best viewed with "git show -b -W". > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Fixes: 245c643cc8b73240c3b88cb55b2911b285a8c10d > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1577546 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek Reviewed-by: Ard Biesheuvel > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: conn_cons_uncond > > OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 127 +++++++------------- > 1 file changed, 44 insertions(+), 83 deletions(-) > > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > index 862fa6ebb4e5..004b753f4d26 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > @@ -26,7 +26,6 @@ VOID *mEfiDevPathNotifyReg; > EFI_EVENT mEfiDevPathEvent; > VOID *mEmuVariableEventReg; > EFI_EVENT mEmuVariableEvent; > -BOOLEAN mDetectVgaOnly; > UINT16 mHostBridgeDevId; > > // > @@ -830,35 +829,33 @@ DetectAndPreparePlatformPciDevicePath ( > ); > ASSERT_EFI_ERROR (Status); > > - if (!mDetectVgaOnly) { > + // > + // Here we decide whether it is LPC Bridge > + // > + if ((IS_PCI_LPC (Pci)) || > + ((IS_PCI_ISA_PDECODE (Pci)) && > + (Pci->Hdr.VendorId == 0x8086) && > + (Pci->Hdr.DeviceId == 0x7000) > + ) > + ) { > // > - // Here we decide whether it is LPC Bridge > + // Add IsaKeyboard to ConIn, > + // add IsaSerial to ConOut, ConIn, ErrOut > // > - if ((IS_PCI_LPC (Pci)) || > - ((IS_PCI_ISA_PDECODE (Pci)) && > - (Pci->Hdr.VendorId == 0x8086) && > - (Pci->Hdr.DeviceId == 0x7000) > - ) > - ) { > - // > - // Add IsaKeyboard to ConIn, > - // add IsaSerial to ConOut, ConIn, ErrOut > - // > - DEBUG ((EFI_D_INFO, "Found LPC Bridge device\n")); > - PrepareLpcBridgeDevicePath (Handle); > - return EFI_SUCCESS; > - } > + DEBUG ((EFI_D_INFO, "Found LPC Bridge device\n")); > + PrepareLpcBridgeDevicePath (Handle); > + return EFI_SUCCESS; > + } > + // > + // Here we decide which Serial device to enable in PCI bus > + // > + if (IS_PCI_16550SERIAL (Pci)) { > // > - // Here we decide which Serial device to enable in PCI bus > + // Add them to ConOut, ConIn, ErrOut. > // > - if (IS_PCI_16550SERIAL (Pci)) { > - // > - // Add them to ConOut, ConIn, ErrOut. > - // > - DEBUG ((EFI_D_INFO, "Found PCI 16550 SERIAL device\n")); > - PreparePciSerialDevicePath (Handle); > - return EFI_SUCCESS; > - } > + DEBUG ((EFI_D_INFO, "Found PCI 16550 SERIAL device\n")); > + PreparePciSerialDevicePath (Handle); > + return EFI_SUCCESS; > } > > // > @@ -877,26 +874,6 @@ DetectAndPreparePlatformPciDevicePath ( > } > > > -/** > - Do platform specific PCI Device check and add them to ConOut, ConIn, ErrOut > - > - @param[in] DetectVgaOnly - Only detect VGA device if it's TRUE. > - > - @retval EFI_SUCCESS - PCI Device check and Console variable update > - successfully. > - @retval EFI_STATUS - PCI Device check or Console variable update fail. > - > -**/ > -EFI_STATUS > -DetectAndPreparePlatformPciDevicePaths ( > - BOOLEAN DetectVgaOnly > - ) > -{ > - mDetectVgaOnly = DetectVgaOnly; > - return VisitAllPciInstances (DetectAndPreparePlatformPciDevicePath); > -} > - > - > /** > Connect the predefined platform default console device. > > @@ -910,50 +887,34 @@ PlatformInitializeConsole ( > ) > { > UINTN Index; > - EFI_DEVICE_PATH_PROTOCOL *VarConout; > - EFI_DEVICE_PATH_PROTOCOL *VarConin; > > // > - // Connect RootBridge > + // Do platform specific PCI Device check and add them to ConOut, ConIn, > + // ErrOut > // > - GetEfiGlobalVariable2 (EFI_CON_OUT_VARIABLE_NAME, (VOID **) &VarConout, > - NULL); > - GetEfiGlobalVariable2 (EFI_CON_IN_VARIABLE_NAME, (VOID **) &VarConin, NULL); > - > - if (VarConout == NULL || VarConin == NULL) { > - // > - // Do platform specific PCI Device check and add them to ConOut, ConIn, > - // ErrOut > - // > - DetectAndPreparePlatformPciDevicePaths (FALSE); > + VisitAllPciInstances (DetectAndPreparePlatformPciDevicePath); > > + // > + // Have chance to connect the platform default console, > + // the platform default console is the minimum device group > + // the platform should support > + // > + for (Index = 0; PlatformConsole[Index].DevicePath != NULL; ++Index) { > // > - // Have chance to connect the platform default console, > - // the platform default console is the minimum device group > - // the platform should support > + // Update the console variable with the connect type > // > - for (Index = 0; PlatformConsole[Index].DevicePath != NULL; ++Index) { > - // > - // Update the console variable with the connect type > - // > - if ((PlatformConsole[Index].ConnectType & CONSOLE_IN) == CONSOLE_IN) { > - EfiBootManagerUpdateConsoleVariable (ConIn, > - PlatformConsole[Index].DevicePath, NULL); > - } > - if ((PlatformConsole[Index].ConnectType & CONSOLE_OUT) == CONSOLE_OUT) { > - EfiBootManagerUpdateConsoleVariable (ConOut, > - PlatformConsole[Index].DevicePath, NULL); > - } > - if ((PlatformConsole[Index].ConnectType & STD_ERROR) == STD_ERROR) { > - EfiBootManagerUpdateConsoleVariable (ErrOut, > - PlatformConsole[Index].DevicePath, NULL); > - } > + if ((PlatformConsole[Index].ConnectType & CONSOLE_IN) == CONSOLE_IN) { > + EfiBootManagerUpdateConsoleVariable (ConIn, > + PlatformConsole[Index].DevicePath, NULL); > + } > + if ((PlatformConsole[Index].ConnectType & CONSOLE_OUT) == CONSOLE_OUT) { > + EfiBootManagerUpdateConsoleVariable (ConOut, > + PlatformConsole[Index].DevicePath, NULL); > + } > + if ((PlatformConsole[Index].ConnectType & STD_ERROR) == STD_ERROR) { > + EfiBootManagerUpdateConsoleVariable (ErrOut, > + PlatformConsole[Index].DevicePath, NULL); > } > - } else { > - // > - // Only detect VGA device and add them to ConOut > - // > - DetectAndPreparePlatformPciDevicePaths (TRUE); > } > } > > -- > 2.14.1.3.gb7cf6e02401b >