(I missed adding devel@edk2.groups.io, resending the series)

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

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://tinyurl.com/pu6admks

Additional resources
---------------------
SEV-SNP whitepaper
https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)

The complete source is available at
https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-2

GHCB spec:
https://developer.amd.com/wp-content/resources/56421.pdf

SEV-SNP firmware specification:
https://developer.amd.com/sev/
	         
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

-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75716): https://edk2.groups.io/g/devel/message/75716
Mute This Topic: https://groups.io/mt/83113760/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 05/27/21 01:10, Brijesh Singh wrote:
> (I missed adding devel@edk2.groups.io, resending the series)
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> 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://tinyurl.com/pu6admks
> 
> Additional resources
> ---------------------
> SEV-SNP whitepaper
> https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf
> 
> APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)
> 
> The complete source is available at
> https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-2
> 
> GHCB spec:
> https://developer.amd.com/wp-content/resources/56421.pdf
> 
> SEV-SNP firmware specification:
> https://developer.amd.com/sev/
> 	         
> 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
> 

I'm confirming that this series is in my review queue.

However, I may need unusually long time to get to it. Thanks for your
patience.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75793): https://edk2.groups.io/g/devel/message/75793
Mute This Topic: https://groups.io/mt/83113760/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 05/27/21 11:42, Laszlo Ersek wrote:
> On 05/27/21 01:10, Brijesh Singh wrote:

>> 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.

> I'm confirming that this series is in my review queue.
> 
> However, I may need unusually long time to get to it. Thanks for your
> patience.

My hope is that I can at least start reviewing this series tomorrow.
Sorry about the delay, I've been working on my todo list the best I can.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75986): https://edk2.groups.io/g/devel/message/75986
Mute This Topic: https://groups.io/mt/83113760/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Brijesh,

On 05/27/21 01:10, Brijesh Singh wrote:
> (I missed adding devel@edk2.groups.io, resending the series)
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> 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://tinyurl.com/pu6admks

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://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf
>
> APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)
>
> The complete source is available at
> https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-2

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://github.com/AMDESE/ovmf>
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)?

Thanks,
Laszlo

>
> GHCB spec:
> https://developer.amd.com/wp-content/resources/56421.pdf
>
> SEV-SNP firmware specification:
> https://developer.amd.com/sev/
>
> 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
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76054): https://edk2.groups.io/g/devel/message/76054
Mute This Topic: https://groups.io/mt/83113760/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76070): https://edk2.groups.io/g/devel/message/76070
Mute This Topic: https://groups.io/mt/83113760/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 06/04/21 13:50, Brijesh Singh wrote:
> 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.

Hmmm... indeed, but that discussion happened off-list, namely under the
original posting of this v3 RFC set that did not reach the list. And now
I was working with my list folder.
> 
> https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-3
> 
> This branch was based on commit 5531fd48ded1271b8775725355ab83994e4bc77c
> from the upstream. 

Right, this branch indeed averts problem (2); it is in sync with the
posted series. Thanks!

Problem (1) stays the same -- git-rebase reports the same issue as
git-am above, for patch#21. So, I'm going to review this version on the
list, but I'll skip patch#21 (or perhaps I'll attempt to make useful
comments there too, if I can).

Thanks!
Laszlo


> 
> 
>> 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
>>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76071): https://edk2.groups.io/g/devel/message/76071
Mute This Topic: https://groups.io/mt/83113760/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 06/04/21 15:09, Laszlo Ersek wrote:
> On 06/04/21 13:50, Brijesh Singh wrote:

>> 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.
> 
> Hmmm... indeed, but that discussion happened off-list, namely under the
> original posting of this v3 RFC set that did not reach the list. And now
> I was working with my list folder.
>>
>> https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-3
>>
>> This branch was based on commit 5531fd48ded1271b8775725355ab83994e4bc77c
>> from the upstream. 
> 
> Right, this branch indeed averts problem (2); it is in sync with the
> posted series. Thanks!
> 
> Problem (1) stays the same -- git-rebase reports the same issue as
> git-am above, for patch#21. So, I'm going to review this version on the
> list, but I'll skip patch#21 (or perhaps I'll attempt to make useful
> comments there too, if I can).

I re-reviewed patch #3 today, and reviewed patch #4 as well.

Because the data flow was not explained in advance, regarding the
"SevSnpEnabled" and "HypervisorFeatures" fields, I wasted a huge amount
of time reviewing ResetVector details that ultimately proved irrelevant.

Additionally, I've found that several patches modify multiple modules in
one step, typically ResetVector and PlatformPei. Honestly, this is
inexplicable to me, given the edk2 coding style. The edk2 style permits
multi-module patches only in the most exceptional cases. In a typical
producer/consumer setup, the producer module patch comes first, the
consumer module patch comes second. Breaking this edk2 rule is very
detrimental to my efficiency as a reviewer.

Either way, I'm done reviewing RFCv3; primarily because tomorrow and the
day after I have some time-sensitive work to do, and afterwards, I'm
going to disapper for a good while, to salvage the shreds of my brain
that I've been left with, after the last two weeks.

In the meantime, assuming no other reviewer wishes to comment RFCv3,
please rework this series carefully -- even if it has "RFC" status,
that's not a license to break edk2 coding style, structure patches
incorrectly, diverge from spec-dictated constants, omit detailed
explanations in commit messages (data flow!), duplicate code (assembly
code!) mindlessly, and so on.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76126): https://edk2.groups.io/g/devel/message/76126
Mute This Topic: https://groups.io/mt/83113760/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-