From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web10.5991.1605772296016206735 for ; Wed, 18 Nov 2020 23:51:36 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MiWg6K/H; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605772295; 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=RfeIWh82HoEUtY5lrDlw+qPrO19A2s10BFZSmu5j4K0=; b=MiWg6K/H0Ws/qUxXLuJ/z5vW2z792dcLH/9A8shZLeBVafz+oZc+9pyqOs7tbMl/s9TwWn S9IfdRmFkphrmcBvvx4uiCzcrpOjc5NsPEQDhpuuqDkQdBT9Lu6jjCDqxpSAOO9nNpZc4D YjSPeERPRX04UWFZgP1naVb9DbaRQns= 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-202-aJDU6IGGPHOYsU9DL2esuQ-1; Thu, 19 Nov 2020 02:51:31 -0500 X-MC-Unique: aJDU6IGGPHOYsU9DL2esuQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8324D8144E1; Thu, 19 Nov 2020 07:51:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-236.ams2.redhat.com [10.36.112.236]) by smtp.corp.redhat.com (Postfix) with ESMTP id CD9BD5D6A8; Thu, 19 Nov 2020 07:51:26 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/4] OvmfPkg: create a SEV secret area in the AmdSev memfd To: Tom Lendacky , devel@edk2.groups.io, jejb@linux.ibm.com Cc: dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, ashish.kalra@amd.com, brijesh.singh@amd.com, tobin@ibm.com, david.kaplan@amd.com, jon.grimm@amd.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" References: <20201112001316.11341-1-jejb@linux.ibm.com> <20201112001316.11341-4-jejb@linux.ibm.com> <6db69ccd-340f-2df2-718b-5f7db09da0b8@redhat.com> <76c21922-aaa5-4ae2-b4b9-055f0720d4ef@amd.com> From: "Laszlo Ersek" Message-ID: <7cc1b7f4-a00b-798f-377a-48d3fadc44ad@redhat.com> Date: Thu, 19 Nov 2020 08:51:25 +0100 MIME-Version: 1.0 In-Reply-To: <76c21922-aaa5-4ae2-b4b9-055f0720d4ef@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 11/18/20 21:39, Tom Lendacky wrote: > On 11/16/20 4:46 PM, Laszlo Ersek via groups.io wrote: >> On 11/12/20 01:13, James Bottomley wrote: >>> SEV needs an area to place an injected secret where OVMF can find it >>> and pass it up as a ConfigurationTable.  This patch implements the >>> area itself as an addition to the SEV enhanced reset vector.  The >>> reset vector scheme allows additions but not removals.  If the size of >>> the reset vector is 22, it only contains the AP reset IP, but if it is >>> 30 (or greater) it contains the SEV secret page location and size. >>> >>> Signed-off-by: James Bottomley >>> --- >>>   OvmfPkg/OvmfPkg.dec                          | 5 +++++ >>>   OvmfPkg/AmdSev/AmdSevX64.fdf                 | 3 +++ >>>   OvmfPkg/ResetVector/ResetVector.inf          | 4 ++++ >>>   OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 4 ++++ >>>   OvmfPkg/ResetVector/ResetVector.nasmb        | 2 ++ >>>   5 files changed, 18 insertions(+) >>> > > ... > >>> ; >>> ; SEV-ES Processor Reset support >>> ; >>> ; sevEsResetBlock: >>> ;   For the initial boot of an AP under SEV-ES, the "reset" RIP must be >>> ;   programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A known >>> offset >>> ;   and GUID will be used to locate this block in the firmware and >>> extract >>> ;   the build time RIP value. The GUID must always be 48 bytes from the >>> ;   end of the firmware. >>> ; >>> ;   0xffffffca (-0x36) - IP value >>> ;   0xffffffcc (-0x34) - CS segment base [31:16] >>> ;   0xffffffce (-0x32) - Size of the SEV-ES reset block >>> ;   0xffffffd0 (-0x30) - SEV-ES reset block GUID >>> ;                        (00f771de-1a7e-4fcb-890e-68c77e2fb44e) >>> ; >>> ;   A hypervisor reads the CS segement base and IP value. The CS >>> segment base >>> ;   value represents the high order 16-bits of the CS segment base, >>> so the >>> ;   hypervisor must left shift the value of the CS segement base by >>> 16 bits to >>> ;   form the full CS segment base for the CS segment register. It >>> would then >>> ;   program the EIP register with the IP value as read. >>> ; >>> >>> TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0 >>> >>> sevEsResetBlockStart: >>>      DD      SEV_ES_AP_RESET_IP >>>      DW      sevEsResetBlockEnd - sevEsResetBlockStart >>>      DB      0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F >>>      DB      0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E >>> sevEsResetBlockEnd: >> >> I'm not exactly sure why we added the padding (TIMES ... DB 0) in edk2 >> commit 30937f2f98c4 ("OvmfPkg: Use the SEV-ES work area for the SEV-ES >> AP reset vector", 2020-08-17). I can imagine it was *already* for the >> same purpose -- to deterministically terminate the above-described >> backwards-traversal of the GUID-ed structures (and at the same time >> remain aligned to 32 bytes, regarding the cumulative size of all >> provided structures). > > The padding is required to "push" the GUID into the proper location at > exactly 48 bytes from the end of the file. Without the padding, the GUID > doesn't line up correctly and can't be located. OK, thanks! So I think that my proposed movement / reworking of the prepended padding should be fine. Laszlo > > Thanks, > Tom > >> >> So, in that vein, I'd propose something like this (relative to master @ >> d448574e7310): >> >>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>> index 980e0138e7fe..957356ff997e 100644 >>> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>> @@ -16,55 +16,83 @@ ALIGN   16 >>>   ; Pad the image size to 4k when page tables are in VTF0 >>>   ; >>>   ; If the VTF0 image has page tables built in, then we need to make >>>   ; sure the end of VTF0 is 4k above where the page tables end. >>>   ; >>>   ; This is required so the page tables will be 4k aligned when VTF0 is >>>   ; located just below 0x100000000 (4GB) in the firmware device. >>>   ; >>>   %ifdef ALIGN_TOP_TO_4K_FOR_PAGING >>>       TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0 >>>   %endif >>> >>> +; >>> +; pre-pad the sequence of GUIDed structures to a multiple of 32 bytes >>> +; >>> +TIMES (31 - (guidedStructuresEnd - guidedStructuresStart + 31) % 32) >>> DB 0 >>> + >>> +guidedStructuresStart: >>> +; >>> +; Zero GUID to terminate decreasing address order traversal. >>> +; >>> +TIMES 16 DB 0 >>> + >>> +; >>> +; Expose the location of the SEV Launch Secret area to the hypervisor >>> +; (necessary when using the remote attestation firmware platform). >>> +; >>> +; sevLaunchSecretDescriptor: >>> +;   This GUIDed structure is chained in decreasing address order from >>> +;   sevEsResetBlock. It describes the guest RAM area where the >>> hypervisor has >>> +;   to securely inject the SEV Launch Secret. The GUID is >>> +;   78C93F1E-ADBC-4259-B92B-CE81E523FBC4. >>> +; >>> +sevLaunchSecretDescriptorStart: >>> +    DD      SEV_LAUNCH_SECRET_BASE >>> +    DD      SEV_LAUNCH_SECRET_SIZE >>> +    DW      sevLaunchSecretDescriptorEnd - >>> sevLaunchSecretDescriptorStart >>> +    DB      0x1E, 0x3F, 0xC9, 0x78, 0xBC, 0xAD, 0x59, 0x42 >>> +    DB      0xB9, 0x2B, 0xCE, 0x81, 0xE5, 0x23, 0xFB, 0xC4 >>> +sevLaunchSecretDescriptorEnd: >>> + >>>   ; >>>   ; SEV-ES Processor Reset support >>>   ; >>>   ; sevEsResetBlock: >>>   ;   For the initial boot of an AP under SEV-ES, the "reset" RIP >>> must be >>>   ;   programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A >>> known offset >>>   ;   and GUID will be used to locate this block in the firmware and >>> extract >>>   ;   the build time RIP value. The GUID must always be 48 bytes from >>> the >>>   ;   end of the firmware. >>>   ; >>>   ;   0xffffffca (-0x36) - IP value >>>   ;   0xffffffcc (-0x34) - CS segment base [31:16] >>>   ;   0xffffffce (-0x32) - Size of the SEV-ES reset block >>>   ;   0xffffffd0 (-0x30) - SEV-ES reset block GUID >>>   ;                        (00f771de-1a7e-4fcb-890e-68c77e2fb44e) >>>   ; >>>   ;   A hypervisor reads the CS segement base and IP value. The CS >>> segment base >>>   ;   value represents the high order 16-bits of the CS segment base, >>> so the >>>   ;   hypervisor must left shift the value of the CS segement base by >>> 16 bits to >>>   ;   form the full CS segment base for the CS segment register. It >>> would then >>>   ;   program the EIP register with the IP value as read. >>>   ; >>> >>> -TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0 >>> - >>>   sevEsResetBlockStart: >>>       DD      SEV_ES_AP_RESET_IP >>>       DW      sevEsResetBlockEnd - sevEsResetBlockStart >>>       DB      0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F >>>       DB      0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E >>>   sevEsResetBlockEnd: >>> +guidedStructuresEnd: >>> >>>   ALIGN   16 >>> >>>   applicationProcessorEntryPoint: >>>   ; >>>   ; Application Processors entry point >>>   ; >>>   ; GenFv generates code aligned on a 4k boundary which will jump to >>> this >>>   ; location.  (0xffffffe0)  This allows the Local APIC Startup IPI >>> to be >>>   ; used to wake up the application processors. >>>   ; >>>       jmp     EarlyApInitReal16 >> >> Back to your patch: >> >> On 11/12/20 01:13, James Bottomley wrote: >>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb >>> b/OvmfPkg/ResetVector/ResetVector.nasmb >>> index 4913b379a9..c5e0fe93ab 100644 >>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb >>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb >>> @@ -83,5 +83,7 @@ >>>   %include "Main.asm" >>> >>>     %define SEV_ES_AP_RESET_IP  FixedPcdGet32 (PcdSevEsWorkAreaBase) >>> +  %define SEV_LAUNCH_SECRET_BASE  FixedPcdGet32 >>> (PcdSevLaunchSecretBase) >>> +  %define SEV_LAUNCH_SECRET_SIZE  FixedPcdGet32 >>> (PcdSevLaunchSecretSize) >>>   %include "Ia16/ResetVectorVtf0.asm" >>> >>> >> >> OK. >> >> Thanks, >> Laszlo >> >> >> >> >> >> >