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

2016-09-01 Thread Fan, Jeff 
Laszlo,

There is no changing request on Ovmf and UefiCpuPkg SmmCpuFeaturesLib instances.

Jeff

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Friday, September 02, 2016 1:47 AM
To: Fan, Jeff; Zeng, Star; edk2-de...@ml01.01.org
Cc: Kinney, Michael D; Justen, Jordan L; Tian, Feng
Subject: Re: [edk2] [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add 
gEfiVariableArchProtocolGuid dependency

On 09/01/16 03:11, Fan, Jeff wrote:
> Laszlo,
> 
> UefiCpuPkg/PiSmmCpuDxeSmm driver and
> UefiCpuPkg/Library/SmmCpuFeatuersLib have no such requirement on 
> gEfiVariableArchProtocolGuid  to access HII type PCD. In fact, our 
> platform SmmCpuFeaturesLib instance (linked by PiSmmCpuDxeSmm) is 
> trying to read HII type PCD.
> 
> The correct solution is to add gEfiVariableArchProtocolGuid dependency 
> in platform SmmCpuFeaturesLib instance and this dependency will be 
> inherited by PiSmmCpuDxeSmm driver.

OVMF also has its own SmmCpuFeaturesLib instance, under 
"OvmfPkg/Library/SmmCpuFeaturesLib". That library instance has no particular 
depex, similarly to UefiCpuPkg's instance.

If you add the depex to your platform's SmmCpuFeaturesLib instance, that should 
keep both UefiCpuPkg's and OvmfPkg's instance unchanged. That sounds great to 
me, thank you!

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2016-09-01 Thread Laszlo Ersek 
On 09/01/16 03:11, Fan, Jeff wrote:
> Laszlo,
> 
> UefiCpuPkg/PiSmmCpuDxeSmm driver and
> UefiCpuPkg/Library/SmmCpuFeatuersLib have no such requirement on
> gEfiVariableArchProtocolGuid  to access HII type PCD. In fact, our
> platform SmmCpuFeaturesLib instance (linked by PiSmmCpuDxeSmm) is
> trying to read HII type PCD.
> 
> The correct solution is to add gEfiVariableArchProtocolGuid
> dependency in platform SmmCpuFeaturesLib instance and this dependency
> will be inherited by PiSmmCpuDxeSmm driver.

OVMF also has its own SmmCpuFeaturesLib instance, under
"OvmfPkg/Library/SmmCpuFeaturesLib". That library instance has no
particular depex, similarly to UefiCpuPkg's instance.

If you add the depex to your platform's SmmCpuFeaturesLib instance, that
should keep both UefiCpuPkg's and OvmfPkg's instance unchanged. That
sounds great to me, thank you!

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2016-08-31 Thread Fan, Jeff 
Laszlo,

UefiCpuPkg/PiSmmCpuDxeSmm driver and UefiCpuPkg/Library/SmmCpuFeatuersLib have 
no such requirement on gEfiVariableArchProtocolGuid  to access HII type PCD.
In fact, our platform SmmCpuFeaturesLib instance (linked by PiSmmCpuDxeSmm) is 
trying to read HII type PCD.

The correct solution is to add gEfiVariableArchProtocolGuid  dependency in 
platform SmmCpuFeaturesLib instance and this dependency will be inherited by 
PiSmmCpuDxeSmm driver.

We will drop this patch #48 eventually.  It has been reverted suggested by you! 
:-)

Thanks your support on it.

Jeff

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Tuesday, August 30, 2016 9:46 PM
To: Fan, Jeff; Zeng, Star; edk2-de...@ml01.01.org
Cc: Kinney, Michael D; Justen, Jordan L; Tian, Feng
Subject: Re: [edk2] [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add 
gEfiVariableArchProtocolGuid dependency

On 08/25/16 10:00, Fan, Jeff wrote:
> Laszlo,
> 
> After discussed with Star, I understood OVMF's circle dependency on Variable 
> Arch protocol and SMM CPU driver.
> 
> I will defer to check-in this patch till found the better solution.

Thank you!

> Before the proper fix adopted, our platforms might not set the HII types 
> dynamic PCDs used by PiSmmCpuDxeSmm driver.

If I understand correctly, such dynamic PCDs are stored in UEFI variables, 
which are in turn exposed via HII (forms). Based on 
"MdeModulePkg/Universal/PCD/Dxe/Pcd.inf", point 2.3 (b).

It seems to me that "Default Storage" vs. "Variable Storage" is only separated 
in the platform DSC file; the declaration for the dynamic PCDs in question 
should be identical in the package DEC file.

So maybe PiSmmCpuDxeSmm should only read the new dynamic PCDs, and if those are 
expected to come from UEFI variables, then the platform could plug a NULL 
library instance into PiSmmCpuDxeSmm that "imbued"
PiSmmCpuDxeSmm with a depex on gEfiVariableArchProtocolGuid.

Alternatively, maybe SmmCpuPlatformHookLib could be extended with a new 
function (or functions) for querying these dynamic settings. OvmfPkg could 
provide a library instance that returned constant defaults (or even dynamic 
PCDs, but without variable access). And UefiCpuPkg could provide a library 
instance with variable-backed PCDs, plus the depex.

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2016-08-30 Thread Laszlo Ersek 
On 08/25/16 10:00, Fan, Jeff wrote:
> Laszlo,
> 
> After discussed with Star, I understood OVMF's circle dependency on Variable 
> Arch protocol and SMM CPU driver.
> 
> I will defer to check-in this patch till found the better solution.

Thank you!

> Before the proper fix adopted, our platforms might not set the HII types 
> dynamic PCDs used by PiSmmCpuDxeSmm driver.

If I understand correctly, such dynamic PCDs are stored in UEFI
variables, which are in turn exposed via HII (forms). Based on
"MdeModulePkg/Universal/PCD/Dxe/Pcd.inf", point 2.3 (b).

It seems to me that "Default Storage" vs. "Variable Storage" is only
separated in the platform DSC file; the declaration for the dynamic PCDs
in question should be identical in the package DEC file.

So maybe PiSmmCpuDxeSmm should only read the new dynamic PCDs, and if
those are expected to come from UEFI variables, then the platform could
plug a NULL library instance into PiSmmCpuDxeSmm that "imbued"
PiSmmCpuDxeSmm with a depex on gEfiVariableArchProtocolGuid.

Alternatively, maybe SmmCpuPlatformHookLib could be extended with a new
function (or functions) for querying these dynamic settings. OvmfPkg
could provide a library instance that returned constant defaults (or
even dynamic PCDs, but without variable access). And UefiCpuPkg could
provide a library instance with variable-backed PCDs, plus the depex.

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2016-08-25 Thread Fan, Jeff 
Laszlo,

After discussed with Star, I understood OVMF's circle dependency on Variable 
Arch protocol and SMM CPU driver.

I will defer to check-in this patch till found the better solution.

Before the proper fix adopted, our platforms might not set the HII types 
dynamic PCDs used by PiSmmCpuDxeSmm driver.

Thanks!
Jeff

-Original Message-
From: Zeng, Star 
Sent: Wednesday, August 24, 2016 9:42 PM
To: Laszlo Ersek; Fan, Jeff; edk2-de...@ml01.01.org
Cc: Kinney, Michael D; Justen, Jordan L; Tian, Feng
Subject: Re: [edk2] [Patch v5 48/48] UefiCpuPkg/PiSmmCpuDxeSmm: Add 
gEfiVariableArchProtocolGuid dependency

[Snipped]

>>>>
>>>> I am not so clear about "the pristine "OVMF_VARS.fd" varstore 
>>>> template that falls right out of the OVMF build".
>>>>
>>>> Variable driver depends on PcdFlashNvStorageVariableBase(64) be set 
>>>> correctly to produce gEfiVariableArchProtocolGuid protocol.
>>>>
>>>> After PiSmmCpuDxeSmm adds gEfiVariableArchProtocolGuid dependency 
>>>> and FvbServicesSmm adds gEfiSmmCpuProtocolGuid dependency, there 
>>>> will be a dependency circle below.
>>>>  - PiSmmCpuDxeSmm depends on Variable driver to produce 
>>>> gEfiVariableArchProtocolGuid protocol.
>>>>  - FvbServicesSmm depends on PiSmmCpuDxeSmm to produce 
>>>> gEfiSmmCpuProtocolGuid protocol.
>>>>  - Variable driver depends on FvbServicesSmm to set
>>>> PcdFlashNvStorageVariableBase(64) PCD.
>>>
>>> I understand, thank you for the explanation. The 64-bit PCD that you 
>>> mention is set at the end of the entry point function.
>>>
>>>> Are below approaches possible in OVMF?
>>>> 1. Do InitializeVariableFvHeader () and
>>>> PcdFlashNvStorageVariableBase(64) PCD set at PEI phase.
>>>
>>> InitializeVariableFvHeader() writes to the pflash chip, which is 
>>> only possible if the code runs in SMM (under the use case we are 
>>> discussing now). So running it as part of a PEIM would not be right.
>>>
>>>> 2. Do InitializeVariableFvHeader () and
>>>> PcdFlashNvStorageVariableBase(64) PCD set in entrypoint with 
>>>> dependency = TRUE, and do other part of code in FvbInitialize() in 
>>>> another gEfiSmmCpuProtocolGuid notification function?
>>>
>>> That's again not good, because it would allow the entry point 
>>> function to exit with success (and to produce the FVB protocol 
>>> interface) even if the pflash configuration on the QEMU command line 
>>> is incorrect (that is, a -D SMM_REQUIRE OVMF build is being run with 
>>> "-bios", rather than the pflash chips).
>>
>> I am a little curious about what makes "writing to the pflash chip is 
>> only possible if the code runs in SMM" in OVMF?
>
> QEMU does that.
>
> That is the way QEMU protects the pflash chip from direct writes by 
> the guest OS. If you pass
>
>   -global driver=cfi.pflash01,property=secure,value=on
>
> to QEMU (which we do), then all write accesses to the chip will be 
> rejected, unless the code doing the write access runs in SMM.
>
>> I meant the code flow for approach 2) I mentioned is like below, I am 
>> not sure if it works or not. :)
>>
>> FvbInitialize() - This may could be done at PEI phase.
>>   InitializeVariableFvHeader()
>> if ValidateFvHeader () returns error status # mark1
>>   allocate buffer to hold data from GetFvbInfo() # mark2
>> endif
>> return PcdOvmfFlashNvStorageVariableBase or the buffer
>>   (allocated at mark2) pointer
>>   set PcdFlashNvStorageVariableBase64/
>>   PcdFlashNvStorageFtwWorkingBase/
>>   PcdFlashNvStorageFtwSpareBase
>>   according to the return from InitializeVariableFvHeader()
>>
>> FvbWriteInitialize() - Notification function on gEfiSmmCpuProtocolGuid
>>   QemuFlashInitialize()
>>   ...
>>   Free buffer(allocated at mark2)
>>   set PcdFlashNvStorageVariableBase64/
>>   PcdFlashNvStorageFtwWorkingBase/
>>   PcdFlashNvStorageFtwSpareBase
>>   again if mark1 returned success
>>   Install FVB protocol
>>   ...
>
> I don't understand the purpose of the buffer allocated at mark2.
>
> Also, currently InitializeVariableFvHeader() erases blocks, after it 
> prints "Variable FV header is not valid. It will be reinitialized". As 
> I said, that part cannot be done in PEI.
>
>  Anyway, I just tried to set PcdFlashNvStorageVariableBase64 / 
> PcdFlashNvStorageFtwWorkingBase / PcdFlashNvStorageFtwS

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

2016-08-24 Thread Zeng, Star 

[Snipped]



I am not so clear about "the pristine "OVMF_VARS.fd" varstore template
that falls right out of the OVMF build".

Variable driver depends on PcdFlashNvStorageVariableBase(64) be set
correctly to produce gEfiVariableArchProtocolGuid protocol.

After PiSmmCpuDxeSmm adds gEfiVariableArchProtocolGuid dependency and
FvbServicesSmm adds gEfiSmmCpuProtocolGuid dependency, there will be a
dependency circle below.
 - PiSmmCpuDxeSmm depends on Variable driver to produce
gEfiVariableArchProtocolGuid protocol.
 - FvbServicesSmm depends on PiSmmCpuDxeSmm to produce
gEfiSmmCpuProtocolGuid protocol.
 - Variable driver depends on FvbServicesSmm to set
PcdFlashNvStorageVariableBase(64) PCD.


I understand, thank you for the explanation. The 64-bit PCD that you
mention is set at the end of the entry point function.


Are below approaches possible in OVMF?
1. Do InitializeVariableFvHeader () and
PcdFlashNvStorageVariableBase(64) PCD set at PEI phase.


InitializeVariableFvHeader() writes to the pflash chip, which is only
possible if the code runs in SMM (under the use case we are discussing
now). So running it as part of a PEIM would not be right.


2. Do InitializeVariableFvHeader () and
PcdFlashNvStorageVariableBase(64) PCD set in entrypoint with dependency
= TRUE, and do other part of code in FvbInitialize() in another
gEfiSmmCpuProtocolGuid notification function?


That's again not good, because it would allow the entry point function
to exit with success (and to produce the FVB protocol interface) even if
the pflash configuration on the QEMU command line is incorrect (that is,
a -D SMM_REQUIRE OVMF build is being run with "-bios", rather than the
pflash chips).


I am a little curious about what makes "writing to the pflash chip is
only possible if the code runs in SMM" in OVMF?


QEMU does that.

That is the way QEMU protects the pflash chip from direct writes by the
guest OS. If you pass

  -global driver=cfi.pflash01,property=secure,value=on

to QEMU (which we do), then all write accesses to the chip will be
rejected, unless the code doing the write access runs in SMM.


I meant the code flow for approach 2) I mentioned is like below, I am
not sure if it works or not. :)

FvbInitialize() - This may could be done at PEI phase.
  InitializeVariableFvHeader()
if ValidateFvHeader () returns error status # mark1
  allocate buffer to hold data from GetFvbInfo() # mark2
endif
return PcdOvmfFlashNvStorageVariableBase or the buffer
  (allocated at mark2) pointer
  set PcdFlashNvStorageVariableBase64/
  PcdFlashNvStorageFtwWorkingBase/
  PcdFlashNvStorageFtwSpareBase
  according to the return from InitializeVariableFvHeader()

FvbWriteInitialize() - Notification function on gEfiSmmCpuProtocolGuid
  QemuFlashInitialize()
  ...
  Free buffer(allocated at mark2)
  set PcdFlashNvStorageVariableBase64/
  PcdFlashNvStorageFtwWorkingBase/
  PcdFlashNvStorageFtwSpareBase
  again if mark1 returned success
  Install FVB protocol
  ...


I don't understand the purpose of the buffer allocated at mark2.

Also, currently InitializeVariableFvHeader() erases blocks, after it
prints "Variable FV header is not valid. It will be reinitialized". As I
said, that part cannot be done in PEI.

 Anyway, I just tried to set PcdFlashNvStorageVariableBase64 /
PcdFlashNvStorageFtwWorkingBase / PcdFlashNvStorageFtwSpareBase right in
the FDF file, at build time -- that is, even earlier than in PEI --, to
the correct values, as dynamic defaults.

(I even verified those values in the build report file.)

It doesn't work: I get the exact same assertion failure. In other words,
even if those PCDs are set to the correct values, VariableSmm blows up
with "Firmware Volume for Variable Store is corrupted".

. Ahh, I realized why this happens! QEMU prevents even *read
accesses* if they don't come from code running in SMM.

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

2016-08-24 Thread Laszlo Ersek 
On 08/24/16 01:22, Zeng, Star wrote:
> On 2016/8/24 11:27, Laszlo Ersek wrote:
>> On 08/23/16 22:39, Zeng, Star wrote:
>>> On 2016/8/23 23:33, Laszlo Ersek wrote:
 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

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

2016-08-24 Thread Zeng, Star 

On 2016/8/24 13:22, Zeng, Star wrote:

On 2016/8/24 11:27, Laszlo Ersek wrote:

On 08/23/16 22:39, Zeng, Star wrote:

On 2016/8/23 23:33, Laszlo Ersek wrote:

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

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

2016-08-23 Thread Zeng, Star 

On 2016/8/24 11:27, Laszlo Ersek wrote:

On 08/23/16 22:39, Zeng, Star wrote:

On 2016/8/23 23:33, Laszlo Ersek wrote:

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.

[...]

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

2016-08-23 Thread Laszlo Ersek 
On 08/23/16 22:39, Zeng, Star wrote:
> On 2016/8/23 23:33, Laszlo Ersek wrote:
>> 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

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

2016-08-23 Thread Zeng, Star 

On 2016/8/23 23:33, Laszlo Ersek wrote:

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

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

2016-08-23 Thread Laszlo Ersek 
e 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 = 0xF
> 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-boun...@lists.01.org] On Behalf
>>>> Of Laszlo Ersek
>>>> Sent: Friday, August 19, 2016 9:19 AM
>>>> To: Fan, Jeff; edk2-de...@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 Dyna

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

2016-08-19 Thread Laszlo Ersek 
On 08/19/16 04: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.

Thanks, I'll try to do that the week after next or so, hopefully.

Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2016-08-18 Thread Zeng, Star 

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-boun...@lists.01.org] On Behalf
Of Laszlo Ersek
Sent: Friday, August 19, 2016 9:19 AM
To: Fan, Jeff; edk2-de...@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.kin...@intel.com>
Cc: Feng Tian <feng.t...@intel.com>
Cc: Giri P Mudusuru <giri.p.mudus...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff@intel.com>
Reviewed-by: Giri P Mudusuru <giri.p.mudus...@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.
+# 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 8a2d564b2a811b8dbc

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

2016-08-18 Thread Zeng, Star 

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?


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-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Friday, August 19, 2016 9:19 AM
To: Fan, Jeff; edk2-de...@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.kin...@intel.com>
Cc: Feng Tian <feng.t...@intel.com>
Cc: Giri P Mudusuru <giri.p.mudus...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff@intel.com>
Reviewed-by: Giri P Mudusuru <giri.p.mudus...@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.
+# 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

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

2016-08-18 Thread Laszlo Ersek 
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?

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-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Friday, August 19, 2016 9:19 AM
> To: Fan, Jeff; edk2-de...@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.kin...@intel.com>
>> Cc: Feng Tian <feng.t...@intel.com>
>> Cc: Giri P Mudusuru <giri.p.mudus...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jeff Fan <jeff@intel.com>
>> Reviewed-by: Giri P Mudusuru <giri.p.mudus...@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.
>> +# 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 
> 7fadaacd50d7

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

2016-08-18 Thread Fan, Jeff 
Laszlo,

I could revert this patch firstly. Could you please dig out the OVMF to address 
the potential issue, then I could re-commit it for those platforms required 
this patch.

Thanks!
Jeff

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Friday, August 19, 2016 9:19 AM
To: Fan, Jeff; edk2-de...@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.kin...@intel.com>
> Cc: Feng Tian <feng.t...@intel.com>
> Cc: Giri P Mudusuru <giri.p.mudus...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff@intel.com>
> Reviewed-by: Giri P Mudusuru <giri.p.mudus...@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.
> +# 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

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

2016-08-18 Thread Laszlo Ersek 
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