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

["Laszlo Ersek" <lersek@redhat.com>]

On 05/01/20 00:09, Tom Lendacky wrote:
> 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.

OK. In such cases, the area in question is always covered with a memory
allocation HOB; what depends on S3 is only whether the memory type is
"boot services data" (S3 disabled on the QEMU cmdline) or AcpiNVS (S3
enabled). The region is then always kept away from DXE, and S3
enablement determines only whether the OS is told to stay away as well.

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

We'll have to continue including CpuMpPei, otherwise we couldn't re-set
the feature control MSR to the desired value at S3 resume. IOW, removing
CpuMpPei from OVMF would regress at least that feature:

https://bugzilla.tianocore.org/show_bug.cgi?id=86

Including CpuMpPei in OVMF was surprisingly quirky; I don't see it going
away anytime soon.

Thanks
Laszlo