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; Tue, 23 Jul 2019 00:31:55 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EF63085539; Tue, 23 Jul 2019 07:31:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-52.ams2.redhat.com [10.36.117.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id 489745D9DE; Tue, 23 Jul 2019 07:31:53 +0000 (UTC) Subject: Re: [PATCH v3 32/35] OvmfPkg/PlatformBootManagerLib: Use a Xen console for ConOut/ConIn To: Anthony PERARD Cc: devel@edk2.groups.io, 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> <5ce18fa6-100f-e792-199f-cdecf6b04177@redhat.com> <20190722170643.GH1208@perard.uk.xensource.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 23 Jul 2019 09:31:52 +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: <20190722170643.GH1208@perard.uk.xensource.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 23 Jul 2019 07:31:55 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/22/19 19:06, Anthony PERARD wrote: > 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. > > Thanks! Laszlo