From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9324821A13482 for ; Thu, 4 May 2017 11:50:05 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP; 04 May 2017 11:50:05 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,289,1491289200"; d="scan'208";a="1143720057" Received: from jljusten-skl.jf.intel.com (HELO localhost) ([10.54.75.15]) by fmsmga001.fm.intel.com with ESMTP; 04 May 2017 11:50:04 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <149392380410.9783.3695261429662975796@jljusten-skl.jf.intel.com> From: Jordan Justen In-Reply-To: <795227c7-2191-239a-a940-4f5ea37adfc3@redhat.com> Cc: Gary Ching-Pang Lin References: <20170503213947.32290-1-lersek@redhat.com> <20170503213947.32290-5-lersek@redhat.com> <149391577010.7999.17667669342464390958@jljusten-skl.jf.intel.com> <795227c7-2191-239a-a940-4f5ea37adfc3@redhat.com> User-Agent: alot/0.5.1 Date: Thu, 04 May 2017 11:50:04 -0700 Subject: Re: [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 May 2017 18:50:05 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-05-04 09:52:48, Laszlo Ersek wrote: > 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_Securit= y_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 If the limit is 32k, why go with 33k? Does the test fail with a 32k limit? -Jordan > 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 increa= se. > = > > = > > Aside from this question: > > = > > Series Reviewed-by: Jordan Justen > = > 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 c= an > >> accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSi= ze > >> to 0x8400 -- beyond DataSize=3D0x8000 from the HCK test, we need some = room > >> for the variable name and attributes as well. > >> > >> Cc: Gary Ching-Pang Lin > >> Cc: Jordan Justen > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Laszlo Ersek > >> --- > >> > >> 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) =3D=3D TRUE > >> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE > >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE > >> !endif > >> = > >> [PcdsFixedAtBuild] > >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChang= e|FALSE > >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32 > >> +!if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) > >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 > >> +!endif > >> +!if $(FD_SIZE_IN_KB) =3D=3D 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) =3D=3D TRUE > >> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE > >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE > >> !endif > >> = > >> [PcdsFixedAtBuild] > >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChang= e|FALSE > >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32 > >> +!if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) > >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 > >> +!endif > >> +!if $(FD_SIZE_IN_KB) =3D=3D 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) =3D=3D TRUE > >> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE > >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE > >> !endif > >> = > >> [PcdsFixedAtBuild] > >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChang= e|FALSE > >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32 > >> +!if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) > >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 > >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800 > >> +!endif > >> +!if $(FD_SIZE_IN_KB) =3D=3D 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 > >> > >> >=20