web: https://github.com/jljusten/edk2/tree/pei-vars-v1

git: https://github.com/jljusten/edk2.git pei-vars-v1

This series moves flash detection into PEI to allow the PEI variable
access drivers to run. If flash is writable, the PCDs are set to point
at the flash memory. If flash is not writable, the PCDs are set to
point at a memory buffer.

I tested KVM with ROM and writable flash, with S3 sleep/resume. I did
not test SMM.

Jordan Justen (10):
  OvmfPkg/build.sh: Add support for --disable-flash switch
  OvmfPkg QemuFlash: Make QemuFlash.* Base class safe
  OvmfPkg QemuFlash: Make QemuFlashDetected external
  OvmfPkg QemuFlash: Add DetectFlashBaseLib.inf 'NULL' library
  OvmfPkg PlatformPei: Detect and set PcdOvmfFlashVariablesEnable
  OvmfPkg/EmuVariableFvbRuntimeDxe: Use PcdOvmfFlashVariablesEnable
  OvmfPkg PlatformPei: Set flash variable PCDs
  OvmfPkg PlatformPei: Initialize memory based variable store buffer
  OvmfPkg: Enable PEI variable access
  OvmfPkg QemuFlashFvbServicesRuntimeDxe: Cleanup init now done in PEI

Laszlo Ersek (2):
  OvmfPkg: resolve PcdLib for all PEIMs individually
  OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default

 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c             |   4 +-
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf           |   3 +-
 OvmfPkg/OvmfPkgIa32.dsc                            |  27 +-
 OvmfPkg/OvmfPkgIa32.fdf                            |   5 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                         |  27 +-
 OvmfPkg/OvmfPkgIa32X64.fdf                         |   5 +-
 OvmfPkg/OvmfPkgX64.dsc                             |  27 +-
 OvmfPkg/OvmfPkgX64.fdf                             |   5 +-
 OvmfPkg/PlatformPei/Platform.c                     |  34 +--
 OvmfPkg/PlatformPei/Platform.h                     |   7 +-
 OvmfPkg/PlatformPei/PlatformPei.inf                |  11 +-
 OvmfPkg/PlatformPei/Vars.c                         | 283 +++++++++++++++++++++
 .../DetectFlashNullLib.c                           |  41 +++
 .../DetectFlashNullLib.inf                         |  60 +++++
 .../FvbServicesRuntimeDxe.inf                      |   4 -
 .../FvbServicesSmm.inf                             |   4 -
 .../FwBlockService.c                               |  28 +-
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c |  59 +++--
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h |  30 ++-
 OvmfPkg/build.sh                                   |  10 +-
 20 files changed, 530 insertions(+), 144 deletions(-)
 create mode 100644 OvmfPkg/PlatformPei/Vars.c
 create mode 100644 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/DetectFlashNullLib.c
 create mode 100644 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/DetectFlashNullLib.inf

-- 
2.11.0

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

On 03/27/17 10:05, Jordan Justen wrote:
> web: https://github.com/jljusten/edk2/tree/pei-vars-v1
> 
> git: https://github.com/jljusten/edk2.git pei-vars-v1
> 
> This series moves flash detection into PEI to allow the PEI variable
> access drivers to run. If flash is writable, the PCDs are set to point
> at the flash memory. If flash is not writable, the PCDs are set to
> point at a memory buffer.

Can you please state the use case for the latter option? That is, when
the PCDs are set to point at the memory buffer.

In that case, the memory buffer is guaranteed to be empty (modulo
internal formatting that PlatformPei would do just-in-time), unless the
VM has just been rebooted within the same QEMU instance.

In other words, clients of the r/o variable2 PPI will get false results,
most of the time -- if I understand correctly. After a fresh boot (which
is most of the boots), no variable will be found in the PEI phase, even
if the \NvVars file contains it, and the variable services in DXE will
later find it.

... I'm not sure if my understanding above is correct, but if it is,
then I think this feature only contributes to the confusing nature of
the memory-emulated variables.

Thanks
Laszlo

> 
> I tested KVM with ROM and writable flash, with S3 sleep/resume. I did
> not test SMM.
> 
> Jordan Justen (10):
>   OvmfPkg/build.sh: Add support for --disable-flash switch
>   OvmfPkg QemuFlash: Make QemuFlash.* Base class safe
>   OvmfPkg QemuFlash: Make QemuFlashDetected external
>   OvmfPkg QemuFlash: Add DetectFlashBaseLib.inf 'NULL' library
>   OvmfPkg PlatformPei: Detect and set PcdOvmfFlashVariablesEnable
>   OvmfPkg/EmuVariableFvbRuntimeDxe: Use PcdOvmfFlashVariablesEnable
>   OvmfPkg PlatformPei: Set flash variable PCDs
>   OvmfPkg PlatformPei: Initialize memory based variable store buffer
>   OvmfPkg: Enable PEI variable access
>   OvmfPkg QemuFlashFvbServicesRuntimeDxe: Cleanup init now done in PEI
> 
> Laszlo Ersek (2):
>   OvmfPkg: resolve PcdLib for all PEIMs individually
>   OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
> 
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c             |   4 +-
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf           |   3 +-
>  OvmfPkg/OvmfPkgIa32.dsc                            |  27 +-
>  OvmfPkg/OvmfPkgIa32.fdf                            |   5 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                         |  27 +-
>  OvmfPkg/OvmfPkgIa32X64.fdf                         |   5 +-
>  OvmfPkg/OvmfPkgX64.dsc                             |  27 +-
>  OvmfPkg/OvmfPkgX64.fdf                             |   5 +-
>  OvmfPkg/PlatformPei/Platform.c                     |  34 +--
>  OvmfPkg/PlatformPei/Platform.h                     |   7 +-
>  OvmfPkg/PlatformPei/PlatformPei.inf                |  11 +-
>  OvmfPkg/PlatformPei/Vars.c                         | 283 +++++++++++++++++++++
>  .../DetectFlashNullLib.c                           |  41 +++
>  .../DetectFlashNullLib.inf                         |  60 +++++
>  .../FvbServicesRuntimeDxe.inf                      |   4 -
>  .../FvbServicesSmm.inf                             |   4 -
>  .../FwBlockService.c                               |  28 +-
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c |  59 +++--
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h |  30 ++-
>  OvmfPkg/build.sh                                   |  10 +-
>  20 files changed, 530 insertions(+), 144 deletions(-)
>  create mode 100644 OvmfPkg/PlatformPei/Vars.c
>  create mode 100644 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/DetectFlashNullLib.c
>  create mode 100644 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/DetectFlashNullLib.inf
> 

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

On 2017-03-27 11:03:42, Laszlo Ersek wrote:
> On 03/27/17 10:05, Jordan Justen wrote:
> > web: https://github.com/jljusten/edk2/tree/pei-vars-v1
> > 
> > git: https://github.com/jljusten/edk2.git pei-vars-v1
> > 
> > This series moves flash detection into PEI to allow the PEI variable
> > access drivers to run. If flash is writable, the PCDs are set to point
> > at the flash memory. If flash is not writable, the PCDs are set to
> > point at a memory buffer.
> 
> Can you please state the use case for the latter option? That is, when
> the PCDs are set to point at the memory buffer.
> 
> In that case, the memory buffer is guaranteed to be empty (modulo
> internal formatting that PlatformPei would do just-in-time), unless the
> VM has just been rebooted within the same QEMU instance.
> 
> In other words, clients of the r/o variable2 PPI will get false results,
> most of the time -- if I understand correctly. After a fresh boot (which
> is most of the boots), no variable will be found in the PEI phase, even
> if the \NvVars file contains it, and the variable services in DXE will
> later find it.
> 
> ... I'm not sure if my understanding above is correct, but if it is,
> then I think this feature only contributes to the confusing nature of
> the memory-emulated variables.

I think we should continue to support the memory vars, even if they
have some obvious drawbacks.

-Jordan

> 
> > 
> > I tested KVM with ROM and writable flash, with S3 sleep/resume. I did
> > not test SMM.
> > 
> > Jordan Justen (10):
> >   OvmfPkg/build.sh: Add support for --disable-flash switch
> >   OvmfPkg QemuFlash: Make QemuFlash.* Base class safe
> >   OvmfPkg QemuFlash: Make QemuFlashDetected external
> >   OvmfPkg QemuFlash: Add DetectFlashBaseLib.inf 'NULL' library
> >   OvmfPkg PlatformPei: Detect and set PcdOvmfFlashVariablesEnable
> >   OvmfPkg/EmuVariableFvbRuntimeDxe: Use PcdOvmfFlashVariablesEnable
> >   OvmfPkg PlatformPei: Set flash variable PCDs
> >   OvmfPkg PlatformPei: Initialize memory based variable store buffer
> >   OvmfPkg: Enable PEI variable access
> >   OvmfPkg QemuFlashFvbServicesRuntimeDxe: Cleanup init now done in PEI
> > 
> > Laszlo Ersek (2):
> >   OvmfPkg: resolve PcdLib for all PEIMs individually
> >   OvmfPkg: resolve PcdLib for PEIMs to PeiPcdLib by default
> > 
> >  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c             |   4 +-
> >  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf           |   3 +-
> >  OvmfPkg/OvmfPkgIa32.dsc                            |  27 +-
> >  OvmfPkg/OvmfPkgIa32.fdf                            |   5 +-
> >  OvmfPkg/OvmfPkgIa32X64.dsc                         |  27 +-
> >  OvmfPkg/OvmfPkgIa32X64.fdf                         |   5 +-
> >  OvmfPkg/OvmfPkgX64.dsc                             |  27 +-
> >  OvmfPkg/OvmfPkgX64.fdf                             |   5 +-
> >  OvmfPkg/PlatformPei/Platform.c                     |  34 +--
> >  OvmfPkg/PlatformPei/Platform.h                     |   7 +-
> >  OvmfPkg/PlatformPei/PlatformPei.inf                |  11 +-
> >  OvmfPkg/PlatformPei/Vars.c                         | 283 +++++++++++++++++++++
> >  .../DetectFlashNullLib.c                           |  41 +++
> >  .../DetectFlashNullLib.inf                         |  60 +++++
> >  .../FvbServicesRuntimeDxe.inf                      |   4 -
> >  .../FvbServicesSmm.inf                             |   4 -
> >  .../FwBlockService.c                               |  28 +-
> >  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c |  59 +++--
> >  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h |  30 ++-
> >  OvmfPkg/build.sh                                   |  10 +-
> >  20 files changed, 530 insertions(+), 144 deletions(-)
> >  create mode 100644 OvmfPkg/PlatformPei/Vars.c
> >  create mode 100644 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/DetectFlashNullLib.c
> >  create mode 100644 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/DetectFlashNullLib.inf
> > 
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 03/27/17 23:47, Jordan Justen wrote:
> On 2017-03-27 11:03:42, Laszlo Ersek wrote:
>> On 03/27/17 10:05, Jordan Justen wrote:
>>> web: https://github.com/jljusten/edk2/tree/pei-vars-v1
>>>
>>> git: https://github.com/jljusten/edk2.git pei-vars-v1
>>>
>>> This series moves flash detection into PEI to allow the PEI variable
>>> access drivers to run. If flash is writable, the PCDs are set to
>>> point at the flash memory. If flash is not writable, the PCDs are
>>> set to point at a memory buffer.
>>
>> Can you please state the use case for the latter option? That is,
>> when the PCDs are set to point at the memory buffer.
>>
>> In that case, the memory buffer is guaranteed to be empty (modulo
>> internal formatting that PlatformPei would do just-in-time), unless
>> the VM has just been rebooted within the same QEMU instance.
>>
>> In other words, clients of the r/o variable2 PPI will get false
>> results, most of the time -- if I understand correctly. After a fresh
>> boot (which is most of the boots), no variable will be found in the
>> PEI phase, even if the \NvVars file contains it, and the variable
>> services in DXE will later find it.
>>
>> ... I'm not sure if my understanding above is correct, but if it is,
>> then I think this feature only contributes to the confusing nature of
>> the memory-emulated variables.
>
> I think we should continue to support the memory vars, even if they
> have some obvious drawbacks.

I don't understand how you can call this "continued support" when Xen
(and any other similarly pflash-less virt host scenarios) will see zero
benefit from the increased complexity. The r/o variable2 PPI will be
available to them, and it will almost always lie to them.

Is that "continued support"? Is the availability of a lying PPI better
than the absence of a functional PPI? I'm out of arguments.

Anyway, I tested the series with the SMM_REQUIRE build -- you mentioned
in the blurb that you didn't test SMM --; there is a regression.
PlatformPei fails to detect flash:

> Loading PEIM at 0x00000844B20 EntryPoint=0x00000844D40 PlatformPei.efi
> Select Item: 0x0
> FW CFG Signature: 0x554D4551
> Select Item: 0x1
> FW CFG Revision: 0x3
> QemuFwCfg interface (DMA) is supported.
> QEMU Flash: Attempting flash detection at FFE00010
> QemuFlashDetected => FD behaves as ROM
> QemuFlashDetected => No
> Platform PEIM Loaded

the SMBASE relocation succeeds:

> Loading SMM driver at 0x0007FFA8000 EntryPoint=0x0007FFA9034 PiSmmCpuDxeSmm.efi
> SMRR Base: 0x7F800000, SMRR Size: 0x800000
> PcdCpuSmmCodeAccessCheckEnable = 1
> mAddressEncMask = 0x0
> SMRAM TileSize = 0x00002000 (0x00001000, 0x00001000)
> SMRAM SaveState Buffer (0x7FF9A000, 0x0000E000)
> CPU[000]  APIC ID=0000  SMBASE=7FF92000  SaveState=7FFA1C00  Size=00000400
> CPU[001]  APIC ID=0001  SMBASE=7FF94000  SaveState=7FFA3C00  Size=00000400
> CPU[002]  APIC ID=0002  SMBASE=7FF96000  SaveState=7FFA5C00  Size=00000400
> CPU[003]  APIC ID=0003  SMBASE=7FF98000  SaveState=7FFA7C00  Size=00000400
> mXdSupported - 0x0
> One Semaphore Size    = 0x40
> Total Semaphores Size = 0x840
> InstallProtocolInterface: [gEfiSmmConfigurationProtocolGuid] 7FFC2040
> SMM IPL registered SMM Entry Point address 7FFE099D
> SmmInstallProtocolInterface: [EfiSmmCpuProtocol] 7FFC2068
> SmmInstallProtocolInterface: [EfiSmmCpuServiceProtocol] 7FFC2084
> SMM S3 SMRAM Structure = 7F47DAD8
> SMM S3 Structure = 7F800000
> SMM CPU Module exit from SMRAM with EFI_SUCCESS
> PageTable is 0!
> SMM IPL closed SMRAM window

and then FvbServicesSmm blows up due to the originally failed flash
detection:

> InstallProtocolInterface: [EfiLoadedImageProtocol] 7E9EEC10
> SmmInstallProtocolInterface: [EfiLoadedImageProtocol] 7FFDD47C
> Loading SMM driver at 0x0007FF5F000 EntryPoint=0x0007FF60034 FvbServicesSmm.efi
> ASSERT .../OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c(970): !_gPcd_FixedAtBuild_PcdSmmSmramRequire

The reason for the failed flash detection, in PlatformPei, is that
PlatformPei does not run in System Management Mode. Therefore QEMU
denies it write access to the pflash chip.

Without the series applied, the flash detection (which involves a write)
only occurs in FvbServicesSmm, which is an SMM driver; it runs in System
Management Mode, so QEMU lets it write to the pflash chip.

NB, the assertion failure (the boot regression) is just one problem. The
real problem (concerning the feature itself) is that the failed dynamic
flash detection in PlatformPei will prevent the PEI phase, in the
SMM_REQUIRE build, from reading the actual pflash variable store.

In the SMM_REQUIRE build, the real variables in the pflash store *would
be* available for reading (via the r/o variable2 PPI), but PlatformPei
falls back to memory emulation, because it cannot successfully write the
pflash. That kind of defeats the entire feature.

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

On 2017-03-28 02:22:37, Laszlo Ersek wrote:
> 
> > InstallProtocolInterface: [EfiLoadedImageProtocol] 7E9EEC10
> > SmmInstallProtocolInterface: [EfiLoadedImageProtocol] 7FFDD47C
> > Loading SMM driver at 0x0007FF5F000 EntryPoint=0x0007FF60034 FvbServicesSmm.efi
> > ASSERT .../OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c(970): !_gPcd_FixedAtBuild_PcdSmmSmramRequire
> 
> The reason for the failed flash detection, in PlatformPei, is that
> PlatformPei does not run in System Management Mode. Therefore QEMU
> denies it write access to the pflash chip.

Ah. Yet another discrepancy from hardware. Anyway, I installed GCC
5.4, and I can now build IA32 again and reproduce the SMM issue. (GCC
6.3 is generating some unsupported relocations for me in the OVMF SEC
image.)

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