* [PATCH] OvmfPkg/PlatformBootManagerLib: connect consoles unconditionally
@ 2018-05-12 22:55 Laszlo Ersek
2018-05-14 10:28 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2018-05-12 22:55 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
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 <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Fixes: 245c643cc8b73240c3b88cb55b2911b285a8c10d
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1577546
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] OvmfPkg/PlatformBootManagerLib: connect consoles unconditionally
2018-05-12 22:55 [PATCH] OvmfPkg/PlatformBootManagerLib: connect consoles unconditionally Laszlo Ersek
@ 2018-05-14 10:28 ` Ard Biesheuvel
2018-05-14 13:26 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2018-05-14 10:28 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 13 May 2018 at 00:55, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Fixes: 245c643cc8b73240c3b88cb55b2911b285a8c10d
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1577546
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] OvmfPkg/PlatformBootManagerLib: connect consoles unconditionally
2018-05-14 10:28 ` Ard Biesheuvel
@ 2018-05-14 13:26 ` Laszlo Ersek
0 siblings, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2018-05-14 13:26 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Jordan Justen, edk2-devel-01
On 05/14/18 12:28, Ard Biesheuvel wrote:
> On 13 May 2018 at 00:55, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Fixes: 245c643cc8b73240c3b88cb55b2911b285a8c10d
>> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1577546
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Thank you (and welcome back :) ), commit f803c03cc2e0.
Laszlo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-05-14 13:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-12 22:55 [PATCH] OvmfPkg/PlatformBootManagerLib: connect consoles unconditionally Laszlo Ersek
2018-05-14 10:28 ` Ard Biesheuvel
2018-05-14 13:26 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox