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

["Gao, Zhichao" <zhichao.gao@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, May 7, 2019 1:42 AM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH V3 2/2]
> MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode
> 
> 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?

I think we can guarantee that the CurrentMode value is valid except the value is 0 or -1. Because the initialize mode section is same with the one in ConSplitterDxe.

> 
> 
> 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/GraphicsConsol
> e.c
> > index 26ea19f300..42386de3f5 100644
> > ---
> >
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
> .c
> > +++
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
> > +++ e.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)?

I think you are right. If no mode is queried, then invalid mode should be returned.

> 
> 
> 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.)

Good catch. I will make the Prefermode to INT32 and adjust the debug code.

> 
> 
> (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/Dxe
> > Core.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).

I have tried that. Something wrong with the ClearScreen function. I will investigate it later.

Thanks,
Zhichao

> 
> 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/GraphicsConsole
> Dxe
> > .inf
> >
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
> eDxe
> > .inf
> > index f7caa65aa9..bcfd306eee 100644
> > ---
> >
> a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
> Dxe
> > .inf
> > +++
> b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
> > +++ eDxe.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
> >