On 05/19/20 23:51, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 > > Reserve a fixed area of memory for SEV-ES use and set a fixed PCD, > PcdSevEsWorkAreaBase, to this value. > > This area will be used by SEV-ES support for two purposes: > 1. Communicating the SEV-ES status during BSP boot to SEC: > Using a byte of memory from the page, the BSP reset vector code can > communicate the SEV-ES status to SEC for use before exception > handling can be enabled in SEC. After SEC, this field is no longer > valid and the standard way of determine if SEV-ES is active should > be used. > > 2. Establishing an area of memory for AP boot support: > A hypervisor is not allowed to update an SEV-ES guest's register > state, so when booting an SEV-ES guest AP, the hypervisor is not > allowed to set the RIP to the guest requested value. Instead an > SEV-ES AP must be re-directed from within the guest to the actual > requested staring location as specified in the INIT-SIPI-SIPI > sequence. > > Use this memory for reset vector code that can be programmed to have > the AP jump to the desired RIP location after starting the AP. This > is required for only the very first AP reset. > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> > --- > OvmfPkg/OvmfPkgX64.fdf | 3 +++ > OvmfPkg/ResetVector/ResetVector.inf | 1 + > OvmfPkg/ResetVector/Ia32/PageTables64.asm | 11 +++++++++++ > OvmfPkg/ResetVector/ResetVector.nasmb | 1 + > 4 files changed, 16 insertions(+) > > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index 88b1e880e603..8836b30a0cef 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -82,6 +82,9 @@ [FD.MEMFD] > 0x009000|0x002000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize > > +0x00B000|0x001000 > +gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize > + > 0x010000|0x010000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > diff --git a/OvmfPkg/ResetVector/ResetVector.inf > b/OvmfPkg/ResetVector/ResetVector.inf > index 483fd90fe785..e94e1bfcce7e 100644 > --- a/OvmfPkg/ResetVector/ResetVector.inf > +++ b/OvmfPkg/ResetVector/ResetVector.inf > @@ -34,6 +34,7 @@ [BuildOptions] > *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ > > [Pcd] > + gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > index c3587a1b7814..73a4eaadb1b6 100644 > --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > @@ -89,6 +89,10 @@ SevExit: > ; If SEV-ES is disabled then EAX will be zero. > ; > CheckSevEsFeature: > + ; Initialize the first byte of the workarea to zero to communicate to > + ; the SEC phase that SEV-ES is not enabled. > + mov byte[SEV_ES_WORK_AREA], 0 > + > xor eax, eax > > ; SEV-ES can't be enabled if SEV isn't, so first check the encryption > @@ -108,6 +112,13 @@ CheckSevEsFeature: > ; Restore encryption mask > mov edx, ebx > > + test eax, eax > + jz NoSevEs > + > + ; Set the first byte of the workarea to one to communicate to the SEC > + ; phase that SEV-ES is enabled. > + mov byte[SEV_ES_WORK_AREA], 1 > + > NoSevEs: > OneTimeCallRet CheckSevEsFeature > > diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb > b/OvmfPkg/ResetVector/ResetVector.nasmb > index bfb77e439105..2967617bfaa0 100644 > --- a/OvmfPkg/ResetVector/ResetVector.nasmb > +++ b/OvmfPkg/ResetVector/ResetVector.nasmb > @@ -72,6 +72,7 @@ > %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase)) > %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase)) > %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize)) > + %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase)) > %include "Ia32/PageTables64.asm" > %endif > >
The OvmfPkg/ResetVector modifications have been moved to this patch, at least in part, from patch "OvmfPkg/ResetVector: Add support for a 32-bit SEV check". And I don't understand why. I mean it's possible that setting the first byte of the work area to 1 does not belong in "OvmfPkg/ResetVector: Add support for a 32-bit SEV check". That's OK; then said manipulation of the work area should be split to its own patch, which I should then review afresh. What's not OK is to move code between two reviewed patches *and* keep my R-b on both. Please be more transparent about incremental changes. (1) Please revert this patch to its v7 state, and keep my R-b on it. (2) Please split the ResetVector changes to a new patch. For the subject line, I suggest: OvmfPkg/ResetVector: communicate SEV-ES status to SEC before exceptions or something similar. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#60214): https://edk2.groups.io/g/devel/message/60214 Mute This Topic: https://groups.io/mt/74336596/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
