On 05/05/17 10:57, Jordan Justen wrote:
> On 2017-05-04 21:07:24, Laszlo Ersek wrote:
>> On 05/03/17 23:39, Laszlo Ersek wrote:
>>>     - Raise VARS_LIVE_SIZE by 8KB to 256KB, VARS_SPARE_SIZE by 8KB to 264KB,
>> Unfortunately, we have a terrible regression here. :(
>>
> <snip>
> 
>> Adapting EmuVariableFvbRuntimeDxe to a non-power-of-two NV spare area
>> size is hopeless. (Definitely hopeless for the time frame & resources
>> I'm looking at.)
>>
>> Worse, even -pflash is broken in the 4MB build, actually. The
>> non-power-of-two NV spare area size, when used as Alignment for
>> AllocateAlignedRuntimePages() in ReserveEmuVariableNvStore(), triggers
>> an assertion. And this path is taken for the -pflash boot as well.
> For a short term fix, would something like this work?

Absolutely. I didn't dare ask for it.

> 1. Force emu fvb buffer alignment to next power-of-two
> 
>    Something like the attachment, but I'm guessing you already wrote
>    something similar.

Yes, I have almost exactly that; please see the attached
"OvmfPkg/PlatformPei: handle non-power-of-two spare size for emu variables".

The difference is that your patch uses HighBitSet32() directly, which
mine uses through GetPowerOfTwo32(). Also yours starts with the full
size, and then subtracts one for the shift in the alignment; mine starts
with half the size (i.e., the spare area size) and uses that in the
alignment.

One other comment on the patch:

> 0001-PlatformPei-Force-EmuVariableNvStore-alignment-size-.patch
> 
> 
> From 58ba393adf60caf806b26dec9aa56d936743f595 Mon Sep 17 00:00:00 2001
> From: Jordan Justen <jordan.l.justen@intel.com>
> Date: Fri, 5 May 2017 01:43:31 -0700
> Subject: [PATCH] PlatformPei: Force EmuVariableNvStore alignment size to power
>  of two
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>  OvmfPkg/PlatformPei/Platform.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 77a8a16c15..97dce8de92 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -504,6 +504,14 @@ ReserveEmuVariableNvStore (
>  {
>    EFI_PHYSICAL_ADDRESS VariableStore;
>    RETURN_STATUS        PcdStatus;
> +  UINT32               EmuFvbSize;
> +  INTN                 SizeHighBit;
> +
> +  EmuFvbSize = 2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize);
> +  SizeHighBit = HighBitSet32 (EmuFvbSize);
> +  if ((EmuFvbSize & (EmuFvbSize - 1)) != 0) {
> +    SizeHighBit++;
> +  }
>  
>    //
>    // Allocate storage for NV variables early on so it will be
> @@ -514,13 +522,13 @@ ReserveEmuVariableNvStore (
>    VariableStore =
>      (EFI_PHYSICAL_ADDRESS)(UINTN)
>        AllocateAlignedRuntimePages (
> -        EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)),
> -        PcdGet32 (PcdFlashNvStorageFtwSpareSize)
> +        EFI_SIZE_TO_PAGES (EmuFvbSize),

I think we should cast EmuFvbSize to UINTN first; EFI_SIZE_TO_PAGES()
likes the "Size" parameter to be UINTN.

> +        1 << (SizeHighBit - 1)
>          );
>    DEBUG ((EFI_D_INFO,
>            "Reserved variable store memory: 0x%lX; size: %dkb\n",
>            VariableStore,
> -          (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
> +          EmuFvbSize / 1024
>          ));
>    PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
>    ASSERT_RETURN_ERROR (PcdStatus);
> -- 2.11.0
> 

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'm tempted to do (1) and commit it with my R-b immediately, but I
realize that "rushing it" is the root of all evil. So I won't rush it.

On 05/05/17 10:57, Jordan Justen wrote:
> 2. Revert 4MB by default patch
>
> This should allow you to start using the 4MB layout for your builds,
> and we can fix the non-flash path before re-enabling 4MB as the
> default.

This works for me, yes. Thank you.

Regarding the non-flash path, I have the attached work-in-progress patches:

- "OvmfPkg: sync PcdVariableStoreSize with PcdFlashNvStorageVariableSize",
- and "wip".

I think that the "wip" patch does all the "simple" fixes in
EmuVariableFvbRuntimeDxe, and what remains is to add code so that the
FVB protocol members actually do their job. Also, the "wip" patch
eliminates any special alignment in the AllocateRuntimePages() call of
PlatformPei, since after the "block size = page size" change, no such
alignment will be necessary. What do you think of this?

So here's the plan:
- based on what you prefer for (1) vs. (2), I'll post that patch as
first patch,
- I'll post the revert of the 4MB default as second patch,
- I think I'll immediately post the patch that syncs
PcdVariableStoreSize as well, as third patch, because it is a small
improvement for the pflash case too.

Does this work for you?

Thank you,
Laszlo