From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 C244C1A1DF6 for ; Tue, 23 Aug 2016 08:33:53 -0700 (PDT) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 35000C04B325; Tue, 23 Aug 2016 15:33:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-95.phx2.redhat.com [10.3.116.95]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7NFXquN020784; Tue, 23 Aug 2016 11:33:52 -0400 To: "Zeng, Star" , "Fan, Jeff" , "edk2-devel@ml01.01.org" References: <1470128388-17960-1-git-send-email-jeff.fan@intel.com> <1470128388-17960-49-git-send-email-jeff.fan@intel.com> <4f61b2b4-eeb4-8435-412f-20848347c88e@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A143D9CFD@shsmsx102.ccr.corp.intel.com> <795dd4fe-cd16-c0f1-7f04-f78601e2c7a8@redhat.com> <82c6b5c9-dcab-f3c1-5ffe-20fb27ddb1af@intel.com> <6cd44902-8c07-8f76-1204-d79c85559ae5@intel.com> Cc: "Kinney, Michael D" , "Tian, Feng" From: Laszlo Ersek Message-ID: Date: Tue, 23 Aug 2016 11:33:52 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <6cd44902-8c07-8f76-1204-d79c85559ae5@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 23 Aug 2016 15:33:53 +0000 (UTC) Subject: Re: [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVariableArchProtocolGuid dependency X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Aug 2016 15:33:54 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 08/18/16 22:57, Zeng, Star wrote: > On 2016/8/19 10:45, Zeng, Star wrote: >> On 2016/8/19 10:26, Laszlo Ersek wrote: >>> On 08/19/16 04:00, Fan, Jeff wrote: >>>> Laszlo, >>>> >>>> I could revert this patch firstly. >>> >>> Thank you, that would be very kind. >>> >>>> Could you please dig out the OVMF to address the potential issue, >>>> then I could re-commit it for those platforms required this patch. >>> >>> The problem is that this week (what remains of it) and the next week >>> I won't really have time for this -- tomorrow I'm hoping to finish >>> up something else in a mortal hurry. It was actually in preparation >>> for rebasing / pushing that work that I pulled "master", and found >>> out about the regression. >>> >>> Can we perhaps get help from the variable stack maintainers? What's >>> the design of the (lack of) depexes on those drivers? >> >> Variable driver consumes >> PcdFlashNvStorageVariableBase(64)/PcdFlashNvStorageVariableSize to >> produce *gEfiVariableArchProtocolGuid* protocol. Variable driver >> registers (SMM)FTW protocol notification function >> SmmFtwNotificationEvent() or FtwNotificationEvent() to produce >> *gEfiVariableWriteArchProtocolGuid* protocol. (SMM)FTW driver has >> dependency on gEfiSmmFirmwareVolumeBlockProtocolGuid or >> gEfiFirmwareVolumeBlockProtocolGuid. >> >> I am not so sure what you said about the (lack of) depexes on those >> drivers. >> >> Did you see variable driver loaded and started when ASSERT appeared >> on OVMF? > > > You may could compare the serial logs to get if there is some driver > execution flow differences for the images built without and with this > patch. The only difference is that in the working case, PiSmmCpuDxeSmm.efi is dispatched immediately before FvbServicesSmm.efi, while in the broken case, PiSmmCpuDxeSmm.efi is not dispatched (it is delayed due to the new depex) and FvbServicesSmm.efi is dispatched without PiSmmCpuDxeSmm.efi being present, and it breaks. I see that PiSmmCpuDxeSmm.efi installs: - gEfiSmmConfigurationProtocolGuid, - EfiSmmCpuProtocol, - EfiSmmCpuServiceProtocol, so it might not be hard to add a depex on one of those (DXE/SMM) protocols to FvbServicesSmm.efi. In particular, the PI 1.4a spec says in Volume 4, "1.6.1 SMM Drivers": the dependency expression in the file section EFI_SECTION_SMM_DEPEX [...] can refer to both UEFI and SMM protocols so we could easily make FvbServicesSmm.efi dependent on either of those protocols, I think. However, is that the official way to delay the entry point function of a DXE_SMM_DRIVER module until the code would actually run in SMM? Section "1.7 SMM Driver Initialization" says: An SMM Driver's initialization phase begins when the driver has been loaded into SMRAM and its entry point is called. An SMM Driver's initialization phase ends when the entry point returns. During SMM Driver initialization, SMM Drivers have access to two sets of protocols: UEFI and SMM. UEFI protocols are those which are installed and discovered using the UEFI Boot Services. UEFI protocols can be located and used by SMM drivers only during SMM Initialization. SMM protocols are those which are installed and discovered using the System Management Services Table (SMST). SMM protocols can be discovered by SMM drivers during initialization time and accessed while inside of SMM. These paragraphs seem to imply that the processor will execute the entry point function of a DXE_SMM_DRIVER module from SMRAM, without the processor necessarily being in SMM just yet. (Which further implies that SMRAM will not have been closed and locked at that point, but that's a side remark only.) This seems reasonable to me, but in the entry point of OVMF's FvbServicesSmm.efi, we specifically need to test a write access to the pflash chip, to make sure that the QEMU configuration is suitable and secure. For this, FvbServicesSmm.efi must be dispatched in SMM. And, apparently, in the current tree, when PiSmmCpuDxe launches first, FvbServicesSmm *is* dispatched in SMM. I find it quite non-intuitive that a DXE_SMM_DRIVER's entry point may or may not be executed in SMM, but more importantly, what's the best way to delay the driver dispatch until after PiSmmCpuDxe has been dispatched? The PiSmmIpl driver (a DXE_RUNTIME_DRIVER) produces the EFI_SMM_BASE2_PROTOCOL, which has a member function called InSmm(). However: - according to the documentation, it doesn't test for SMM, it tests for being executed from SMRAM (which are two different things!), and it only really makes sense for SMM/DXE combined drivers, - in fact I don't need a query-like function here, for the code, but likely a new depex for FvbServicesSmm that delays its dispatch until after PiSmmCpuDxe. Actually, I think the simplest way to solve this in OVMF is to add a depex on EFI_SMM_CPU_PROTOCOL to FvbServicesSmm. From the spec: Provides access to CPU-related information while in SMM. [...] This protocol allows SMM drivers to access architecture-standard registers from any of the CPU save state areas. [...] I think a DXE_SMM_DRIVER might perfectly want to use this protocol in its entry point function (in the general case, for whatever reason), so it seems very suitable for delaying FvbServicesSmm. For example, in "QuarkPlatformPkg/Acpi/DxeSmm/SmmPowerManagement/SmmPowerManagement.inf", I think we see the same pattern: - MODULE_TYPE = DXE_SMM_DRIVER - Depex on gEfiSmmCpuProtocolGuid ... Okay, I tried to test this patch (in combination with re-adding the gEfiVariableArchProtocolGuid dependency to PiSmmCpuDxeSmm): > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf > index ba2d3679a46d..a241f7702ca2 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf > @@ -88,4 +88,4 @@ [FeaturePcd] > gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > > [Depex] > - TRUE > + gEfiSmmCpuProtocolGuid With this patch, in the build report I get: > Module Name: FvbServicesSmm > Module INF Path: OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf > > [...] > > >----------------------------------------------------------------------------------------------------------------------< > Final Dependency Expression (DEPEX) Instructions > PUSH gEfiSmmCpuProtocolGuid > PUSH gEfiPcdProtocolGuid > PUSH gEfiSmmBase2ProtocolGuid > PUSH gEfiSmmAccess2ProtocolGuid > PUSH gEfiDevicePathUtilitiesProtocolGuid > AND > AND > AND > AND > END > ------------------------------------------------------------------------------------------------------------------------ > Dependency Expression (DEPEX) from INF > (gEfiSmmCpuProtocolGuid) AND (gEfiPcdProtocolGuid) AND (gEfiSmmBase2ProtocolGuid) AND (gEfiSmmAccess2ProtocolGuid) AND > (gEfiDevicePathUtilitiesProtocolGuid) > ------------------------------------------------------------------------------------------------------------------------ > From Module INF: gEfiSmmCpuProtocolGuid > From Library INF: (gEfiPcdProtocolGuid) AND (gEfiSmmBase2ProtocolGuid) AND (gEfiSmmAccess2ProtocolGuid) AND > (gEfiDevicePathUtilitiesProtocolGuid) > <----------------------------------------------------------------------------------------------------------------------> and FvbServicesSmm *is* appropriately delayed. However, the variable driver blows up: > Loading SMM driver at 0x0007FF7D000 EntryPoint=0x0007FF7D271 VariableSmm.efi > mSmmMemLibInternalMaximumSupportAddress = 0xFFFFFFFFF > VarCheckLibRegisterSetVariableCheckHandler - 0x7FF89334 Success > Firmware Volume for Variable Store is corrupted > > ASSERT_EFI_ERROR (Status = Volume Corrupt) > ASSERT MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c(927): !EFI_ERROR (Status) This is the call tree that fails: VariableServiceInitialize() [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] VariableCommonInitialize() [MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c] InitNonVolatileVariableStore() [MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c] However, the variable store is definitely not corrupted; this failure reproduces with the pristine "OVMF_VARS.fd" varstore template that falls right out of the OVMF build. Thanks! Laszlo >> Thanks, >> Star >> >>> >>> Anyway, if you could live with the patch reverted for one or two weeks, >>> then reverting it would be best, IMO. It results in a regression, even >>> if ultimately it might only expose a bug in something else. >>> >>> Thanks! >>> Laszlo >>> >>> >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>>> Of Laszlo Ersek >>>> Sent: Friday, August 19, 2016 9:19 AM >>>> To: Fan, Jeff; edk2-devel@ml01.01.org >>>> Cc: Kinney, Michael D; Tian, Feng >>>> Subject: Re: [edk2] [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add >>>> gEfiVariableArchProtocolGuid dependency >>>> >>>> On 08/02/16 10:59, Jeff Fan wrote: >>>>> PiSmmCpuDxeSmm driver's entry point will get some PCDs supported >>>>> dynamic type. >>>>> In case those PCDs are set as DynamicHii type in platform DSC File, it >>>>> implies that EFI Variable Arch protocol is required. >>>>> >>>>> This fix is to add gEfiVariableArchProtocolGuid dependency on >>>>> PiSmmCpuDxeSmm driver to make sure those DynamicHii PCDs could be >>>>> read correctly. >>>>> >>>>> Cc: Michael Kinney >>>>> Cc: Feng Tian >>>>> Cc: Giri P Mudusuru >>>>> Cc: Laszlo Ersek >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: Jeff Fan >>>>> Reviewed-by: Giri P Mudusuru >>>>> --- >>>>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf >>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf >>>>> index d7e6e07..83e3c55 100644 >>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf >>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf >>>>> @@ -4,7 +4,7 @@ >>>>> # This SMM driver performs SMM initialization, deploy SMM Entry >>>>> Vector, # provides CPU specific services in SMM. >>>>> # >>>>> -# Copyright (c) 2009 - 2015, Intel Corporation. All rights >>>>> reserved.
>>>>> +# Copyright (c) 2009 - 2016, Intel Corporation. All rights >>>>> +reserved.
>>>>> # >>>>> # This program and the accompanying materials # are licensed and >>>>> made available under the terms and conditions of the BSD License @@ >>>>> -155,7 +155,7 @@ [Pcd] >>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode >>>>> ## CONSUMES >>>>> >>>>> [Depex] >>>>> - gEfiMpServiceProtocolGuid >>>>> + gEfiMpServiceProtocolGuid AND gEfiVariableArchProtocolGuid >>>>> >>>>> [UserExtensions.TianoCore."ExtraFiles"] >>>>> PiSmmCpuDxeSmmExtra.uni >>>>> >>>> >>>> This patch (commit 7503cd70fb86) breaks OVMF. From the OVMF log: >>>> >>>> Loading SMM driver at 0x0007FFDA000 EntryPoint=0x0007FFDA271 >>>> FvbServicesSmm.efi QEMU Flash: Attempting flash detection at FFE00010 >>>> QemuFlashDetected => FD behaves as ROM QemuFlashDetected => No ASSERT >>>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c(248): >>>> !_gPcd_FixedAtBuild_PcdSmmSmramRequire >>>> >>>> This symptom means that the SMM build of the >>>> QemuFlashFvbServicesRuntimeDxe driver is not actually running in SMM >>>> when it tries to access the pflash chip at startup. Therefore QEMU >>>> prevents the access, and then OVMF gives up. >>>> >>>> Here's the bisection log: >>>> >>>> git bisect start >>>> # good: [de74668f5ea713b7e91e01318f0d15d2bf0effce] >>>> MdeModulePkg/PeiCore: Fix ConverSinglePpiPointer () typo. >>>> git bisect good de74668f5ea713b7e91e01318f0d15d2bf0effce >>>> # bad: [7822a1d91d53e80525f571183a24d54488f5437f] >>>> NetworkPkg/IpSecDxe: Fix wrong IKE header "FLAG" update git bisect >>>> bad 7822a1d91d53e80525f571183a24d54488f5437f >>>> # good: [8a2d564b2a811b8dbc85f90e14a67ae4ade21201] >>>> UefiCpuPkg/MpInitLib: Sort processor by ascending order of APIC ID >>>> git bisect good 8a2d564b2a811b8dbc85f90e14a67ae4ade21201 >>>> # good: [7fadaacd50d716e8e054a94c20db56cca98e961e] UefiCpuPkg/CpuDxe: >>>> Consume MpInitLib to produce CPU MP Protocol services git bisect good >>>> 7fadaacd50d716e8e054a94c20db56cca98e961e >>>> # bad: [a012df5ec643a0c08c2b723a02919a5c9373ca74] PcAtChipsetPkg >>>> AcpiTimerLib: Wait 363 ACPI timer counts to get TSC Freq git bisect >>>> bad a012df5ec643a0c08c2b723a02919a5c9373ca74 >>>> # good: [51d4779d7bfb74bfdbb0e196846de78567127348] >>>> MdePkg/MpService.h: Fixed typo in function header to match PI spec >>>> git bisect good 51d4779d7bfb74bfdbb0e196846de78567127348 >>>> # good: [f3b91fa04adea2389c5a6d0fbd9a584d149bae09] UefiCpuPkg/CpuDxe: >>>> Fixed typo in function header to match PI spec git bisect good >>>> f3b91fa04adea2389c5a6d0fbd9a584d149bae09 >>>> # bad: [7503cd70fb864a5663edb121c9b2488b4c69e7f5] >>>> UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVariableArchProtocolGuid >>>> dependency git bisect bad 7503cd70fb864a5663edb121c9b2488b4c69e7f5 >>>> # first bad commit: [7503cd70fb864a5663edb121c9b2488b4c69e7f5] >>>> UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVariableArchProtocolGuid dependency >>>> >>>> I see that this patch appeared in the final, v5 version of the >>>> series, as the last patch in the series. I never got around testing >>>> v5. I request that we please revert it for now, and then we >>>> investigate the problem. >>>> >>>> From the time we worked on SMM enablement for OVMF, I recall that >>>> pulling in the SMM variable driver stack was tricky. The three layers >>>> (Firmware Volume Block (2), Fault Tolerant Write, and Variable) have >>>> apparent / implicit dependencies between them, but these are not >>>> actually expressed in DepExes. I don't know / don't remember why that >>>> is the case, but I recall that I had to fiddle with their inclusion >>>> order in the FDF files. Following the inclusion order of the >>>> preexistent, SMM-less variable driver stack made it all work, if I >>>> remember correctly. >>>> >>>> I don't know why those depexes are not spelled out explicitly in >>>> those drivers, but at this point I think that the new DepEx on >>>> PiSmmCpuDxeSmm.inf upsets the dispatch order, and things break. I >>>> fully agree that this should hopefully be fixed by spelling out all >>>> DepExes explicitly everywhere, and I can also imagine it is actually >>>> a bug in OVMF somewhere, but for now, until we figure it out, can we >>>> please revert the patch? >>>> >>>> The commit carries Mike's Tested-by -- I wonder what the differences >>>> are between Mike's test platform and OVMF. >>>> >>>> Thank you, >>>> Laszlo >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel