From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 8CD9421A13487 for ; Thu, 4 May 2017 09:52:51 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F07E9804E4; Thu, 4 May 2017 16:52:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F07E9804E4 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com F07E9804E4 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-56.phx2.redhat.com [10.3.116.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id CFEA880E6C; Thu, 4 May 2017 16:52:49 +0000 (UTC) To: Jordan Justen , edk2-devel-01 References: <20170503213947.32290-1-lersek@redhat.com> <20170503213947.32290-5-lersek@redhat.com> <149391577010.7999.17667669342464390958@jljusten-skl.jf.intel.com> Cc: Gary Ching-Pang Lin From: Laszlo Ersek Message-ID: <795227c7-2191-239a-a940-4f5ea37adfc3@redhat.com> Date: Thu, 4 May 2017 18:52:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <149391577010.7999.17667669342464390958@jljusten-skl.jf.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 04 May 2017 16:52:51 +0000 (UTC) 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 16:52:51 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 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 >> 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) == 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 >> >>