> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, May 7, 2019 1:42 AM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao....@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; > Ni, Ray <ray...@intel.com>; Zeng, Star <star.z...@intel.com>; Gao, Liming > <liming....@intel.com>; Sean Brogan <sean.bro...@microsoft.com>; > Michael Turner <michael.tur...@microsoft.com>; Bret Barkelew > <bret.barke...@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.w...@intel.com> > > Cc: Hao Wu <hao.a...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Star Zeng <star.z...@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > Cc: Sean Brogan <sean.bro...@microsoft.com> > > Cc: Michael Turner <michael.tur...@microsoft.com> > > Cc: Bret Barkelew <bret.barke...@microsoft.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Zhichao Gao <zhichao....@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-