Re: [edk2-devel] [RESEND PATCH RFC v3 00/22] Add AMD Secure Nested Paging (SEV-SNP) support

["Brijesh Singh" <brijesh.singh@amd.com>]

Hi Laszlo,

On 6/4/21 4:32 AM, Laszlo Ersek wrote:
> Hi Brijesh,
>
> On 05/27/21 01:10, Brijesh Singh wrote:
>> (I missed adding devel@edk2.groups.io, resending the series)
>>
>> 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%7Ccc15730c886844d4783708d9273ba4e2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637583960920109416%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=iN6ph%2BfbfEyY7xIeUAQEeB5FgSAjbeg6VNrU1P6zevU%3D&reserved=0
>>
>> SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
>> new hardware-based memory protections. SEV-SNP adds strong memory
>> integrity protection to help prevent malicious hypervisor-based
>> attacks like data replay, memory re-mapping and more in order to
>> create an isolated memory encryption environment.
>>
>> This series provides the basic building blocks to support booting the
>> SEV-SNP VMs, it does not cover all the security enhancement introduced
>> by the SEV-SNP such as interrupt protection.
>>
>> Many of the integrity guarantees of SEV-SNP are enforced through a new
>> structure called the Reverse Map Table (RMP). Adding a new page to
>> SEV-SNP VM requires a 2-step process. First, the hypervisor assigns a
>> page to the guest using the new RMPUPDATE instruction. This
>> transitions the page to guest-invalid. Second, the guest validates the
>> page using the new PVALIDATE instruction. The SEV-SNP VMs can use the
>> new "Page State Change Request NAE" defined in the GHCB specification
>> to ask hypervisor to add or remove page from the RMP table.
>>
>> Each page assigned to the SEV-SNP VM can either be validated or
>> unvalidated, as indicated by the Validated flag in the page's RMP
>> entry. There are two approaches that can be taken for the page
>> validation: Pre-validation and Lazy Validation.
>>
>> Under pre-validation, the pages are validated prior to first use. And
>> under lazy validation, pages are validated when first accessed. An
>> access to a unvalidated page results in a #VC exception, at which time
>> the exception handler may validate the page. Lazy validation requires
>> careful tracking of the validated pages to avoid validating the same
>> GPA more than once. The recently introduced "Unaccepted" memory type
>> can be used to communicate the unvalidated memory ranges to the Guest
>> OS.
>>
>> At this time we only support the pre-validation. OVMF detects all the
>> available system RAM in the PEI phase. When SEV-SNP is enabled, the
>> memory is validated before it is made available to the EDK2 core.
>>
>> This series does not implements the following SEV-SNP features yet:
>>
>> * CPUID filtering
>> * Lazy validation
>> * Interrupt security
>>
>> The series builds on SNP pre-patch posted here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftinyurl.com%2Fpu6admks&data=04%7C01%7Cbrijesh.singh%40amd.com%7Ccc15730c886844d4783708d9273ba4e2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637583960920119403%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6NGf3nC%2BmDpPIUOIkSaQ0AW0LdDylM5eAvIH7oZdXWg%3D&reserved=0
> That series ("[PATCH v3 00/13] Add GHCBv2 macro and helpers") has been
> merged at this point, as commit range dbc22a178546..adfa3327d4fc. [*]
>
>> Additional resources
>> ---------------------
>> SEV-SNP whitepaper
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2FSEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7Ccc15730c886844d4783708d9273ba4e2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637583960920119403%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=YeTxsYnwYYiONaJ%2BizikzwjH7czwLVUxR7cwDAo%2F1qA%3D&reserved=0
>>
>> APM 2: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F24593.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7Ccc15730c886844d4783708d9273ba4e2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637583960920119403%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2BtOsP5zw%2BFPZzCBHQYYSCXTpRdxPXW4okrJmiRNwDH4%3D&reserved=0 (section 15.36)
>>
>> The complete source is available at
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Ftree%2Fsev-snp-rfc-2&data=04%7C01%7Cbrijesh.singh%40amd.com%7Ccc15730c886844d4783708d9273ba4e2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637583960920119403%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=L8FXX8X%2FITpLvY6JnXXMbZvTQ%2Br0VLsau5DRJ4kKYN8%3D&reserved=0
> So, I'm having trouble applying this series. I attempted to apply it in
> preparation for reviewing patch#2 with a larger context, but I failed,
> as follows:
>
> - When I try applying the series with git-am, upon current master
>   (c410ad4da4b7), patch#21 ("UefiCpuPkg/MpInitLib: Use SEV-SNP AP
>   Creation NAE event to launch APs") does not apply.
>
>   AFAICT, that's because your modification of GetApResetVectorSize() did
>   not (could not) take into account Tom's commit dbc22a178546
>   ("UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack
>   area", 2021-05-29).
>
> - Your remote branch (with HEAD @ 2dbd79823402) is based on upstream
>   commit 01c0ab90beb3 ("AzurePipelines: Add support for ArmPlatformPkg",
>   2021-04-28). If I try to rebase the branch from there to current
>   master (c410ad4da4b7), I get the following rebase action list:
>
>    1  pick 570829c5a0d6 MdePkg: Expand the SEV MSR to include the SNP definition
>    2  pick b9247f69bdfe MdePkg: Define the GHCB Hypervisor features
>    3  pick d09ed6d44ffd MdePkg: Define the Page State Change VMGEXIT structures
>    4  pick 7148b2684f87 MdePkg: Add AsmPvalidate() support
>    5  pick d6a2c2a0d625 OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask()
>    6  pick 9b1037d0d9ac OvmfPkg: Use MemEncryptSevClearMmioPageEncMask() to clear EncMask from Mmio
>    7  pick 556e8fc40179 OvmfPkg/BaseMemEncryptSevLib: Remove CacheFlush parameter
>    8  pick 03e27af79c61 OvmfPkg/VmgExitLib: Allow PMBASE register access in Dxe phase
>    9  pick a81925eeb1c6 OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled()
>   10  pick 3d443240f91c OvmfPkg: Reserve Secrets page in MEMFD
>   11  pick 94dad29970b0 OvmfPkg: Reserve CPUID page for the SEV-SNP guest
>   12  pick b3e3faa12b0f OvmfPkg: Validate the data pages used in the Reset vector and SEC phase
>   13  pick 62290e03c79a UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs
>   14  pick c04e71dabf63 OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field
>   15  pick 76072671f367 OvmfPkg/MemEncryptSevLib: Extend Es Workarea to include hv features
>   16  pick 2bf0eaf2beea OvmfPkg/ResetVector: Invalidate the GHCB page
>   17  pick 2f050b2a1033 OvmfPkg: Add a library to support registering GHCB GPA
>   18  pick b2681bdfbebc OvmfPkg: register GHCB gpa for the SEV-SNP guest
>   19  pick d9f1abb1ff35 UefiCpuPkg/MpLib: add support to register GHCB GPA when SEV-SNP is enabled
>   20  pick 814084815108 OvmfPkg/MemEncryptSevLib: Add support to validate system RAM
>   21  pick ec34893c46ab OvmfPkg/BaseMemEncryptSevLib: Skip the pre-validated system RAM
>   22  pick 37af54f86c3a OvmfPkg/MemEncryptSevLib: Add support to validate > 4GB memory in PEI phase
>   23  pick 25891f51499e OvmfPkg/SecMain: Pre-validate the memory used for decompressing Fv
>   24  pick f2f55135b562 OvmfPkg/PlatformPei: Validate the system RAM when SNP is active
>   25  pick a28b66462eae OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table
>   26  pick b10c0e61913b OvmfPkg/AmdSev: Expose the SNP reserved pages through configuration table
>   27  pick 2dbd79823402 MdePkg/GHCB: Increase the GHCB protocol max version
>
> This is 27 patches, while your series contains 22 patches. It *seems*
> like some of these 27 patches have been merged via [*] already, but it's
> not easy to say which ones. In particular, I can't just drop a
> contiguous *prefix* of this rebase action list, because your *posted*
> patch#1 is "UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs", but
> on your topic branch, that's patch#13! And if I dropped the 12 patches
> before that, then we'd be left with 27-12=15 patches, which obviously
> doesn't match your posted series consisting of 22 patches. I assume some
> of the patches may have been reordered, but I wouldn't like to guess.
>
> I believe we have two problems here: (1) the patch set does not apply to
> current master, (2) the posted patch set doesn't even match your remote
> topic branch.
>
> Problem (1) is somewhat expected (the master branch is expected to
> diverge over time), but problem (2) should never occur. Please never do
> this. If you provide a fetch URL + branch reference in your cover
> letter, then that remote topic branch must match the posted patches
> *forver*. It effectively becomes read only, same as your posted emails
> are read-only. If you need to modify the branch, please create a brand
> new topic branch (possibly with a new version number in the name), and
> rebase that branch.
>
> (It's also possible that you modified your local branch just before
> posting, without pushing it to the <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Ccc15730c886844d4783708d9273ba4e2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637583960920119403%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=AbSYW0DGz0m1L7Xf9tkpF33coQE%2Brg1tDHYEFk9eKfc%3D&amp;reserved=0>
> repository afterwards, but that's quite disruptive too.)
>
> So... what do you want me to do?
>
> - Are at least patches 01 through 20 (as posted to the list)
>   authoritative? Should I review those?
>
> - Or would you like to rebase and repost the entire series (this time
>   keeping the posted version and the fetchable topic branch in sync)?

The main issue is I typed wrong branch name in the cover letter. The
branch name should be "sev-snp-rfc-3" and not "sev-snp-rfc-2". I
apologies for it :(. Ray asked the branch name and I replied him with
the correct branch.

https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-3

This branch was based on commit 5531fd48ded1271b8775725355ab83994e4bc77c
from the upstream. 


> Thanks,
> Laszlo
>
>> GHCB spec:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.amd.com%2Fwp-content%2Fresources%2F56421.pdf&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Ccc15730c886844d4783708d9273ba4e2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637583960920119403%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=DLMzZTZIu1kQa%2BKdDWhDsBatiP%2BnHRZEBMiAPwc%2FYIo%3D&amp;reserved=0
>>
>> SEV-SNP firmware specification:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.amd.com%2Fsev%2F&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Ccc15730c886844d4783708d9273ba4e2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637583960920119403%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eVdldUsQlNqFpDHGCjwl0vbh1Wn3D3dag5CLxybdSM8%3D&amp;reserved=0
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>>
>> Changes since v2:
>>  * Add support for the AP creation.
>>  * Use the module-scoping override to make AmdSevDxe use the IO port for PCI reads.
>>  * Use the reserved memory type for CPUID and Secrets page.
>>  *
>> Changes since v1:
>>  * Drop the interval tree support to detect the pre-validated overlap region.
>>  * Use an array to keep track of pre-validated regions.
>>  * Add support to query the Hypervisor feature and verify that SNP feature is supported.
>>  * Introduce MemEncryptSevClearMmioPageEncMask() to clear the C-bit from MMIO ranges.
>>  * Pull the SevSecretDxe and SevSecretPei into OVMF package build.
>>  * Extend the SevSecretDxe to expose confidential computing blob location through
>>    EFI configuration table.
>>
>> Brijesh Singh (21):
>>   UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs
>>   OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled()
>>   OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled
>>     field
>>   OvmfPkg/MemEncryptSevLib: extend Es Workarea to include hv features
>>   OvmfPkg: reserve Secrets page in MEMFD
>>   OvmfPkg: reserve CPUID page for the SEV-SNP guest
>>   OvmfPkg/ResetVector: validate the data pages used in SEC phase
>>   OvmfPkg/ResetVector: invalidate the GHCB page
>>   OvmfPkg: add library to support registering GHCB GPA
>>   OvmfPkg/PlatformPei: register GHCB gpa for the SEV-SNP guest
>>   UefiCpuPkg/MpLib: add support to register GHCB GPA when SEV-SNP is
>>     enabled
>>   OvmfPkg/AmdSevDxe: do not use extended PCI config space
>>   OvmfPkg/MemEncryptSevLib: add support to validate system RAM
>>   OvmfPkg/BaseMemEncryptSevLib: skip the pre-validated system RAM
>>   OvmfPkg/MemEncryptSevLib: add support to validate > 4GB memory in PEI
>>     phase
>>   OvmfPkg/SecMain: pre-validate the memory used for decompressing Fv
>>   OvmfPkg/PlatformPei: validate the system RAM when SNP is active
>>   OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table
>>   OvmfPkg/MemEncryptSevLib: skip page state change for Mmio address
>>   OvmfPkg/AmdSev: expose the SNP reserved pages through configuration
>>     table
>>   MdePkg/GHCB: increase the GHCB protocol max version
>>
>> Tom Lendacky (1):
>>   UefiCpuPkg/MpInitLib: Use SEV-SNP AP Creation NAE event to launch APs
>>
>>  OvmfPkg/OvmfPkg.dec                           |  21 ++
>>  UefiCpuPkg/UefiCpuPkg.dec                     |  11 +
>>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |   5 +-
>>  OvmfPkg/Bhyve/BhyveX64.dsc                    |   5 +-
>>  OvmfPkg/OvmfPkgIa32.dsc                       |   2 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   7 +-
>>  OvmfPkg/OvmfPkgX64.dsc                        |   8 +-
>>  OvmfPkg/OvmfXen.dsc                           |   5 +-
>>  OvmfPkg/OvmfPkgX64.fdf                        |  17 +-
>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf        |   4 +
>>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf        |   1 +
>>  .../DxeMemEncryptSevLib.inf                   |   3 +
>>  .../PeiMemEncryptSevLib.inf                   |   7 +
>>  .../SecMemEncryptSevLib.inf                   |   3 +
>>  .../GhcbRegisterLib/GhcbRegisterLib.inf       |  33 +++
>>  OvmfPkg/PlatformPei/PlatformPei.inf           |   5 +
>>  OvmfPkg/ResetVector/ResetVector.inf           |   4 +
>>  OvmfPkg/Sec/SecMain.inf                       |   3 +
>>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   4 +
>>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   4 +
>>  MdePkg/Include/Register/Amd/Ghcb.h            |   2 +-
>>  .../Guid/ConfidentialComputingSecret.h        |  18 ++
>>  OvmfPkg/Include/Library/GhcbRegisterLib.h     |  27 ++
>>  OvmfPkg/Include/Library/MemEncryptSevLib.h    |  31 +-
>>  .../X64/SnpPageStateChange.h                  |  31 ++
>>  .../BaseMemEncryptSevLib/X64/VirtualMemory.h  |  19 ++
>>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |  19 ++
>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c          |  22 ++
>>  OvmfPkg/AmdSev/SecretPei/SecretPei.c          |  15 +-
>>  .../DxeMemEncryptSevLibInternal.c             |  27 ++
>>  .../Ia32/MemEncryptSevLib.c                   |  17 ++
>>  .../PeiMemEncryptSevLibInternal.c             |  27 ++
>>  .../SecMemEncryptSevLibInternal.c             |  19 ++
>>  .../X64/DxeSnpSystemRamValidate.c             |  40 +++
>>  .../X64/PeiDxeVirtualMemory.c                 | 167 ++++++++++-
>>  .../X64/PeiSnpSystemRamValidate.c             | 126 ++++++++
>>  .../X64/SecSnpSystemRamValidate.c             |  36 +++
>>  .../X64/SnpPageStateChangeInternal.c          | 230 +++++++++++++++
>>  .../Library/GhcbRegisterLib/GhcbRegisterLib.c |  97 +++++++
>>  OvmfPkg/PlatformPei/AmdSev.c                  |  81 ++++++
>>  OvmfPkg/PlatformPei/MemDetect.c               |  12 +
>>  OvmfPkg/Sec/SecMain.c                         | 106 +++++++
>>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  11 +-
>>  .../MpInitLib/Ia32/SevSnpRmpAdjustInternal.c  |  31 ++
>>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 274 ++++++++++++++++--
>>  .../MpInitLib/X64/SevSnpRmpAdjustInternal.c   |  44 +++
>>  OvmfPkg/FvmainCompactScratchEnd.fdf.inc       |   5 +
>>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm  |  23 ++
>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm     | 227 +++++++++++++++
>>  OvmfPkg/ResetVector/ResetVector.nasmb         |   6 +
>>  UefiCpuPkg/Library/MpInitLib/MpEqu.inc        |   1 +
>>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm |  51 ++++
>>  52 files changed, 1956 insertions(+), 38 deletions(-)
>>  create mode 100644 OvmfPkg/Library/GhcbRegisterLib/GhcbRegisterLib.inf
>>  create mode 100644 OvmfPkg/Include/Library/GhcbRegisterLib.h
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChange.h
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
>>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
>>  create mode 100644 OvmfPkg/Library/GhcbRegisterLib/GhcbRegisterLib.c
>>  create mode 100644 UefiCpuPkg/Library/MpInitLib/Ia32/SevSnpRmpAdjustInternal.c
>>  create mode 100644 UefiCpuPkg/Library/MpInitLib/X64/SevSnpRmpAdjustInternal.c
>>