Re: [edk2-devel] [PATCH v7 33/43] OvmfPkg: Reserve a page in memory for the SEV-ES usage

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 4/30/20 4:12 PM, 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.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@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 think I might want to protect the area from DXE allocations, too. Is 
there an easy way to detect that PEI is active vs DXE? Even so, will it 
*always* be the case that PEI will start the APs first? I'd hate to see a 
change down the road where PEI doesn't start the APs and then things break.

Thanks,
Tom

> 
> 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.
> 
> Thanks,
> Tom
> 
>>
>> Thanks!
>> Laszlo
>>