From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.33612.1590422445132695888 for ; Mon, 25 May 2020 09:00:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dV89PJqI; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590422444; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=COchHC0IHWP4KovSyedKNS4Txy6IxZvDJbCwJhvJZzI=; b=dV89PJqI5MGPyremHm+4Uw8YuxtPsc9bGhn6+zmoXwxxj5slhpwZKz7SdcxgWw+ttUTW4t IOgmeAh5x3QdqwztGjpl313AJE+zAJmu8JsFdssq+6Dd6q3HWHosmXtkfabQR3XDV7XwAv N7VYZMFowKP2xWlRcPXkXm4yeyJke4o= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-58-gumEmOYgMNWMTRDZuLq7NA-1; Mon, 25 May 2020 12:00:29 -0400 X-MC-Unique: gumEmOYgMNWMTRDZuLq7NA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4533191164; Mon, 25 May 2020 16:00:28 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-168.ams2.redhat.com [10.36.114.168]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0EC8410013D5; Mon, 25 May 2020 16:00:25 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 34/46] OvmfPkg: Reserve a page in memory for the SEV-ES usage To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: <5f3a4f30804261206adde675b983f42b777dd5d8.1589925074.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <0f3cdd22-189d-3980-7639-7e7f58f909cb@redhat.com> Date: Mon, 25 May 2020 18:00:25 +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: <5f3a4f30804261206adde675b983f42b777dd5d8.1589925074.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 05/19/20 23:51, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 > > 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Reviewed-by: Laszlo Ersek > Signed-off-by: Tom Lendacky > --- > OvmfPkg/OvmfPkgX64.fdf | 3 +++ > OvmfPkg/ResetVector/ResetVector.inf | 1 + > OvmfPkg/ResetVector/Ia32/PageTables64.asm | 11 +++++++++++ > OvmfPkg/ResetVector/ResetVector.nasmb | 1 + > 4 files changed, 16 insertions(+) > > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index 88b1e880e603..8836b30a0cef 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 > > diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf > index 483fd90fe785..e94e1bfcce7e 100644 > --- a/OvmfPkg/ResetVector/ResetVector.inf > +++ b/OvmfPkg/ResetVector/ResetVector.inf > @@ -34,6 +34,7 @@ [BuildOptions] > *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ > > [Pcd] > + gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > index c3587a1b7814..73a4eaadb1b6 100644 > --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > @@ -89,6 +89,10 @@ SevExit: > ; If SEV-ES is disabled then EAX will be zero. > ; > CheckSevEsFeature: > + ; Initialize the first byte of the workarea to zero to communicate to > + ; the SEC phase that SEV-ES is not enabled. > + mov byte[SEV_ES_WORK_AREA], 0 > + > xor eax, eax > > ; SEV-ES can't be enabled if SEV isn't, so first check the encryption > @@ -108,6 +112,13 @@ CheckSevEsFeature: > ; Restore encryption mask > mov edx, ebx > > + test eax, eax > + jz NoSevEs > + > + ; Set the first byte of the workarea to one to communicate to the SEC > + ; phase that SEV-ES is enabled. > + mov byte[SEV_ES_WORK_AREA], 1 > + > NoSevEs: > OneTimeCallRet CheckSevEsFeature > > diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb > index bfb77e439105..2967617bfaa0 100644 > --- a/OvmfPkg/ResetVector/ResetVector.nasmb > +++ b/OvmfPkg/ResetVector/ResetVector.nasmb > @@ -72,6 +72,7 @@ > %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase)) > %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase)) > %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize)) > + %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase)) > %include "Ia32/PageTables64.asm" > %endif > > The OvmfPkg/ResetVector modifications have been moved to this patch, at least in part, from patch "OvmfPkg/ResetVector: Add support for a 32-bit SEV check". And I don't understand why. I mean it's possible that setting the first byte of the work area to 1 does not belong in "OvmfPkg/ResetVector: Add support for a 32-bit SEV check". That's OK; then said manipulation of the work area should be split to its own patch, which I should then review afresh. What's not OK is to move code between two reviewed patches *and* keep my R-b on both. Please be more transparent about incremental changes. (1) Please revert this patch to its v7 state, and keep my R-b on it. (2) Please split the ResetVector changes to a new patch. For the subject line, I suggest: OvmfPkg/ResetVector: communicate SEV-ES status to SEC before exceptions or something similar. Thanks Laszlo