Re: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode

["Laszlo Ersek" <lersek@redhat.com>]

On 05/05/19 04:17, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1412
>
> Original logic:
> Connect the graphics device -> connect it as graphics consoles
> and initialize its parameters(Mode = -1, invalid) -> connect it
> as console spliter and add the device to the list(use SetMode to
> set mode to the user defined mode or the best mode the devices
> supported if the mode is invalid. *clear the screen at this phase*)
>
> Changed logic:
> Connect the graphics device -> connect it as graphics consoles
> and initialize its parameters(initialize the mode to the user
> defined mode or the best mode. *directly set the mode value without
> using SetMode, that would not clear the screen) -> connect it as
> console spliter and add the device to the list(use SetMode to set
> mode to the user defined mode or the best mode the devices supported
> if the mode is invalid. *now the mode is already set, so it would
> not clear the screen*).
>
> Also remove the section of SetMode for debug version.
>
> But there would be no clear screen operation while reconnect the
> graphics device. Instead, clear the screen when stop the graphics
> console device.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  .../GraphicsConsoleDxe/GraphicsConsole.c      | 44 ++++++++++++++-----
>  .../GraphicsConsoleDxe/GraphicsConsoleDxe.inf |  2 +
>  2 files changed, 36 insertions(+), 10 deletions(-)

This driver is too complex for me to decide whether this patch is
correct.

The spec writes:

> If the output device is not in a valid text mode at the time of the
> EFI_BOOT_SERVICES.HandleProtocol() call, the device is to indicate
> that its CurrentMode is -1.

(1) Because the patch sets a value different from -1 to CurrentMode, can
we guarantee that the console will be usable immediately?


More comments below:

On 05/05/19 04:17, Gao, Zhichao wrote:
> diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> index 26ea19f300..42386de3f5 100644
> --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole.c
> @@ -1,7 +1,7 @@
>  /** @file
>    This is the main routine for initializing the Graphics Console support routines.
>
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
> @@ -384,6 +384,12 @@ GraphicsConsoleControllerDriverStart (
>    EFI_GRAPHICS_OUTPUT_PROTOCOL_MODE    *Mode;
>    UINTN                                SizeOfInfo;
>    EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info;
> +  UINTN                                PreferMode;
> +  UINTN                                Index;
> +  UINTN                                Column;
> +  UINTN                                Row;
> +  UINTN                                DefaultColumn;
> +  UINTN                                DefaultRow;
>
>    ModeNumber = 0;
>
> @@ -567,16 +573,32 @@ GraphicsConsoleControllerDriverStart (
>    //
>    Private->SimpleTextOutputMode.MaxMode = (INT32) MaxMode;
>
> -  DEBUG_CODE_BEGIN ();
> -    Status = GraphicsConsoleConOutSetMode (&Private->SimpleTextOutput, 0);
> -    if (EFI_ERROR (Status)) {
> -      goto Error;
> -    }
> -    Status = GraphicsConsoleConOutOutputString (&Private->SimpleTextOutput, (CHAR16 *)L"Graphics Console Started\n\r");
> -    if (EFI_ERROR (Status)) {
> -      goto Error;
> +  //
> +  // Initialize the Mode of graphics console devices
> +  //
> +  PreferMode = 0;
> +  DefaultColumn = PcdGet32 (PcdConOutColumn);
> +  DefaultRow = PcdGet32 (PcdConOutRow);
> +  Column = 0;
> +  Row = 0;
> +  for (Index = 0; Index < MaxMode; Index++) {
> +    if (DefaultColumn != 0 && DefaultRow != 0) {
> +      if ((Private->ModeData[Index].Columns == Column) &&
> +          (Private->ModeData[Index].Rows == Row)) {
> +        PreferMode = Index;
> +        break;
> +      }
> +    } else {
> +      if ((Private->ModeData[Index].Columns > Column) &&
> +          (Private->ModeData[Index].Rows > Row)) {
> +        Column = Private->ModeData[Index].Columns;
> +        Row = Private->ModeData[Index].Rows;
> +        PreferMode = Index;
> +      }
>      }
> -  DEBUG_CODE_END ();
> +  }
> +  Private->SimpleTextOutput.Mode->Mode = (INT32)PreferMode;

(2) The loop above can terminate without assigning Index to PreferMode.

In that case, the assignment right after the loop will set the Mode
field to 0, when overwriting the initial value (-1).

Is this intentional?

Because, if we can find no matching mode, it seems like we should stick
with (-1) -- for "current text mode is not valid". In that case, the
following passage from the spec would apply:

> On connecting to the output device the caller is required to verify
> the mode of the output device, and if it is not acceptable to set it
> to something it can use.

Should we perhaps change the type of PreferMode to INT32, and set it
initially to (-1)?


More comments:

On 05/05/19 04:17, Gao, Zhichao wrote:
> +  DEBUG ((DEBUG_INFO, "Graphics Console Started, Mode: %x\n\r", PreferMode));

(3) DEBUG messages should not contain "\r" characters. My understanding
is that PrintLib adds "\r" automatically.


(4) UINTN values (which may be UINT64, dependent on build architecture)
should not be printed with "%x". UINTN should be converted to UINT64
explicitly, for printing, and then printed with "%Lx" (or "%Lu").

(If you decide to switch PreferMode to INT32, then %d is the format
specifier to use, of course.)


(5) I applied the series, for testing, on top of commit fbb0ec7ea4c0
(current master). I tested the patches briefly with OVMF.

With the patches applied, the firmware crashes when I run "reconnect
-r", as the first command, in the built-in UEFI shell:

> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 00000000 !!!!
> ExceptionData - 0000000000000000
> RIP  - 000000007EECB8F9, CS  - 0000000000000038, RFLAGS - 0000000000010012
> RAX  - 0000D0EC8148E589, RCX - 000000007EF06FE0, RDX - 000000000904A7A0
> RBX  - 0000000000000013, RSP - 000000007EEC9110, RBP - 000000007EEC9140
> RSI  - 000000007EEC92D8, RDI - 000000007EF07010
> R8   - 0000000000000007, R9  - 000000012E9C46D6, R10 - 0000000400020100
> R11  - 0000000000000008, R12 - 0000000000000000, R13 - 0000000000000000
> R14  - 0000000000000000, R15 - 0000000000000000
> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
> GS   - 0000000000000030, SS  - 0000000000000030
> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007EC01000
> CR4  - 0000000000000668, CR8 - 0000000000000000
> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> GDTR - 000000007EBEDA98 0000000000000047, LDTR - 0000000000000000
> IDTR - 000000007E241018 0000000000000FFF,   TR - 0000000000000000
> FXSAVE_STATE - 000000007EEC8D70
> !!!! Find image based on IP(0x7EECB8F9)
> Build/Ovmf3264/NOOPT_GCC48/X64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll
> (ImageBase=000000007EECB000, EntryPoint=000000007EED00C3) !!!!

The crash offset is 0x000000007EECB8F9-0x000000007EECB000 = 0x8F9.

The crash occurs in the InternalBaseLibIsListValid() function:

>     //
>     // Count the total number of nodes in List.
>     // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength
>     //
>     do {
>       Ptr = Ptr->ForwardLink;
>      8f5:       48 8b 45 f0             mov    -0x10(%rbp),%rax
>      8f9:       48 8b 00                mov    (%rax),%rax        <------- here
>      8fc:       48 89 45 f0             mov    %rax,-0x10(%rbp)

(RAX = 0xD0EC_8148_E589, which is approximately 208 TB, so obviously
something corrupted a list).

The crash does not occur ("reconnect -r" works fine) if I build OVMF at
fbb0ec7ea4c0 (current master).

Thanks,
Laszlo

>
>    //
>    // Install protocol interfaces for the Graphics Console device.
> @@ -669,6 +691,8 @@ GraphicsConsoleControllerDriverStop (
>      return EFI_NOT_STARTED;
>    }
>
> +  SimpleTextOutput->ClearScreen (SimpleTextOutput);
> +
>    Private = GRAPHICS_CONSOLE_CON_OUT_DEV_FROM_THIS (SimpleTextOutput);
>
>    Status = gBS->UninstallProtocolInterface (
> diff --git a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
> index f7caa65aa9..bcfd306eee 100644
> --- a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
> +++ b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
> @@ -65,6 +65,8 @@
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution   ## SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow                 ## SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn              ## SOMETIMES_CONSUMES
>
>  [UserExtensions.TianoCore."ExtraFiles"]
>    GraphicsConsoleDxeExtra.uni
>