From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 10 Jul 2019 03:49:01 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 715B530833A5; Wed, 10 Jul 2019 10:49:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-153.ams2.redhat.com [10.36.117.153]) by smtp.corp.redhat.com (Postfix) with ESMTP id A58D62B4B1; Wed, 10 Jul 2019 10:48:58 +0000 (UTC) Subject: Re: [PATCH v3 32/35] OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn To: Anthony PERARD , devel@edk2.groups.io Cc: xen-devel@lists.xenproject.org, Ard Biesheuvel , Jordan Justen , Julien Grall References: <20190704144233.27968-1-anthony.perard@citrix.com> <20190704144233.27968-33-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <5ce18fa6-100f-e792-199f-cdecf6b04177@redhat.com> Date: Wed, 10 Jul 2019 12:48:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190704144233.27968-33-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Wed, 10 Jul 2019 10:49:00 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/04/19 16:42, Anthony PERARD wrote: > On a Xen PVH guest, none of the existing serial or console interface > works, so we add a new one, based on XenConsoleSerialPortLib, and > implemented via SerialDxe. > > That is a simple console implementation that can works on both PVH > guest and HVM guests, even if it rarely going to be use on HVM. > > Have PlatformBootManagerLib look for the new console, when running as a > Xen guest. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689 > Signed-off-by: Anthony PERARD > --- > > Notes: > v3: > - removed PciSioSerialDxe and IsaSerialDxe from OvmfXen, since they > would not be used, maybe, to check. > - some coding style fix > > - not changed: PciSioSerialDxe: even if we add SerialDxe, we still needs > PciSioSerialDxe to have OVMF use the emulated serial port on HVM. > > v2: > - Use MdeModulePkg/Universal/SerialDxe instead of something new. > - Have PlatformInitializeConsole() look for it by using the > known-in-advance device path for the xen console in the > PLATFORM_CONSOLE_CONNECT_ENTRY. > > OvmfPkg/OvmfXen.dsc | 4 ++ > OvmfPkg/OvmfXen.fdf | 1 + > .../PlatformBootManagerLib.inf | 4 ++ > .../PlatformBootManagerLib/BdsPlatform.h | 1 + > .../PlatformBootManagerLib/BdsPlatform.c | 3 +- > .../PlatformBootManagerLib/PlatformData.c | 59 ++++++++++++++++++- > 6 files changed, 69 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc > index 1ecae3fb45..487bada64d 100644 > --- a/OvmfPkg/OvmfXen.dsc > +++ b/OvmfPkg/OvmfXen.dsc > @@ -586,6 +586,10 @@ [Components] > OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > OvmfPkg/XenBusDxe/XenBusDxe.inf > OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf > + MdeModulePkg/Universal/SerialDxe/SerialDxe.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/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf > index fa0830a324..5c1a925d6a 100644 > --- a/OvmfPkg/OvmfXen.fdf > +++ b/OvmfPkg/OvmfXen.fdf > @@ -312,6 +312,7 @@ [FV.DXEFV] > INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > INF OvmfPkg/XenBusDxe/XenBusDxe.inf > INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf > +INF MdeModulePkg/Universal/SerialDxe/SerialDxe.inf > > INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf > INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > index b2d3b4fb4d..646a1c522c 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > @@ -61,6 +61,10 @@ [Pcd] > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId > gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES > > [Pcd.IA32, Pcd.X64] > gEfiMdePkgTokenSpaceGuid.PcdFSBClock > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h > index 49a072b400..153e215101 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h > +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h > @@ -165,6 +165,7 @@ typedef struct { > #define CONSOLE_IN BIT1 > #define STD_ERROR BIT2 > extern PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[]; > +extern PLATFORM_CONSOLE_CONNECT_ENTRY gXenPlatformConsole[]; > > // > // Platform BDS Functions > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > index 9ae590293a..ee6ee6608f 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c > @@ -399,7 +399,8 @@ PlatformBootManagerBeforeConsole ( > // > EfiBootManagerDispatchDeferredImages (); > > - PlatformInitializeConsole (gPlatformConsole); > + PlatformInitializeConsole ( > + XenDetected() ? gXenPlatformConsole : gPlatformConsole); > PcdStatus = PcdSet16S (PcdPlatformBootTimeOut, > GetFrontPageTimeoutFromQemu ()); > ASSERT_RETURN_ERROR (PcdStatus); > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c > index 36aab784d7..a9b1fe274a 100644 > --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c > +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c > @@ -9,18 +9,19 @@ > > #include "BdsPlatform.h" > #include > +#include > > // > // Debug Agent UART Device Path structure > // > -#pragma pack(1) > +#pragma pack (1) > typedef struct { > VENDOR_DEVICE_PATH VendorHardware; > UART_DEVICE_PATH Uart; > VENDOR_DEVICE_PATH TerminalType; > EFI_DEVICE_PATH_PROTOCOL End; > } VENDOR_UART_DEVICE_PATH; > -#pragma pack() > +#pragma pack () > > // > // USB Keyboard Device Path structure > @@ -43,6 +44,18 @@ typedef struct { > } VENDOR_RAMFB_DEVICE_PATH; > #pragma pack () > > +// > +// Xen Console Device Path structure > +// > +#pragma pack(1) > +typedef struct { > + VENDOR_DEVICE_PATH VendorHardware; > + UART_DEVICE_PATH Uart; > + VENDOR_DEVICE_PATH TerminalType; > + EFI_DEVICE_PATH_PROTOCOL End; > +} XEN_CONSOLE_DEVICE_PATH; > +#pragma pack() > + This version of the patch addresses all of my v2 review comments (either by code changes or by explanations in the Notes section) -- thanks for that. However, when you arrived at my reuqest (6) in , and searched the source file for "pack(" -- in order to insert a space character before the opening paren --, the match was *not* around the new XEN_CONSOLE_DEVICE_PATH structure. Instead, it was around the preexistent VENDOR_UART_DEVICE_PATH structure. And so you fixed the style for the old code, and not the new code. But: that's actually useful. Because now that I'm looking at VENDOR_UART_DEVICE_PATH, it seems that we don't need the new type XEN_CONSOLE_DEVICE_PATH at all. Is that right? So: (1) Please drop XEN_CONSOLE_DEVICE_PATH. (2) Please replace the comment Debug Agent UART Device Path structure with Vendor UART Device Path structure on VENDOR_UART_DEVICE_PATH. (3) Please preserve the "misplaced" whitespace fix, for "pack(", around VENDOR_UART_DEVICE_PATH. (4) Please use VENDOR_UART_DEVICE_PATH as the type of gXenConsoleDevicePath. With those: Reviewed-by: Laszlo Ersek Thanks! Laszlo > ACPI_HID_DEVICE_PATH gPnpPs2KeyboardDeviceNode = gPnpPs2Keyboard; > ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode = gPnp16550ComPort; > UART_DEVICE_PATH gUartDeviceNode = gUart; > @@ -141,6 +154,37 @@ STATIC VENDOR_RAMFB_DEVICE_PATH gQemuRamfbDevicePath = { > gEndEntire > }; > > +STATIC XEN_CONSOLE_DEVICE_PATH gXenConsoleDevicePath = { > + { > + { > + HARDWARE_DEVICE_PATH, > + HW_VENDOR_DP, > + { > + (UINT8) (sizeof (VENDOR_DEVICE_PATH)), > + (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8) > + } > + }, > + EDKII_SERIAL_PORT_LIB_VENDOR_GUID > + }, > + { > + { > + MESSAGING_DEVICE_PATH, > + MSG_UART_DP, > + { > + (UINT8) (sizeof (UART_DEVICE_PATH)), > + (UINT8) ((sizeof (UART_DEVICE_PATH)) >> 8) > + } > + }, > + 0, > + FixedPcdGet64 (PcdUartDefaultBaudRate), > + FixedPcdGet8 (PcdUartDefaultDataBits), > + FixedPcdGet8 (PcdUartDefaultParity), > + FixedPcdGet8 (PcdUartDefaultStopBits), > + }, > + gPcAnsiTerminal, > + gEndEntire > +}; > + > // > // Predefined platform default console device path > // > @@ -163,6 +207,17 @@ PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[] = { > } > }; > > +PLATFORM_CONSOLE_CONNECT_ENTRY gXenPlatformConsole[] = { > + { > + (EFI_DEVICE_PATH_PROTOCOL *)&gXenConsoleDevicePath, > + (CONSOLE_OUT | CONSOLE_IN | STD_ERROR) > + }, > + { > + NULL, > + 0 > + } > +}; > + > // > // Predefined platform connect sequence > // >