From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2ACD28197C for ; Thu, 5 Jan 2017 08:38:26 -0800 (PST) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ABE1463175; Thu, 5 Jan 2017 16:38:26 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-100.phx2.redhat.com [10.3.116.100]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v05GcOYV013001; Thu, 5 Jan 2017 11:38:25 -0500 To: Anthony PERARD , edk2-devel@ml01.01.org, xen-devel@lists.xenproject.org References: <20161208153340.2285-1-anthony.perard@citrix.com> <20161208153340.2285-13-anthony.perard@citrix.com> From: Laszlo Ersek Message-ID: Date: Thu, 5 Jan 2017 17:38:23 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20161208153340.2285-13-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 05 Jan 2017 16:38:26 +0000 (UTC) Subject: Re: [PATCH RFC 12/14] OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jan 2017 16:38:26 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 12/08/16 16:33, Anthony PERARD wrote: > and add OvmfPkg/XenConsoleIo/XenConsoleIo to XenOvmf platform. > > It actually look for gEfiSerialIoProtocolGuid. > --- > .../Library/PlatformBootManagerLib/BdsPlatform.c | 33 ++++++++++++++++++++++ > .../PlatformBootManagerLib.inf | 2 ++ > OvmfPkg/XenOvmf.dsc | 4 +++ > OvmfPkg/XenOvmf.fdf | 1 + > 4 files changed, 40 insertions(+) > > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > index bd64cc3..b8972f7 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > @@ -904,6 +904,31 @@ DetectAndPreparePlatformPciDevicePaths ( > return VisitAllPciInstances (DetectAndPreparePlatformPciDevicePath); > } > > +#include > +EFI_STATUS > +EFIAPI > +add_serial ( > + IN EFI_HANDLE DeviceHandle, > + IN VOID *Instance, > + IN VOID *Context > + ) > +{ > + EFI_DEVICE_PATH_PROTOCOL *DevicePath = NULL; > + > + DevicePath = DevicePathFromHandle(DeviceHandle); > + if (DevicePath == NULL) { > + return EFI_NOT_FOUND; > + } > + > + DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gTerminalTypeDeviceNode); > + DEBUG((EFI_D_ERROR, "%a %d: full path: %s\n", __FUNCTION__, __LINE__, > + ConvertDevicePathToText(DevicePath, TRUE, FALSE) > + )); > + EfiBootManagerUpdateConsoleVariable (ConOut, DevicePath, NULL); > + EfiBootManagerUpdateConsoleVariable (ConIn, DevicePath, NULL); > + EfiBootManagerUpdateConsoleVariable (ErrOut, DevicePath, NULL); > + return EFI_SUCCESS; > +} > > VOID > PlatformInitializeConsole ( > @@ -931,6 +956,14 @@ Arguments: > GetEfiGlobalVariable2 (EFI_CON_OUT_VARIABLE_NAME, (VOID **) &VarConout, NULL); > GetEfiGlobalVariable2 (EFI_CON_IN_VARIABLE_NAME, (VOID **) &VarConin, NULL); > > + // do xen console > + //VISIT_PCI_INSTANCE_CALLBACK CallBackFunction > + VisitAllInstancesOfProtocol ( > + &gEfiSerialIoProtocolGuid, > + add_serial, > + (VOID*)NULL > + ); > + > if (VarConout == NULL || VarConin == NULL) { > // > // Do platform specific PCI Device check and add them to ConOut, ConIn, ErrOut > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > index 4a6bece..74ab6b1 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > @@ -73,6 +73,8 @@ > gEfiLoadedImageProtocolGuid # PROTOCOL SOMETIMES_PRODUCED > gEfiFirmwareVolume2ProtocolGuid # PROTOCOL SOMETIMES_CONSUMED > > + gEfiSerialIoProtocolGuid > + > [Guids] > gEfiXenInfoGuid > gEfiEndOfDxeEventGroupGuid > diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc > index 31a2185..8bce996 100644 > --- a/OvmfPkg/XenOvmf.dsc > +++ b/OvmfPkg/XenOvmf.dsc > @@ -590,6 +590,10 @@ > OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > OvmfPkg/XenBusDxe/XenBusDxe.inf > OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf > + OvmfPkg/XenConsoleIo/XenConsoleIo.inf { > + > + SerialPortLib|OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf > + } > MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf > MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf > MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf > diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf > index f6876d7..a40d186 100644 > --- a/OvmfPkg/XenOvmf.fdf > +++ b/OvmfPkg/XenOvmf.fdf > @@ -223,6 +223,7 @@ INF OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf > INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > INF OvmfPkg/XenBusDxe/XenBusDxe.inf > INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf > +INF OvmfPkg/XenConsoleIo/XenConsoleIo.inf > > !if $(SECURE_BOOT_ENABLE) == TRUE > INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf > Okay, so this patch forced me to review the current additions of serial port devpaths to ConIn, ConOut, ErrOut in OvmfPkg/Library/PlatformBootManagerLib/ I think I understand what you intend to do. I'd like to propose an alternative I feel would be better. You are introducing a new driver called "XenConsoleIo" which I assume is a DXE_DRIVER module that produces gEfiSerialIoProtocolGuid. The reason I can only "assume" is that you apparently forgot to include the driver in the patch, with "git add". Nonetheless, without having seen the driver, I claim that it should be unnecessary. Namely, MdeModulePkg provides a generic platform serial driver, under MdeModulePkg/Universal/SerialDxe/SerialDxe.inf This driver is already used in the ARM Xen guest platform: ArmVirtPkg/ArmVirtXen.dsc with a dependency on OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf which you also employ above. Delegating the actual serial port access to the SerialPortLib class, the SerialDxe driver produces one instance of EFI_SERIAL_IO_PROTOCOL. The device path protocol instance that it installs on the handle is constant, and known to other edk2 modules. ( In turn, the TerminalDxe driver layers on top of the EFI_SERIAL_IO_PROTOCOL instance, and produces simple text in/out protocols on a child handle. The device path node appended specifically for the child handle determines the terminal type (e.g., vt100, pc-ansi, etc) to emulate. The terminal type can be selected by the platform by adding a devpath to the Con* variables that ends with the desired device path node, or else the TerminalDxe driver can fall back to a default type. But, I digress. ) What this all boils down to is: (1) Rather than introducing a new driver (again, not having it seen), please include "MdeModulePkg/Universal/SerialDxe/SerialDxe.inf" in the XenOvmf DSC and FDF files. You can resolve the SerialPortLib class to XenConsoleSerialPortLib either for this one module only, or generally (same as in "ArmVirtPkg/ArmVirtXen.dsc"). (I assume that in a Xen HVM guest, both the emulated serial port and the paravirt xen console are available ) (2) In the code, we shouldn't locate the appropriate handle dynamically. The full device path is known statically, in advance. Please refer to "mSerialConsole" in ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c Therefore I suggest that we add a "gXenPlatformConsole" array to OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c with an initializer that effects the following: - gXenPlatformConsole[1] is entirely zeroed, - gXenPlatformConsole[0].ConnectType = CONSOLE_IN | CONSOLE_OUT | STD_ERROR; - gXenPlatformConsole[0].DevicePath should be what's seen in "mSerialConsole" above, but with the terminal type GUID pre-filled to PC-ANSI ("gPcAnsiTerminal"). Then, we shouldn't touch the PlatformInitializeConsole() function; instead modify the PlatformInitializeConsole() call in PlatformBootManagerBeforeConsole() like this: PlatformInitializeConsole ( XenDetected() ? gXenPlatformConsole : gPlatformConsole); This will cause PlatformInitializeConsole() to do the right thing, and it won't introduce special handling for gEfiSerialIoProtocolGuid that would superfluously run on QEMU as well. Furthermore, in a Xen HVM guest, I reckon this will multiplex terminal IO to both emulated serial and Xen console. Thanks Laszlo