Re: [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB

[Laszlo Ersek <lersek@redhat.com>]

On 05/04/17 18:36, Jordan Justen wrote:
> On 2017-05-03 14:39:46, Laszlo Ersek wrote:
>> The "ConfirmSetOfLargeVariable" test case of the Secure Boot Logo Test
>> ("Microsoft.UefiSecureBootLogo.Tests") suite in the Microsoft Hardware
>> Certification Kit sets a 32 KB large non-authenticated variable.
> 
> According to
> http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf
> 
> "The maximum supported variable size must be at least 64kB"
> 
> Should we just bump the size to match this? We should be able to make
> this change later once it is in a test/spec, but for some reason I
> thought the requirement was already 64k.

The 32KB requirement comes from the most recent Secure Boot Logo Test. I
installed both the Windows Server 2008 R2 SP1 test controller and the
Windows 2016 Server test client just the other day, together with the
most recent filters, using the following descriptions:

https://msdn.microsoft.com/en-us/library/windows/hardware/jj123537.aspx
https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Controller_VM
https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Client_VM

Given that this limit can be bumped without breaking compatibility, as
you say, I'd like to remain frugal with it, same as we were in James's
commit f5404a3eba1d ("OvmfPkg: Increase the maximum size for
Authenticated variables", 2016-03-24).

I don't understand why the plugfest presentation and the SB Logo Test
require different limits... But, I'm certain our QE will find out in
short order once the SB Logo Test catches up with the presentation, and
I expect I'll submit the corresponding patch soon after.

I dislike the speculation in this series, but breaking compatibility is
even worse. (A lot worse, to me at least.) So I consider the varstore
restructuring the smaller of two wrongs. However, wrt.
PcdMaxVariableSize, it seems we're not being forced to either of those
wrongs (i.e., breaking compat or speculation), so we can delay the increase.

> 
> Aside from this question:
> 
> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thanks a lot!

I'll await your ACK for the above argument before pushing the series.

Thanks,
Laszlo

>> In the FD_SIZE_4MB build, our live varstore is now 256 KB big, so we can
>> accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSize
>> to 0x8400 -- beyond DataSize=0x8000 from the HCK test, we need some room
>> for the variable name and attributes as well.
>>
>> Cc: Gary Ching-Pang Lin <glin@suse.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     v2:
>>     - adjust to FD_SIZE_IN_KB
>>     - update commit msg to state 256 KB for the varstore [Jordan]
>>
>>  OvmfPkg/OvmfPkgIa32.dsc    | 6 ++++++
>>  OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
>>  OvmfPkg/OvmfPkgX64.dsc     | 6 ++++++
>>  3 files changed, 18 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 26b807dde9fa..e0779ddaa426 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -404,28 +404,34 @@ [PcdsFeatureFlag]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>  !endif
>>  !if $(SMM_REQUIRE) == TRUE
>>    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>  !endif
>>  
>>  [PcdsFixedAtBuild]
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>    gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>> +!endif
>> +!if $(FD_SIZE_IN_KB) == 4096
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>> +!endif
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>  
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>  
>>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>  
>>    # DEBUG_INIT      0x00000001  // Initialization
>>    # DEBUG_WARN      0x00000002  // Warnings
>>    # DEBUG_LOAD      0x00000004  // Load events
>>    # DEBUG_FS        0x00000008  // EFI File system
>>    # DEBUG_POOL      0x00000010  // Alloc & Free (pool)
>>    # DEBUG_PAGE      0x00000020  // Alloc & Free (page)
>>    # DEBUG_INFO      0x00000040  // Informational debug messages
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 41f06a6b6a66..bbe26e2cf452 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>  !endif
>>  !if $(SMM_REQUIRE) == TRUE
>>    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>  !endif
>>  
>>  [PcdsFixedAtBuild]
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>    gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>> +!endif
>> +!if $(FD_SIZE_IN_KB) == 4096
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>> +!endif
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>  
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>  
>>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>  
>>    # DEBUG_INIT      0x00000001  // Initialization
>>    # DEBUG_WARN      0x00000002  // Warnings
>>    # DEBUG_LOAD      0x00000004  // Load events
>>    # DEBUG_FS        0x00000008  // EFI File system
>>    # DEBUG_POOL      0x00000010  // Alloc & Free (pool)
>>    # DEBUG_PAGE      0x00000020  // Alloc & Free (page)
>>    # DEBUG_INFO      0x00000040  // Informational debug messages
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 053c84b685c5..ff795815f65f 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>  !endif
>>  !if $(SMM_REQUIRE) == TRUE
>>    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>  !endif
>>  
>>  [PcdsFixedAtBuild]
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>    gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>> +!endif
>> +!if $(FD_SIZE_IN_KB) == 4096
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>> +!endif
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>  
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>  
>>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>  
>>    # DEBUG_INIT      0x00000001  // Initialization
>>    # DEBUG_WARN      0x00000002  // Warnings
>>    # DEBUG_LOAD      0x00000004  // Load events
>>    # DEBUG_FS        0x00000008  // EFI File system
>>    # DEBUG_POOL      0x00000010  // Alloc & Free (pool)
>>    # DEBUG_PAGE      0x00000020  // Alloc & Free (page)
>>    # DEBUG_INFO      0x00000040  // Informational debug messages
>> -- 
>> 2.9.3
>>
>>