On 04/30/20 23:12, Tom Lendacky wrote: > On 4/30/20 1:58 PM, Laszlo Ersek wrote: >> Hi Tom, > > Hi Laszlo, > >> >> On 04/22/20 19:41, Lendacky, Thomas wrote: >>> BZ: >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7Cce256f35aa2e4748e8e008d7ed3874ae%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637238699042461059&sdata=tXX8nkBo3fB4OVTs2avevW8pwL6AcqJHvFhvlshKySI%3D&reserved=0 >>> >>> >>> 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 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >>> index 36414c1f8b49..a0bea86f9875 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 >>> >>> >> >> in patch #28 ("OvmfPkg: Create a GHCB page for use during Sec phase"), >> we carve out two ranges in FD.MEMFD, and introduce a matching set of 4 >> PCDs. >> >> Then in patch #29 ("OvmfPkg/PlatformPei: Reserve GHCB-related areas if >> S3 is supported"), we reserve those ranges from the OS, as AcpiNVS, if >> S3 is supported. The reason we only reserve those ranges if S3 is >> enabled because the ranges are only needed in SEC. (See the details in >> the commit mesage of patch #29.) >> >> In this patch (patch #33), we carve out a third region in FD.MEMFD. We >> don't seem to ever reserve it. I think that's minimally a problem for >> S3; the same argument should apply as to the other two areas. Do you >> agree? > > Nice catch! Yes, I missed this one. > >> >> >> Furthermore, I wonder if we should reserve this "work area" from the OS, >> and even from the DXE phase (!), *regardless* of S3. I can't immediately >> tell when it's the last time (with S3 disabled) when this area is used. >> >> As I understand it, it is only used the first time the APs are booted >> up. And that should happen still in the PEI phase, because CpuMpPei >> boots up all the APs and counts them. Afterwards (still in the PEI >> phase), the APs should be sleeping in ApWakeupFunction(), namely in the >> code added by patch #40 ("UefiCpuPkg: Allow AP booting under SEV-ES"). >> If the AP is woken again, it is actually only "released" by the >> hypervisor, and it goes through the special 64bit->16bit transition, >> again implemented in patch#40. >> >> So ultimately it shouldn't be necessary to reserve this third region (at >> PcdSevEsWorkAreaBase), if S3 is disabled, because it is never used past >> the very first AP boot (which happens when CpuMpPei counts the APs). >> >> Do I understand right? > > Yes, that is correct. So I just need to do the same thing for this area > that I did in patch #29. > > I can probably shift patch #29 after #33 and have one patch for the S3 > reservation instead of having two separate patches doing S3 reservation.
Sounds good, thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#58636): https://edk2.groups.io/g/devel/message/58636 Mute This Topic: https://groups.io/mt/73201943/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
