From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.8971.1618319634330430044 for ; Tue, 13 Apr 2021 06:13:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=OULcQ3PG; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1618319633; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0cbjBeC2qQiamJ1BMownlSsolGnIlKlV0l0peRhPg5Y=; b=OULcQ3PGqbtj4lMXzc9TQqvZ2EUcxSrWS69x9YRA8cSlIVfeYsvN6T3dGpXG1WK9PKaTdZ MCVx5rr+MAasleLq66AGFvaRZ2j0yZZBzy+bv4BxhAcmnNWyJGPowJJ1Z0QdwNGg6E13Ib HHSzZKvzoIZJY0+daRN+O1EJCIDQTno= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-76-6wnB_kHXOaChrYUSLjpa-g-1; Tue, 13 Apr 2021 09:13:49 -0400 X-MC-Unique: 6wnB_kHXOaChrYUSLjpa-g-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2BE8B83DD20; Tue, 13 Apr 2021 13:13:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-199.ams2.redhat.com [10.36.115.199]) by smtp.corp.redhat.com (Postfix) with ESMTP id F2C1E5944D; Tue, 13 Apr 2021 13:13:45 +0000 (UTC) Subject: Re: [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest To: Brijesh Singh , devel@edk2.groups.io Cc: James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Ard Biesheuvel References: <20210324153215.17971-1-brijesh.singh@amd.com> <20210324153215.17971-2-brijesh.singh@amd.com> <6bfc9b77-57ae-02ea-5be1-eeb15eab446b@amd.com> <778bc927-94b0-56cb-708b-612d2498dc4b@redhat.com> From: "Laszlo Ersek" Message-ID: <3b581697-b76b-1654-4174-b52f7ee77e46@redhat.com> Date: Tue, 13 Apr 2021 15:13:44 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 04/13/21 13:29, Brijesh Singh wrote: > > On 4/13/21 4:49 AM, Laszlo Ersek wrote: >> On 04/12/21 16:52, Brijesh Singh wrote: >>> Hi James and Laszlo, >>> >>> I was planning to work to add the support to reserve the Secrets and >>> CPUID page in E820 map and then create the EFI configuration table entry >>> for it so that guest OS can reach to it. We have two packages >>> "SecretDxe" and "SecretPei" in OvmfPkg/AmdSev. Any issues if I use them >>> in the OvmfPkg.dsc ? Here is what I was thinking: >>> >>> 1) Rename the PcdSevLaunchSecretBase -> PcdSevSecretsBase >>> >>> 2) When SNP is enabled then VMM use this page as secrets page for the SNP >>> >>> 3) When SEV or SEV-ES is enabled then VMM uses this page as a launch >>> secret page >>> >>> This will allow me to drop PcdOvmfSnpSecretsBase. This will not just >>> save 4-bytes but also minimize the code duplication. >> I'm pretty unhappy about needing a separate page for each such purpose. >> We're wasting room in MEMFD. The GUIDed structs that we expose to QEMU >> seem to be flexible enough to describe non-page-aligned addresses, >> right? Can we pack larger amounts of cruft into MEMFD pages? > > With the GUID approach we should be able to pack multiple fields into a > page but unfortunately in the case of SEV-SNP both the CPUID and Secrets > need to be a page size. Without SNP support the we reserve the following > page for the SEV/SEV-ES: > > 1 page for Launch Secret. > > 2 pages for the GHCB > > 1 page for EsWorkArea > > Both the EsWorkArea and LaunchSecret does not need to be the page > aligned or page sized. Since the SNP needs a full page for the secrets > so  I was inclined to use the same secrets page for both SEV and SNP.  > At the end all we need to do is  reserve one extra page for CPUID to > make the SNP work. > > In future the EsWorkArea page can be used to pack additional information > without needed to reserve full page (if feature does not require page). Thank you for doing this analysis. I would much welcome a separate series, just for "compressing" as many of the artifacts we now have down to as few pages as possible. Could you propose a series like that? Please CC Tom, James and Jiewen for review. For this, feel free to rename PCDs as needed, and also to introduce new (packed, if needed) structure types; probably under . If some constants have to be doubly-defined, for C code and for assembly code separately, for example, I'm fine with that -- normally, that's bad practice, and if we can avoid it, that's great; but being conservative with MEMFD is more important to me. (I hope I'm not going to eat my words a few months down the road; this is how I feel right now anyway.) Thanks! Laszlo > > >> >> I'm not looking forward to the day when we run out of slack in MEMFD and >> we get to shift PEIFV / DXEFV. (Every time we need to increase the DXEFV >> size, the same risk exists -- which is why I've been thinking for a >> while now that OVMF includes too many features already.) This can >> introduce obscure changes to the UEFI memory map, which has caused >> compat problems in the past, for example with the "crash" utility. >> >> The feature creep in OVMF has gone off the rails in the last few years, >> really. (Not that I'm not guilty myself.) >> >> Thanks, >> Laszlo >> >>> Thoughts ? >>> >>> -Brijesh >>> >>> On 3/24/21 10:31 AM, Brijesh Singh wrote: >>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C04be68371db9458cbdc108d8fe61713a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539041773579324%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=J62%2BVSCZRawAkzsi9xtS43cpowZxSCx%2BwcDYNwdF3qA%3D&reserved=0 >>>> >>>> During the SEV-SNP guest launch sequence, two special pages need to >>>> be inserted, the secrets page and cpuid page. The secrets page, >>>> contain the VM platform communication keys. The guest BIOS and OS >>>> can use this key to communicate with the SEV firmware to get the >>>> attestation report. The Cpuid page, contain the CPUIDs entries >>>> filtered through the AMD-SEV firmware. >>>> >>>> The VMM will locate the secrets and cpuid page addresses through a >>>> fixed GUID and pass them to SEV firmware to populate further. >>>> For more information about the page content, see the SEV-SNP spec. >>>> >>>> To simplify the pre-validation range calculation in the next patch, >>>> the CPUID and Secrets pages are moved to the start of the >>>> MEMFD_BASE_ADDRESS. >>>> >>>> Cc: James Bottomley >>>> Cc: Min Xu >>>> Cc: Jiewen Yao >>>> Cc: Tom Lendacky >>>> Cc: Jordan Justen >>>> Cc: Ard Biesheuvel >>>> Cc: Laszlo Ersek >>>> Signed-off-by: Brijesh Singh >>>> --- >>>> OvmfPkg/OvmfPkg.dec | 8 +++++++ >>>> OvmfPkg/OvmfPkgX64.fdf | 24 ++++++++++++-------- >>>> OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 19 ++++++++++++++++ >>>> OvmfPkg/ResetVector/ResetVector.inf | 4 ++++ >>>> OvmfPkg/ResetVector/ResetVector.nasmb | 2 ++ >>>> 5 files changed, 48 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec >>>> index 4348bb45c6..062926772d 100644 >>>> --- a/OvmfPkg/OvmfPkg.dec >>>> +++ b/OvmfPkg/OvmfPkg.dec >>>> @@ -317,6 +317,14 @@ >>>> gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42 >>>> gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43 >>>> >>>> + ## The base address of the CPUID page used by SEV-SNP >>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase|0|UINT32|0x48 >>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize|0|UINT32|0x49 >>>> + >>>> + ## The base address of the Secrets page used by SEV-SNP >>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|0|UINT32|0x50 >>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize|0|UINT32|0x51 >>>> + >>>> [PcdsDynamic, PcdsDynamicEx] >>>> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2 >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10 >>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >>>> index d519f85328..ea214600be 100644 >>>> --- a/OvmfPkg/OvmfPkgX64.fdf >>>> +++ b/OvmfPkg/OvmfPkgX64.fdf >>>> @@ -67,27 +67,33 @@ ErasePolarity = 1 >>>> BlockSize = 0x10000 >>>> NumBlocks = 0xD0 >>>> >>>> -0x000000|0x006000 >>>> +0x000000|0x001000 >>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize >>>> + >>>> +0x001000|0x001000 >>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize >>>> + >>>> +0x002000|0x006000 >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize >>>> >>>> -0x006000|0x001000 >>>> +0x008000|0x001000 >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize >>>> >>>> -0x007000|0x001000 >>>> +0x009000|0x001000 >>>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize >>>> >>>> -0x008000|0x001000 >>>> +0x00A000|0x001000 >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize >>>> >>>> -0x009000|0x002000 >>>> +0x00B000|0x002000 >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >>>> >>>> -0x00B000|0x001000 >>>> -gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize >>>> - >>>> -0x00C000|0x001000 >>>> +0x00D000|0x001000 >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize >>>> >>>> +0x00F000|0x001000 >>>> +gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize >>>> + >>>> 0x010000|0x010000 >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >>>> >>>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>>> index 9c0b5853a4..5456f02924 100644 >>>> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>>> @@ -47,6 +47,25 @@ TIMES (15 - ((guidedStructureEnd - guidedStructureStart + 15) % 16)) DB 0 >>>> ; >>>> guidedStructureStart: >>>> >>>> +; >>>> +; SEV-SNP boot support >>>> +; >>>> +; sevSnpBlock: >>>> +; For the initial boot of SEV-SNP guest, a Secrets and CPUID page must be >>>> +; reserved by the BIOS at a RAM area defined by SEV_SNP_SECRETS_PAGE >>>> +; and SEV_SNP_CPUID_PAGE. A VMM will locate this information using the >>>> +; SEV-SNP boot block. >>>> +; >>>> +; GUID (SEV-SNP boot block): bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9 >>>> +; >>>> +sevSnpBootBlockStart: >>>> + DD SEV_SNP_SECRETS_PAGE >>>> + DD SEV_SNP_CPUID_PAGE >>>> + DW sevSnpBootBlockEnd - sevSnpBootBlockStart >>>> + DB 0xC2, 0xC0, 0x39, 0xBD, 0x8e, 0x2F, 0x43, 0x42 >>>> + DB 0x83, 0xE8, 0x1B, 0x74, 0xCE, 0xBC, 0xB7, 0xD9 >>>> +sevSnpBootBlockEnd: >>>> + >>>> ; >>>> ; SEV Secret block >>>> ; >>>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf >>>> index dc38f68919..d890bb6b29 100644 >>>> --- a/OvmfPkg/ResetVector/ResetVector.inf >>>> +++ b/OvmfPkg/ResetVector/ResetVector.inf >>>> @@ -37,6 +37,10 @@ >>>> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase >>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize >>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase >>>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize >>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase >>>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb >>>> index 5fbacaed5f..2c194958f4 100644 >>>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb >>>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb >>>> @@ -75,6 +75,8 @@ >>>> %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase)) >>>> %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8) >>>> %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16) >>>> + %define SEV_SNP_SECRETS_PAGE FixedPcdGet32 (PcdOvmfSnpSecretsBase) >>>> + %define SEV_SNP_CPUID_PAGE FixedPcdGet32 (PcdOvmfSnpCpuidBase) >>>> %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize)) >>>> %include "Ia32/Flat32ToFlat64.asm" >>>> %include "Ia32/PageTables64.asm" >