[edk2-devel] [PATCH V3 2/2] MdeModulePkg/GraphicsConsoleDxe: Initialize the output mode
Gao, Zhichao
zhichao.gao at intel.com
Tue May 7 07:11:28 UTC 2019
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek at redhat.com]
> Sent: Tuesday, May 7, 2019 1:42 AM
> To: devel at edk2.groups.io; Gao, Zhichao <zhichao.gao at intel.com>
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>;
> Ni, Ray <ray.ni at intel.com>; Zeng, Star <star.zeng at intel.com>; Gao, Liming
> <liming.gao at intel.com>; Sean Brogan <sean.brogan at microsoft.com>;
> Michael Turner <Michael.Turner at microsoft.com>; Bret Barkelew
> <Bret.Barkelew at 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 at intel.com>
> > Cc: Hao Wu <hao.a.wu at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Star Zeng <star.zeng at intel.com>
> > Cc: Liming Gao <liming.gao at intel.com>
> > Cc: Sean Brogan <sean.brogan at microsoft.com>
> > Cc: Michael Turner <Michael.Turner at microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao at 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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#40082): https://edk2.groups.io/g/devel/message/40082
Mute This Topic: https://groups.io/mt/31505827/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list