From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org 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 7CE842035A7D2 for ; Thu, 16 Nov 2017 10:39:07 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5BC99861C; Thu, 16 Nov 2017 18:43:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-80.rdu2.redhat.com [10.10.120.80]) by smtp.corp.redhat.com (Postfix) with ESMTP id 47ED4614D7; Thu, 16 Nov 2017 18:43:14 +0000 (UTC) To: Paolo Bonzini Cc: edk2-devel@lists.01.org, Jordan Justen , Ard Biesheuvel References: <20171116104716.15144-1-pbonzini@redhat.com> <20171116104716.15144-2-pbonzini@redhat.com> From: Laszlo Ersek Message-ID: <01286b14-d1b0-04e0-086e-ed2d93d11384@redhat.com> Date: Thu, 16 Nov 2017 19:43:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171116104716.15144-2-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 16 Nov 2017 18:43:17 +0000 (UTC) Subject: Re: [PATCH 1/2] OvmfPkg: create a separate PlatformDebugLibIoPort instance for SEC X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Nov 2017 18:39:08 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/16/17 11:47, Paolo Bonzini wrote: > The next patch will want to add a global variable to > PlatformDebugLibIoPort, but this is not suitable for the SEC > phase, because SEC runs from read-only flash. The solution is > to have two library instances, one for SEC and another > for all other firmware phases. This patch adds the "plumbing" > for the SEC library instance, separating the INF files and > moving the constructor to a separate C source file. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Jordan Justen (Intel address) > Signed-off-by: Paolo Bonzini > --- > OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c | 15 ------- > .../PlatformDebugLibIoPort/DebugLibDetect.c | 32 +++++++++++++ > .../PlatformDebugLibIoPort/DebugLibDetectRom.c | 32 +++++++++++++ > .../PlatformDebugLibIoPort.inf | 1 + > .../PlatformRomDebugLibIoPort.inf | 52 ++++++++++++++++++++++ > OvmfPkg/OvmfPkgIa32.dsc | 2 +- > OvmfPkg/OvmfPkgIa32X64.dsc | 2 +- > OvmfPkg/OvmfPkgX64.dsc | 2 +- > 8 files changed, 120 insertions(+), 18 deletions(-) > create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c > create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c > create mode 100644 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf Very nice! I have a number of superficial comments. I've considered squashing these changes into your patch myself, but I didn't know what you'd prefer, so first I'll just list them. > diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c > index 5435767c1c..a5572a8eeb 100644 > --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c > +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c > @@ -29,21 +29,6 @@ > // > #define MAX_DEBUG_MESSAGE_LENGTH 0x100 > > -/** > - This constructor function does not have to do anything. > - > - @retval EFI_SUCCESS The constructor always returns RETURN_SUCCESS. > - > -**/ > -RETURN_STATUS > -EFIAPI > -PlatformDebugLibIoPortConstructor ( > - VOID > - ) > -{ > - return EFI_SUCCESS; > -} > - > /** > Prints a debug message to the debug output device if the specified error level is enabled. > (1) This is a pre-existent wart in the code. The return type is RETURN_STATUS, but we return EFI_SUCCESS instead of RETURN_SUCCESS. Numerically they are the same, but the message is different. RETURN_* is for library instances that have MODULE_TYPE=BASE, meaning that they can be built into modules that are more "primitive" than DXE_DRIVERs (such as SEC, PEIM, PEI_CORE, ...). This distinction primarily influences the constructor function's parameter list -- for MODULE_TYPE=BASE library instances, it is just VOID --, but it also dictates whether we should use EFI_xxx or RETURN_xxx as status codes. Can you please prepend a patch to the series that replaces EFI_SUCCESS with RETURN_SUCCESS in this function? (2) Similarly, as a "precursor cleanup", should not be included anywhere in this library instance. Namely, "MdePkg/Include/Uefi.h" has the following leading comment (rewrapped here for readability): > Root include file for Mde Package UEFI, UEFI_APPLICATION type modules. > > This is the include file for any module of type UEFI and > UEFI_APPLICATION. Uefi modules only use types defined via this include > file and can be ported easily to any environment. UEFI_DRIVER and UEFI_APPLICATION modules are the *least* primitive module types, while this library instance is on the other end of the spectrum. Now, "MdePkg/Include/Uefi.h" is just a convenience header for including: #include #include >>From these, "MdePkg/Include/Uefi/UefiBaseType.h" is totally fine to include wherever, while "MdePkg/Include/Uefi/UefiSpec.h" is the one that should be kept out of BASE lib classes. Thus, in the same prepended patch, can you please also try to replace with ? The lib instance should continue building just the same. (If it breaks, that means we have a layering violation in the code somewhere.) Consequently: > diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c > new file mode 100644 > index 0000000000..fee908861b > --- /dev/null > +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c > @@ -0,0 +1,32 @@ > +/** @file > + Constructor code for QEMU debug port library. > + Non-SEC instance. > + > + Copyright (c) 2017, Red Hat, Inc.
> + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php. > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include > +#include > + > +/** > + This constructor function does not have anything to do. > + > + @retval EFI_SUCCESS The constructor always returns RETURN_SUCCESS. > + > +**/ > +RETURN_STATUS > +EFIAPI > +PlatformDebugLibIoPortConstructor ( > + VOID > + ) > +{ > + return EFI_SUCCESS; > +} > diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c > new file mode 100644 > index 0000000000..407fe613ff > --- /dev/null > +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetectRom.c > @@ -0,0 +1,32 @@ > +/** @file > + Constructor code for QEMU debug port library. > + SEC instance. > + > + Copyright (c) 2017, Red Hat, Inc.
> + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php. > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include > +#include > + > +/** > + This constructor function does not have to do anything. > + > + @retval EFI_SUCCESS The constructor always returns RETURN_SUCCESS. > + > +**/ > +RETURN_STATUS > +EFIAPI > +PlatformRomDebugLibIoPortConstructor ( > + VOID > + ) > +{ > + return EFI_SUCCESS; > +} (3) In the above two files, should not be included, plus the constructors should return RETURN_SUCCESS. (4) It's a bit higher up in the patch (I'm mentioning it here only for keeping the previous points focused): if you diff the two new files against each other, you get - This constructor function does not have anything to do. + This constructor function does not have to do anything. Please consider unifying this. Then, we get to the INF files: > diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf > index 0e74fe94cb..65d8683f1f 100644 > --- a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf > +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf > @@ -30,6 +30,7 @@ > > [Sources] > DebugLib.c > + DebugLibDetect.c > > [Packages] > MdePkg/MdePkg.dec > diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf > new file mode 100644 > index 0000000000..93763d47dd > --- /dev/null > +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf > @@ -0,0 +1,52 @@ > +## @file > +# Instance of Debug Library for the QEMU debug console port. > +# It uses Print Library to produce formatted output strings. > +# > +# Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
> +# Copyright (c) 2017, Red Hat, Inc.
> +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php. > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = PlatformRomDebugLibIoPort > + FILE_GUID = CEB0D9D3-328F-4C24-8C02-28FA1986AE1B > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = DebugLib > + CONSTRUCTOR = PlatformRomDebugLibIoPortConstructor > + > +# > +# VALID_ARCHITECTURES = IA32 X64 IPF EBC > +# > + > +[Sources] > + DebugLib.c > + DebugLibDetectRom.c > + > +[Packages] > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseMemoryLib > + IoLib > + PcdLib > + PrintLib > + BaseLib > + DebugPrintErrorLevelLib > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdDebugIoPort ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel ## CONSUMES > + (5) This is totally my fault, but last night I couldn't think of it: The LIBRARY_CLASS assignment in the [Defines] section supports a syntax that restricts the library instance to specific client module types. Such as: LIBRARY_CLASS = DebugLib|SEC LIBRARY_CLASS = DebugLib|PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION (The module types need not be sorted "logically" within LIBRARY_CLASS, but I did that above for a pleasing read.) Such restrictions are helpful because then an invalid / unintended library class resolution in a DSC file is caught at build time; it won't get to cause weird behavior at runtime. Please consider adding these lists in v3. > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index c2f534fdbf..7ccb61147f 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -207,7 +207,7 @@ > !ifdef $(DEBUG_ON_SERIAL_PORT) > DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf > !else > - DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf > + DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf > !endif > ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf > ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 9f300a2e6f..237ec71b5e 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -212,7 +212,7 @@ > !ifdef $(DEBUG_ON_SERIAL_PORT) > DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf > !else > - DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf > + DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf > !endif > ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf > ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 1ffcf37f8b..a5047fa38e 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -212,7 +212,7 @@ > !ifdef $(DEBUG_ON_SERIAL_PORT) > DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf > !else > - DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf > + DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf > !endif > ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf > ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf > This is fine, but I'll use the opportunity to ask you to tweak the git config of your edk2 clone a bit :) The below hints are from . (6) Looking at the above hunks, it is not readily apparent to the reviewer what section of the DSC file is being patched. It can be improved: (6a) run: git config diff.ini.xfuncname '^\[[A-Za-z0-9_., ]+]' (6b) add the following to ".git/info/attributes": *.dec diff=ini *.dsc diff=ini *.dsc.inc diff=ini *.fdf diff=ini *.fdf.inc diff=ini *.inf diff=ini As a result, the hunks will look like: > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index c2f534fdbf3b..7ccb61147f27 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -207,7 +207,7 @@ [LibraryClasses.common.SEC] > !ifdef $(DEBUG_ON_SERIAL_PORT) > DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf > !else > - DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf > + DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf > !endif > ReportStatusCodeLib|MdeModulePkg/Library/PeiReportStatusCodeLib/PeiReportStatusCodeLib.inf > ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf Note "[LibraryClasses.common.SEC]" after the "@@" marker. (Yes, we could carry a .gitattributes file in the tree, but that would require us to agree upon it.) (7) Consider creating a file called "edk2.diff.order", with the following contents: *.dec *.dsc.inc *.dsc *.fdf.inc *.fdf *.inf *.h *.vfr *.c and setting the "diff.orderFile" config knob to the pathname of that file. Then the reviewer will get to see hunks in the following order: - package declaration file changes, - platform DSC and flash description changes, - module INF changes - C header changes, - VFR (visual form representation) changes, - finally, C language changes. (Yes, we could carry an order file in the tree as well, but my attempt to do it for QEMU had failed, so I didn't try posting it for edk2.) I hope you aren't overly annoyed by the above wall of text. Do you have time for a v3? Thank you! Laszlo