From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web11.4281.1590580216975343423 for ; Wed, 27 May 2020 04:50:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eOJOQhxQ; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590580216; 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=S/MvRTLIs3mGjUqVJBv3PpPjirDIgYtFCdH9H7M0Ovw=; b=eOJOQhxQgVbi6jWN0mDUq7t7DmtL3i3pgVmi8rQNsg9kDvW+CstDwveRDavnMou9Hjjbh1 3gG4T1STiCLfALnRpKXI2L2QPatjy+LtUGE5K4F1L3LNOE5Cg/Lr4axGwKXKu6BZ7NvIXK YlAIToT+zSCbGmPeGVh1niQrsP8t/AI= 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-319-13oXSR-YPhinzVR1f7mMLw-1; Wed, 27 May 2020 07:50:12 -0400 X-MC-Unique: 13oXSR-YPhinzVR1f7mMLw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AF8EF835B46; Wed, 27 May 2020 11:50:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-77.ams2.redhat.com [10.36.113.77]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8F889648D7; Wed, 27 May 2020 11:50:08 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 34/46] OvmfPkg: Reserve a page in memory for the SEV-ES usage To: Tom Lendacky , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: <5f3a4f30804261206adde675b983f42b777dd5d8.1589925074.git.thomas.lendacky@amd.com> <0f3cdd22-189d-3980-7639-7e7f58f909cb@redhat.com> <69067fe2-093b-0699-f460-79c9b081440f@amd.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 27 May 2020 13:50:07 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 05/26/20 23:47, Tom Lendacky wrote: > On 5/26/20 9:28 AM, Tom Lendacky wrote: >> On 5/25/20 11:00 AM, Laszlo Ersek wrote: >>> On 05/19/20 23:51, 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%7C498df3e8d335449e596508d800c4c955%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260192476035384&sdata=UKux5gXwpNe59RKQHTyk577b%2B%2FBmTIdblij8JWhXBG4%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 >>>> 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 was trying to keep everything logically grouped. The early use of >> this area is to communicate the SEV-ES status to SEC and so logically >> I thought that should be done when the area was introduced. >> >>> >>> 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. >> >> Sorry about that. A bad assumption on my part about being able to do >> that here and in a few other places. >> >>> >>> Please be more transparent about incremental changes. >>> >>> (1) Please revert this patch to its v7 state, and keep my R-b on it. >> >> Will do. >> >>> >>> (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. >> >> Will do. > > Actually, these changes can remain part of the revert to the v6 version > of patch 36 ("OvmfPkg/ResetVector: Add support for a 32-bit SEV check") > so that no changes are seen in that patch from the original v6 that was > reviewed. Yes, that works perfectly fine too; I just thought you really wanted to move the "communicate SEV-ES status to SEC before exceptions" part to a different patch. If you are OK with keeping version 6 of the patch, I'm completely OK with that! When you post v9 of the series, please explain in the blurb what patches you've re-set to version 6. If you can use "git notes" (or manual editing) to mark the same information on the individual patches (outside of their commit messages), that's even better. Thanks! Laszlo