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