Re: [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add gEfiVariableArchProtocolGuid dependency

["Zeng, Star" <star.zeng@intel.com>]

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 <michael.d.kinney@intel.com>
>>>> Cc: Feng Tian <feng.tian@intel.com>
>>>> Cc: Giri P Mudusuru <giri.p.mudusuru@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
>>>> Reviewed-by: Giri P Mudusuru <giri.p.mudusuru@intel.com>
>>>> ---
>>>>  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.<BR>
>>>> +# Copyright (c) 2009 - 2016, Intel Corporation. All rights
>>>> +reserved.<BR>
>>>>  #
>>>>  # 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
>>>