From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=jordan.l.justen@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 2A02F210C514F for ; Thu, 2 Aug 2018 23:42:30 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Aug 2018 23:42:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,437,1526367600"; d="scan'208";a="251373781" Received: from dmheuer-mobl.amr.corp.intel.com (HELO localhost) ([10.252.203.233]) by fmsmga006.fm.intel.com with ESMTP; 02 Aug 2018 23:42:29 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <153327854869.17436.13213153845980323214@jljusten-skl> From: Jordan Justen In-Reply-To: <20180803003045.31740-1-lersek@redhat.com> Cc: Ard Biesheuvel , Brijesh Singh References: <20180803003045.31740-1-lersek@redhat.com> User-Agent: alot/0.7 Date: Thu, 02 Aug 2018 23:42:28 -0700 Subject: Re: [PATCH] OvmfPkg/PlatformDebugLibIoPort: fix port detection for use in the DXE Core X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Aug 2018 06:42:31 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2018-08-02 17:30:45, Laszlo Ersek wrote: > The DXE Core is one of those modules that call > ProcessLibraryConstructorList() manually. > = > Before DxeMain() [MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c] calls > ProcessLibraryConstructorList(), and through it, our > PlatformDebugLibIoPortConstructor() function, DxeMain() invokes the > DEBUG() macro multiple times. That macro lands in our > PlatformDebugLibIoPortFound() function -- which currently relies on the > "mDebugIoPortFound" global variable that has (not yet) been set by the > constructor. As a result, early debug messages from the DXE Core are lost. > = > Move the device detection into PlatformDebugLibIoPortFound(), also caching > the fact (not just the result) of the device detection. > = > (We could introduce a separate DebugLib instance just for the DXE Core, > but the above approach works for all modules that currently consume the > PlatformDebugLibIoPort instance (which means "everything but SEC").) > = > This restores messages such as: > = > > CoreInitializeMemoryServices: > > BaseAddress - 0x7AF21000 Length - 0x3CDE000 MinimalMemorySizeNeeded -= 0x10F4000 > = > Keep the empty constructor function -- OVMF's DebugLib instances have > always had constructors; we had better not upset constructor dependency > ordering by making our instance(s) constructor-less. > = > Cc: Ard Biesheuvel > Cc: Brijesh Singh > Cc: Jordan Justen > Fixes: c09d9571300a089c35f5df2773b70edc25050d0d > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > = > Notes: > Brijesh, can you please test this patch on SEV, with and without > capturing the debug port? (In the first case, the debug log should ju= st > work; in the second case, the boot should remain fast.) Thanks! > = > Repo: https://github.com/lersek/edk2.git > Branch: debuglib_dxecore > = > OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c | 24 +++++++++++= +++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > = > diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c b/Ov= mfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c > index 81c44eece95f..74aef2e37b42 100644 > --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c > +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLibDetect.c > @@ -16,14 +16,26 @@ > #include > #include "DebugLibDetect.h" > = > + > +// > +// Set to TRUE if the debug I/O port has been checked > +// > +STATIC BOOLEAN mDebugIoPortChecked =3D FALSE; Could this be a static variable in the function? If not, should it be follow by a blank line? I agree that Brijesh should verify it, but pending that: Reviewed-by: Jordan Justen > // > // Set to TRUE if the debug I/O port is enabled > // > STATIC BOOLEAN mDebugIoPortFound =3D FALSE; > = > /** > - This constructor function checks if the debug I/O port device is prese= nt, > - caching the result for later use. > + This constructor function must not do anything. > + > + Some modules consuming this library instance, such as the DXE Core, in= voke > + the DEBUG() macro before they explicitly call > + ProcessLibraryConstructorList(). Therefore the auto-generated call from > + ProcessLibraryConstructorList() to this constructor function may be pr= eceded > + by some calls to PlatformDebugLibIoPortFound() below. Hence > + PlatformDebugLibIoPortFound() must not rely on anything this construct= or > + could set up. > = > @retval RETURN_SUCCESS The constructor always returns RETURN_SUCCESS. > = > @@ -34,12 +46,12 @@ PlatformDebugLibIoPortConstructor ( > VOID > ) > { > - mDebugIoPortFound =3D PlatformDebugLibIoPortDetect(); > return RETURN_SUCCESS; > } > = > /** > - Return the cached result of detecting the debug I/O port device. > + At the first call, check if the debug I/O port device is present, and = cache > + the result for later use. At subsequent calls, return the cached resul= t. > = > @retval TRUE if the debug I/O port device was detected. > @retval FALSE otherwise > @@ -51,5 +63,9 @@ PlatformDebugLibIoPortFound ( > VOID > ) > { > + if (!mDebugIoPortChecked) { > + mDebugIoPortFound =3D PlatformDebugLibIoPortDetect (); > + mDebugIoPortChecked =3D TRUE; > + } > return mDebugIoPortFound; > } > -- = > 2.14.1.3.gb7cf6e02401b >=20