On 06/07/21 14:26, Laszlo Ersek wrote: > On 05/27/21 01:11, Brijesh Singh wrote: >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 >> >> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a >> secrets page. > > For pure SEV? > >> >> When SEV-SNP is enabled, the secrets page contains the VM platform >> communication keys. The guest BIOS and OS can use this key to communicate >> with the SEV firmware to get attesation report. See the SEV-SNP firmware >> spec for more details for the content of the secrets page. >> >> When SEV and SEV-ES is enabled, the secrets page contains the information >> provided by the guest owner after the attestation. See the SEV >> LAUNCH_SECRET command for more details. >> >> Cc: James Bottomley <j...@linux.ibm.com> >> Cc: Min Xu <min.m...@intel.com> >> Cc: Jiewen Yao <jiewen....@intel.com> >> Cc: Tom Lendacky <thomas.lenda...@amd.com> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Erdem Aktas <erdemak...@google.com> >> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com> >> --- >> OvmfPkg/OvmfPkgX64.dsc | 2 ++ >> OvmfPkg/OvmfPkgX64.fdf | 5 +++++ >> OvmfPkg/AmdSev/SecretPei/SecretPei.inf | 1 + >> OvmfPkg/AmdSev/SecretPei/SecretPei.c | 15 ++++++++++++++- >> 4 files changed, 22 insertions(+), 1 deletion(-) > > How is all of the above related to the "OvmfPkg/OvmfPkgX64.dsc" > platform, where remote attestation is not a goal? > > What you describe makes sense to me, but only for the remote-attested > "OvmfPkg/AmdSev/AmdSevX64.dsc" platform. (Which already includes > SecretPei and SecretDxe, and sets the necessary PCDs.) > > Then, even if we limit this patch only to the "OvmfPkg/AmdSev/SecretPei" > module, the commit message does not explain sufficiently why the secrets > page must be reserved for good. The "SEV-SNP firmware spec" reference is > vague at best; I'm permanently lost between the dozen PDF files I have > downloaded locally from the AMD website. Please include a specific > document number, revision number, and chapter/section identifier. > > Honestly I'm getting a *rushed* vibe on this whole series. Why is that? > > Assume that I'm dumb. You won't be far from the truth. Then hold my hand > through all this?
Here's the v2 discussion: - 9804ecb5-8afd-c56e-4982-d1a6ebad3de8@redhat.com">http://mid.mail-archive.com/9804ecb5-8afd-c56e-4982-d1a6ebad3de8@redhat.com - https://edk2.groups.io/g/devel/message/74797 - https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00112.html That discussion refers to a different use case, raised by Dov. That use case might justify reserving the area even for plain SEV. It's out of scope for now, AIUI. ( And even for that separate use case, James showed down-thread that *not* reserving the page forever in the firmware is more flexible. - aed7d3490fe6edee74440ed8e4cd5364fb2ba4af.camel@linux.ibm.com">http://mid.mail-archive.com/aed7d3490fe6edee74440ed8e4cd5364fb2ba4af.camel@linux.ibm.com - https://edk2.groups.io/g/devel/message/74801 - https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00116.html ) AFAICT, the only effect of the v2 sub-thread on the patch has been that we now use the Reserved memory type rather than AcpiNVS (when SEV-SNP is in use). I have two comments on that: - It's good that we're not mixing in the other use case raised by Dov (i.e., enabling the guest-kernel to read secrets from the injected page even under plain SEV). - It's still unclear to me why the reservation needs to be permanent under SEV-SNP. Thanks Laszlo >> >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index 999738dc39cd..ea08e1fabc65 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -716,6 +716,7 @@ [Components] >> OvmfPkg/SmmAccess/SmmAccessPei.inf >> !endif >> UefiCpuPkg/CpuMpPei/CpuMpPei.inf >> + OvmfPkg/AmdSev/SecretPei/SecretPei.inf >> >> !if $(TPM_ENABLE) == TRUE >> OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf >> @@ -966,6 +967,7 @@ [Components] >> OvmfPkg/PlatformDxe/Platform.inf >> OvmfPkg/AmdSevDxe/AmdSevDxe.inf >> OvmfPkg/IoMmuDxe/IoMmuDxe.inf >> + OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf >> >> !if $(SMM_REQUIRE) == TRUE >> OvmfPkg/SmmAccess/SmmAccess2Dxe.inf >> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >> index d6be798fcadd..9126b8eb5014 100644 >> --- a/OvmfPkg/OvmfPkgX64.fdf >> +++ b/OvmfPkg/OvmfPkgX64.fdf >> @@ -88,6 +88,9 @@ [FD.MEMFD] >> 0x00C000|0x001000 >> >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize >> >> +0x00D000|0x001000 >> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize >> + >> 0x010000|0x010000 >> >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >> >> @@ -179,6 +182,7 @@ [FV.PEIFV] >> INF SecurityPkg/Tcg/TcgPei/TcgPei.inf >> INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf >> !endif >> +INF OvmfPkg/AmdSev/SecretPei/SecretPei.inf >> >> >> ################################################################################ >> >> @@ -314,6 +318,7 @@ [FV.DXEFV] >> INF ShellPkg/Application/Shell/Shell.inf >> >> INF MdeModulePkg/Logo/LogoDxe.inf >> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf >> >> # >> # Network modules >> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >> b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >> index 08be156c4bc0..9265f8adee12 100644 >> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf >> @@ -26,6 +26,7 @@ [LibraryClasses] >> HobLib >> PeimEntryPoint >> PcdLib >> + MemEncryptSevLib >> >> [FixedPcd] >> gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase >> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c >> b/OvmfPkg/AmdSev/SecretPei/SecretPei.c >> index ad491515dd5d..51eb094555aa 100644 >> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c >> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c >> @@ -7,6 +7,7 @@ >> #include <PiPei.h> >> #include <Library/HobLib.h> >> #include <Library/PcdLib.h> >> +#include <Library/MemEncryptSevLib.h> >> >> EFI_STATUS >> EFIAPI >> @@ -15,10 +16,22 @@ InitializeSecretPei ( >> IN CONST EFI_PEI_SERVICES **PeiServices >> ) >> { >> + UINTN Type; >> + >> + // >> + // The location of the secret page should be marked reserved so that >> guest OS >> + // does not treated as a system RAM. >> + // >> + if (MemEncryptSevSnpIsEnabled ()) { >> + Type = EfiReservedMemoryType; >> + } else { >> + Type = EfiBootServicesData; >> + } >> + >> BuildMemoryAllocationHob ( >> PcdGet32 (PcdSevLaunchSecretBase), >> PcdGet32 (PcdSevLaunchSecretSize), >> - EfiBootServicesData >> + Type >> ); >> >> return EFI_SUCCESS; >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76128): https://edk2.groups.io/g/devel/message/76128 Mute This Topic: https://groups.io/mt/83113766/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
