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

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

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

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.

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