From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 02 Oct 2019 08:26:17 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DCEC710275E2; Wed, 2 Oct 2019 15:26:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-71.rdu2.redhat.com [10.10.120.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id CFB2760BE0; Wed, 2 Oct 2019 15:26:11 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 38/44] UefiCpuPkg: Allow AP booting under SEV-ES From: "Laszlo Ersek" To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <81e310d1f2929f839cd166d1c7de6694220743b6.1568922729.git.thomas.lendacky@amd.com> <284e15f0-25ee-bb69-dcd1-09e146346c69@redhat.com> Message-ID: Date: Wed, 2 Oct 2019 17:26:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <284e15f0-25ee-bb69-dcd1-09e146346c69@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.64]); Wed, 02 Oct 2019 15:26:17 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/02/19 17:15, Laszlo Ersek 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 >> >> 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. ... I believe I understand that this is precisely what cannot work under SEV-ES -- because we cannot launch an AP at an address that's dynamically chosen by the firmware (and passed to the hypervisor), like with INIT-SIPI-SIPI. And so firmware and hypervisor have to agree upon a *constant* AP reset vector address, in advance. We have two options: - pick the reset vector address *constant* such that it falls into RAM, or - let the AP reset vector reside in pflash, but then the code in pflash has to look for a parameter block at a fixed address in RAM. So in the end, both options require the same -- we need a RAM address constant that is determined at firmware build time. Either for the reset vector itself, or for the reset vector's parameter block. Thanks Laszlo