From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: citrix.com, ip: 216.71.145.155, mailfrom: anthony.perard@citrix.com) Received: from esa3.hc3370-68.iphmx.com (esa3.hc3370-68.iphmx.com [216.71.145.155]) by groups.io with SMTP; Mon, 22 Jul 2019 10:07:04 -0700 Authentication-Results: esa3.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=anthony.perard@citrix.com; spf=Pass smtp.mailfrom=anthony.perard@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa3.hc3370-68.iphmx.com: no sender authenticity information available from domain of anthony.perard@citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa3.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="anthony.perard@citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa3.hc3370-68.iphmx.com: domain of anthony.perard@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa3.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="anthony.perard@citrix.com"; x-conformance=sidf_compatible; x-record-type="v=spf1"; x-record-text="v=spf1 ip4:209.167.231.154 ip4:178.63.86.133 ip4:195.66.111.40/30 ip4:85.115.9.32/28 ip4:199.102.83.4 ip4:192.28.146.160 ip4:192.28.146.107 ip4:216.52.6.88 ip4:216.52.6.188 ip4:162.221.158.21 ip4:162.221.156.83 ~all" Received-SPF: None (esa3.hc3370-68.iphmx.com: no sender authenticity information available from domain of postmaster@mail.citrix.com) identity=helo; client-ip=162.221.158.21; receiver=esa3.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: ucuFdCvgFoB5YDU07k1g/aZqlmqqVXCub5I96NpBeizuNgmvvZo6JGtZ3bBzS0ILuwcn2M8ZEk rTtHQXxhRKZuxRxjbAiAdV28BskupeGxtshXG2byGPTujddzYUk1Uh91f69B69oll4oCVsqlVs yOxqRmeQm7nAmDRv4oPHUryWRC5vPCaEAVZMzfYnt3B3Rk0fH/0iSIqv+76p/7pesObPc6Rlaf 7CIk1V5Pair+JPixRlq79Gfg3KfnutKsqhU5uXTUWHTokeHWxVFTm26DNWZEVjAy77kctFfr9a Ps4= X-SBRS: 2.7 X-MesageID: 3292009 X-Ironport-Server: esa3.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.64,295,1559534400"; d="scan'208";a="3292009" Date: Mon, 22 Jul 2019 18:06:43 +0100 From: "Anthony PERARD" To: Laszlo Ersek CC: , , Ard Biesheuvel , Jordan Justen , Julien Grall Subject: Re: [PATCH v3 32/35] OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn Message-ID: <20190722170643.GH1208@perard.uk.xensource.com> References: <20190704144233.27968-1-anthony.perard@citrix.com> <20190704144233.27968-33-anthony.perard@citrix.com> <5ce18fa6-100f-e792-199f-cdecf6b04177@redhat.com> MIME-Version: 1.0 In-Reply-To: <5ce18fa6-100f-e792-199f-cdecf6b04177@redhat.com> User-Agent: Mutt/1.12.1 (2019-06-15) Return-Path: anthony.perard@citrix.com Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline On Wed, Jul 10, 2019 at 12:48:57PM +0200, Laszlo Ersek wrote: > 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 > > --- > > 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 I'm going to add the following to the commit message: Since we use VENDOR_UART_DEVICE_PATH, fix its description and coding style. -- Anthony PERARD