From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by ml01.01.org (Postfix) with ESMTP id EF3E81A1E02 for ; Thu, 18 Aug 2016 19:57:44 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 18 Aug 2016 19:57:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,543,1464678000"; d="scan'208";a="1038242483" Received: from shzintpr01.sh.intel.com (HELO [10.7.209.21]) ([10.239.4.80]) by orsmga002.jf.intel.com with ESMTP; 18 Aug 2016 19:57:45 -0700 To: Laszlo Ersek , "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> Cc: "Kinney, Michael D" , "Tian, Feng" , star.zeng@intel.com From: "Zeng, Star" Message-ID: <6cd44902-8c07-8f76-1204-d79c85559ae5@intel.com> Date: Fri, 19 Aug 2016 10:57:12 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <82c6b5c9-dcab-f3c1-5ffe-20fb27ddb1af@intel.com> 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: Fri, 19 Aug 2016 02:57:45 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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. Thanks, Star > > > 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 >>>