Re: [edk2] [PATCH 3/8] OvmfPkg: Add NV Variable storage within FD

Subject: Re: [edk2] [PATCH 3/8] OvmfPkg: Add NV Variable storage within FD

From: Laszlo Ersek <lersek@redhat.com>

To: edk2-devel@lists.sourceforge.net

Date: 2013-10-31 06:03:20

On 10/28/13 22:27, Jordan Justen wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen 
> ---
>  OvmfPkg/OvmfPkgIa32.fdf    |   93 ++++++++++++++++++++++++++++++++++++++------
>  OvmfPkg/OvmfPkgIa32X64.fdf |   93 ++++++++++++++++++++++++++++++++++++++------
>  OvmfPkg/OvmfPkgX64.fdf     |   93 ++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 243 insertions(+), 36 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index 40f0651..03cdedc 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -30,26 +30,95 @@ DEFINE FD_SIZE_1MB=
>  
>  !ifdef $(FD_SIZE_1MB)
>  [FD.OVMF]
> -BaseAddress   = 0xFFF00000
> -Size          = 0x00100000
> +BaseAddress   = 0xFFF00000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
> +Size          = 0x00100000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize

Ah, much clearer; FDF spec to the rescue. We're capturing these values
from the FDF tokens in the PCDs (so that we can access them in C too,
presumably).

>  ErasePolarity = 1
> -BlockSize     = 0x1000
> +BlockSize     = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
>  NumBlocks     = 0x100
> +!else
> +[FD.OVMF]
> +BaseAddress   = 0xFFE00000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
> +Size          = 0x00200000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
> +ErasePolarity = 1
> +BlockSize     = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
> +NumBlocks     = 0x200
> +!endif

This duplicates the the !else branch (ie. when the flash size is 2MB),
and performs the same PCD assignments as above.

At this point we're independent of the FD size.

Five PCDs remain unused (PcdOvmfFlashNvStorage*).


> +
> +0x00000000|0x0000e000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize

According to 2.3.4 FD Region Layout, this starts a region as usual (at
offset 0 from BaseAddress, size 0xe000 == 56K), and the offset and the
size are captured in the PCDs (shortcut form).

Do you have any estimate how much "useful" data these 56 kilobytes can
save? (For example, kernel stack dumps are sometimes stored there.) Is
56 KB a "usual" amount for real hardware?

Regarding the PCDs that have been introduced previously... We're now
down to 4 unused PCDs. Interestingly, the size of the region is assigned
to a PCD in a different "namespace" (GUID).

> +#NV_VARIABLE_STORE
> +DATA = {
> +  ## This is the EFI_FIRMWARE_VOLUME_HEADER
> +  # ZeroVector []
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  # FileSystemGuid: gEfiSystemNvDataFvGuid         =
> +  #  { 0xFFF12B8D, 0x7696, 0x4C8B,
> +  #    { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }}
> +  0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
> +  0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
> +  # FvLength: 0x20000

(1) Isn't FvLength too big? I mean we're in a region that's 56 KB in
size, and FvLength is 128 KB. This part looks like a copy fron Nt32Pkg
(and/or EmulatorPkg), shouldn't we update it?

> +  0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  #Signature "_FVH"       #Attributes
> +  0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00,
> +  #HeaderLength #CheckSum #ExtHeaderOffset #Reserved #Revision
> +  0x48, 0x00, 0x19, 0xF9, 0x00, 0x00, 0x00, 0x02,
> +  #Blockmap[0]: 2 Blocks * 0x10000 Bytes / Block

OK, so blocksize dictates FvLength... Where does blocksize come from?

According to "MdePkg/Include/Pi/PiFirmwareVolume.h",
EFI_FV_BLOCK_MAP_ENTRY.Length (visible below) determines the size of the
blocks.

So, I think we could update this if we wanted to -- I don't know if we
need to.

> +  0x20, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
> +  #Blockmap[1]: End
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  ## This is the VARIABLE_STORE_HEADER
> +!if $(SECURE_BOOT_ENABLE) == TRUE
> +  #Signature: gEfiAuthenticatedVariableGuid =
> +  #  { 0xaaf32c78, 0x947b, 0x439a,
> +  #    { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
> +  0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
> +  0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
> +!else
> +  #Signature: gEfiVariableGuid =
> +  #  { 0xddcf3616, 0x3275, 0x4164,
> +  #    { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }}
> +  0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,
> +  0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,
> +!endif
> +  #Size: 0xe000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xdfb8
> +  # This can speed up the Variable Dispatch a bit.
> +  0xB8, 0xDF, 0x00, 0x00,
> +  #FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
> +  0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +}

OK. I'll trust you on this.

>  
> -0x00000000|0x000EC000

(So normally this introduced FVMAIN_COMPACT...)

> +0x0000e000|0x00001000
> +#NV_EVENT_LOG
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize

I guess we'll use this in a ringbuffer-like fashion... One block (4K).

And we've consumed two further OVMF PCDs; we're down to two
(PcdOvmfFlashNvStorageFtw*).

> +
> +0x0000f000|0x00001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> +#NV_FTW_WORKING
> +DATA = {
> +  # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid         =
> +  #  { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65,  0x0, 0xfd, 0x9f, 0x1b, 0x95 }}
> +  0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49,
> +  0xa0, 0xce, 0x65,  0x0, 0xfd, 0x9f, 0x1b, 0x95,
> +  # Crc:UINT32            #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved
> +  0x2c, 0xaf, 0x2c, 0x64, 0xFE, 0xFF, 0xFF, 0xFF,
> +  # WriteQueueSize: UINT64
> +  0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +}

Guess it's OK... Resembles Nt32Pkg.

No idea about the purpose. (I should probably educate myself about fault
tolerant writes.)

One PCD left.

> +
> +0x00010000|0x00010000
> +#NV_FTW_SPARE
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> +

OK, used up last PCD. 64 KB for the "fault tolerant spare", whatever it is.

> +!ifdef $(FD_SIZE_1MB)
> +0x00020000|0x000CC000
>  FV = FVMAIN_COMPACT

The size of FVMAIN_COMPACT used to be (in the 1MB build) 0x000EC000 ==
944 KB. We're now making sure that FVMAIN_COMPACT ends at the same
offset (0xEC000), but its region only starts after the nvvar-related
areas. So the new size is 0xCC000 == 816 KB, 128 KB less than before.
Hopefully it's enough (for release builds).

>  
>  0x000EC000|0x14000
>  FV = SECFV

Yes, from this on the offsets/sizes are unchanged.

I think the commit message could state explicitly that we're carving out
the nvvar storage of where FVMAIN_COMPACT used to start.


> -!else
> -[FD.OVMF]
> -BaseAddress   = 0xFFE00000
> -Size          = 0x00200000
> -ErasePolarity = 1
> -BlockSize     = 0x1000
> -NumBlocks     = 0x200

Yep, this part can go, we did it above (with PCD assignments).

>  
> -0x00000000|0x001CC000
> +!else
> +0x00020000|0x001AC000
>  FV = FVMAIN_COMPACT
>  
>  0x001CC000|0x34000

OK. The nvvar storage comes off the leading range of FVMAIN_COMPACT for
the 2MB build as well. The new FVMAIN_COMPACT size is 1712 KB.

The other two files match.

Do we need to fix up FvLength (1)?

Thanks,
Laszlo


------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel