Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES

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

On 10/2/19 10:15 AM, Laszlo Ersek via Groups.Io wrote:
> Adding Phil.
> 
> I'm looking at this patch only because one thing caught my attention in
> the previous one, "OvmfPkg: Add support for SEV-ES AP reset vector
> re-directing":
> 
> On 09/19/19 21:53, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> Typically, an AP is booted using the INIT-SIPI-SIPI sequence. This
>> sequence is intercepted by the hypervisor, which sets the AP's registers
>> to the values requested by the sequence. At that point, the hypervisor can
>> start the AP, which will then begin execution at the appropriate location.
>>
>> Under SEV-ES, AP booting presents some challenges since the hypervisor is
>> not allowed to alter the AP's register state. In this situation, we have
>> to distinguish between the AP's first boot and AP's subsequent boots.
>>
>> First boot:
>>  Once the AP's register state has been defined (which is before the guest
>>  is first booted) it cannot be altered. Should the hypervisor attempt to
>>  alter the register state, the change would be detected by the hardware
>>  and the VMRUN instruction would fail. Given this, the first boot for the
>>  AP is required to begin execution with this initial register state, which
>>  is typically the reset vector. This prevents the BSP from directing the
>>  AP startup location through the INIT-SIPI-SIPI sequence.
>>
>>  To work around this, provide a four-byte field at offset 0xffffffd0 that
>>  can contain an IP / CS register combination, that if non-zero, causes
>>  the AP to perform a far jump to that location instead of a near jump to
>>  EarlyBspInitReal16. Before booting the AP for the first time, the BSP
>>  should set the IP / CS value for the AP based on the value that would be
>>  derived from the INIT-SIPI-SIPI sequence.
> 
> I don't understand how this can work: the guest-phys address 0xffffffd0
> is backed by read-only pflash in most OVMF deployments.
> 
> In addition:
> 
> [...]
> 
>> @@ -1002,6 +1204,7 @@ WakeUpAP (
>>        CpuMpData->InitFlag   != ApInitDone) {
>>      ResetVectorRequired = TRUE;
>>      AllocateResetVector (CpuMpData);
>> +    AllocateSevEsAPMemory (CpuMpData);
>>      FillExchangeInfoData (CpuMpData);
>>      SaveLocalApicTimerSetting (CpuMpData);
>>    }
>> @@ -1038,6 +1241,15 @@ WakeUpAP (
>>        }
>>      }
>>      if (ResetVectorRequired) {
>> +      //
>> +      // For SEV-ES, set the jump address for initial AP boot
>> +      //
>> +      if (CpuMpData->SevEsActive) {
>> +        SEV_ES_AP_JMP_FAR *JmpFar = (SEV_ES_AP_JMP_FAR *)0xFFFFFFD0;
>> +
>> +        JmpFar->ApStart.Rip = 0;
>> +        JmpFar->ApStart.Segment = (UINT16) (ExchangeInfo->BufferStart >> 4);
>> +      }
> 
> Even if the address is backed by a single "unified" pflash, mapped r/w
> -- which we can call a "non-standard OVMF deployment" nowadays --, a
> normal store doesn't appear sufficient to me. The first write to pflash
> will flip it to "programming mode", and the values stored are supposed
> to be pflash commands (not just the raw data we intend to put in place).

There is a corresponding patch in Qemu that does not set the
KVM_MEM_READONLY flag for the ROM when starting an SEV-ES guest, thus
allowing the MP library to update the location with desired address.

> 
> See for example the QemuFlashWrite() function in
> "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c". (But, again, that
> is used with the pflash chip that hosts the variable store, and is
> therefore mapped r/w.)
> 
> 
> Taking a step back... I don't think APs execute any code from pflash,
> when MpInitLib boots them.

Correct, a non-SEV-ES guest will not execute this code because the
INIT-SIPI-SIPI sequence will direct the RIP to the desired location.

Thanks,
Tom

> 
> In OVMF, PcdCpuApLoopMode is ApInHltLoop (value 1), therefore
> "CpuMpData->WakeUpByInitSipiSipi" should be TRUE, and
> "ResetVectorRequired" too should be TRUE, at first AP boot.
> Consequently, the reset vector seems to be allocated with
> AllocateResetVector().
> 
> AllocateResetVector() has separate implementations for PEI and DXE, but
> in both cases, it returns RAM. So I don't see where the AP accesses (or
> executes) pflash.
> 
> Thanks,
> Laszlo
> 
> 
>