Re: [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK

[Laszlo Ersek <lersek@redhat.com>]

On 05/05/17 17:43, Jordan Justen wrote:
> On 2017-05-05 05:07:25, Laszlo Ersek wrote:
>>
>> Which one do you prefer:
>> (1) I can take your patch, stick in the UINTN cast, and expand the
>> commit message a bit (similarly to what's on my patch),
>> (2) we can go with my patch as well.
> 
> I prefer your patch.
> 
> Re: "0001-OvmfPkg-PlatformPei-handle-non-power-of-two-spare-si.patch"
> 
>> +    Alignment = GetPowerOfTwo32 (Alignment) << 1;
> 
> Do you think this might end up needing a UINT32 cast with 64-bit PEI?

No, the prototype is

UINT32
EFIAPI
GetPowerOfTwo32 (
  IN      UINT32                    Operand
  )

and we can safely shift the returned UINT32 in both 32-bit PEI and
64-bit PEI with the << operator. The result of the shift will also have
type UINT32, which we can safely re-assign to the UINT32 Alignment variable.

Shifting the result of GetPowerOfTwo64() with << would be problematic in
32-bit PEI.

> 
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thanks!

I verified this in the (now again default 2MB build) with the -bios
command line like this:

(1) boot to the UEFI shell normally. The log says:

> Reserved variable store memory: 0x7F50000; size: 128kb,
> alignment: 0x10000
> ...
> EMU Variable FVB Started
> EMU Variable FVB: Using pre-reserved block at 7F50000
> EMU Variable FVB: Basic FV headers were invalid
> Installing FVB for EMU Variable support
> ...
> Ftw: FtwWorkSpaceLba - 0x0, WorkBlockSize  - 0x10000,
> FtwWorkSpaceBase - 0xE000
> Ftw: FtwSpareLba     - 0x1, SpareBlockSize - 0x10000
> Ftw: NumberOfWorkBlock - 0x1, FtwWorkBlockLba - 0x0
> Ftw: WorkSpaceLbaInSpare - 0x0, WorkSpaceBaseInSpare - 0xE000
> Ftw: Remaining work space size - FE0
> Ftw: Work block header check mismatch
> Ftw: Work block header check mismatch
> Ftw: Both working and spare blocks are invalid, init workspace
> Ftw: start to reclaim work space
> Ftw: reclaim work space successfully

(2) run the following shell commands:

Shell>  setvar TestVar0000 -guid ebe29c42-f3d1-4f96-a7c2-5585ce88f056
-bs -nv
="0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"

Shell> reset

(3) after reboot, the log says:

> Reserved variable store memory: 0x7F50000; size: 128kb,
> alignment: 0x10000
> ...
> EMU Variable FVB Started
> EMU Variable FVB: Using pre-reserved block at 7F50000
> EMU Variable FVB: Found valid pre-existing FV
> ...
> Ftw: FtwWorkSpaceLba - 0x0, WorkBlockSize  - 0x10000,
> FtwWorkSpaceBase - 0xE000
> Ftw: FtwSpareLba     - 0x1, SpareBlockSize - 0x10000
> Ftw: NumberOfWorkBlock - 0x1, FtwWorkBlockLba - 0x0
> Ftw: WorkSpaceLbaInSpare - 0x0, WorkSpaceBaseInSpare - 0xE000
> Ftw: Remaining work space size - FE0

(4) run the following shell command:

Shell> setvar TestVar0000 -guid ebe29c42-f3d1-4f96-a7c2-5585ce88f056
EBE29C42-F3D1-4F96-A7C2-5585CE88F056 - TestVar0000 - 0040 Bytes
01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF
01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF
01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF


Also tested this with the 4MB build, using pflash. The log says

> Reserved variable store memory: 0xBFE80000; size: 528kb,
> alignment: 0x80000

The alignment is 512KB, which is the next power of two after 264KB.

> 
>> Regarding the non-flash path, I have the attached work-in-progress patches:
>>
>> - "OvmfPkg: sync PcdVariableStoreSize with PcdFlashNvStorageVariableSize",
>> - and "wip".
> 
> I looked over the non-wip patch, and it seems good, but maybe we
> should just revert the default size for now.

Works for me.

> For a revert of bba8dfbec3bbc4fba7fa6398ba3cf76593e0725e:
> 
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> 

Thanks!

Commits:

  1  6e49d01cfb43 Revert "OvmfPkg: make the 4MB flash size the default"
  2  0c79471d6a98 OvmfPkg/PlatformPei: handle non-power-of-two spare
                  size for emu variables

I've also filed the following TianoCore Feature Request:

https://bugzilla.tianocore.org/show_bug.cgi?id=527

If you or Gary feel inclined to work on this, that would be awesome. I
have no idea when I'll get to it; my TODO list hasn't been this long in
ages. Of course I cannot really *request* that you guys please pick up
my slack, just sayin' that you please feel free to... :)

Thanks!
Laszlo