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.2309.1591897990620073359 for ; Thu, 11 Jun 2020 10:53:10 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=B4/zuL6a; 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=1591897989; 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=qZmZS5qxsSag4ZaR44lJo7REhk5ZLYLmkSHv6s0lgj4=; b=B4/zuL6aeOPezcGqAO7qv20MavAj1lT1JkGh0WXAkIoeVZ5z9w7MRm35S6puWfvZ+ANE4g 9AcyU+jvXXhbqhYW5+bX+MY8fMqcuEhZnwgyiVceoFao1OtZ+hRsczkoCdUuOY0JXg0A0V pvemlVSn0i1FUtxHApDwLZIqjQQePd0= 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-508-ShUKw9UhOdWtoZ5aloAiqQ-1; Thu, 11 Jun 2020 13:53:04 -0400 X-MC-Unique: ShUKw9UhOdWtoZ5aloAiqQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8491918A8229; Thu, 11 Jun 2020 17:53:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-21.ams2.redhat.com [10.36.114.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id 484FF5C1B2; Thu, 11 Jun 2020 17:53:00 +0000 (UTC) Subject: Re: [PATCH v9 29/46] OvmfPkg: Create a GHCB page for use during Sec phase To: Tom Lendacky , devel@edk2.groups.io Cc: Brijesh Singh , Ard Biesheuvel , Eric Dong , Jordan Justen , Liming Gao , Michael D Kinney , Ray Ni References: <9cf7e23dbe3cfa648c909c13b0176afa2f79fb07.1591363657.git.thomas.lendacky@amd.com> <5e659466-b05c-6119-2ff3-c061bda1e649@redhat.com> <0c84585b-93d1-28d1-629c-5a0cbbdc10cc@amd.com> From: "Laszlo Ersek" Message-ID: <59213763-ed8c-bdb4-d25f-3b0f952be65c@redhat.com> Date: Thu, 11 Jun 2020 19:52:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <0c84585b-93d1-28d1-629c-5a0cbbdc10cc@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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 06/11/20 17:25, Tom Lendacky wrote: > On 6/11/20 4:56 AM, Laszlo Ersek wrote: >> Hi Tom, >> >> On 06/05/20 15:27, Tom Lendacky 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%7C38f855613c974b9f23e108d80dedc415%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637274662138263584&sdata=QfK586IbkE%2B8zOieyD4nQJ6ALvmzE2YlsNOnB9o7lpQ%3D&reserved=0 >>> >>> >>> A GHCB page is needed during the Sec phase, so this new page must be >>> created. Since the #VC exception handler routines assume that a per-CPU >>> variable area is immediately after the GHCB, this per-CPU variable area >>> must also be created. Since the GHCB must be marked as an un-encrypted, >>> or shared, page, an additional pagetable page is required to break down >>> the 2MB region where the GHCB page lives into 4K pagetable entries. >>> >>> Create a new entry in the OVMF memory layout for the new page table >>> page and for the SEC GHCB and per-CPU variable pages. After breaking >>> down >>> the 2MB page, update the GHCB page table entry to remove the encryption >>> mask. >>> >>> The GHCB page will be used by the SEC #VC exception handler. The #VC >>> exception handler will fill in the necessary fields of the GHCB and exit >>> to the hypervisor using the VMGEXIT instruction. The hypervisor then >>> accesses the GHCB in order to perform the requested function. >>> >>> Four new fixed PCDs are needed to support the SEC GHCB page: >>>    - PcdOvmfSecGhcbBase  UINT32 value that is the base address of the >>>                          GHCB used during the SEC phase. >>>    - PcdOvmfSecGhcbSize  UINT32 value that is the size, in bytes, of the >>>                          GHCB area used during the SEC phase. >>> >>>    - PcdOvmfSecGhcbPageTableBase  UINT32 value that is address of a page >>>                          table page used to break down the 2MB page into >>>                          512 4K pages. >>>    - PcdOvmfSecGhcbPageTableSize  UINT32 value that is the size, in >>> bytes, >>>                          of the page table page. >>> >>> Cc: Jordan Justen >>> Cc: Laszlo Ersek >>> Cc: Ard Biesheuvel >>> Signed-off-by: Tom Lendacky >>> --- >>>   OvmfPkg/OvmfPkg.dec                       |  9 +++ >>>   OvmfPkg/OvmfPkgX64.fdf                    |  6 ++ >>>   OvmfPkg/ResetVector/ResetVector.inf       |  5 ++ >>>   OvmfPkg/ResetVector/Ia32/PageTables64.asm | 76 ++++++++++++++++++++ >>>   OvmfPkg/ResetVector/ResetVector.nasmb     | 17 +++++ >>>   5 files changed, 113 insertions(+) >>> >>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec >>> index 65bb2bb0eb4c..02ad62ed9f43 100644 >>> --- a/OvmfPkg/OvmfPkg.dec >>> +++ b/OvmfPkg/OvmfPkg.dec >>> @@ -281,6 +281,15 @@ [PcdsFixedAtBuild] >>>     ## Number of page frames to use for storing grant table entries. >>>     gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33 >>>   +  ## Specify the extra page table needed to mark the GHCB as >>> unencrypted. >>> +  #  The value should be a multiple of 4KB for each. >>> +  >>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|0x0|UINT32|0x3a >>> +  >>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize|0x0|UINT32|0x3b >>> + >>> +  ## The base address of the SEC GHCB page used by SEV-ES. >>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x3c >>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x3d >>> + >>>   [PcdsDynamic, PcdsDynamicEx] >>>     gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2 >>>     >>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10 >>> >>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >>> index bfca1eff9e83..88b1e880e603 100644 >>> --- a/OvmfPkg/OvmfPkgX64.fdf >>> +++ b/OvmfPkg/OvmfPkgX64.fdf >>> @@ -76,6 +76,12 @@ [FD.MEMFD] >>>   0x007000|0x001000 >>>   >>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize >>> >>>   +0x008000|0x001000 >>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize >>> >>> + >>> +0x009000|0x002000 >>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >>> >>> + >>>   0x010000|0x010000 >>>   >>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >>> >>>   diff --git a/OvmfPkg/ResetVector/ResetVector.inf >>> b/OvmfPkg/ResetVector/ResetVector.inf >>> index b0ddfa5832a2..483fd90fe785 100644 >>> --- a/OvmfPkg/ResetVector/ResetVector.inf >>> +++ b/OvmfPkg/ResetVector/ResetVector.inf >>> @@ -26,6 +26,7 @@ [Sources] >>>   [Packages] >>>     OvmfPkg/OvmfPkg.dec >>>     MdePkg/MdePkg.dec >>> +  MdeModulePkg/MdeModulePkg.dec >>>     UefiCpuPkg/UefiCpuPkg.dec >>>     [BuildOptions] >>> @@ -33,5 +34,9 @@ [BuildOptions] >>>      *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ >>>     [Pcd] >>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase >>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase >>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize >>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase >>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize >>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm >>> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm >>> index abad009f20f5..9f86ddf6f08f 100644 >>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm >>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm >>> @@ -21,6 +21,11 @@ BITS    32 >>>   %define PAGE_2M_MBO            0x080 >>>   %define PAGE_2M_PAT          0x01000 >>>   +%define PAGE_4K_PDE_ATTR (PAGE_ACCESSED + \ >>> +                          PAGE_DIRTY + \ >>> +                          PAGE_READ_WRITE + \ >>> +                          PAGE_PRESENT) >>> + >>>   %define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \ >>>                             PAGE_ACCESSED + \ >>>                             PAGE_DIRTY + \ >>> @@ -75,6 +80,37 @@ NoSev: >>>   SevExit: >>>       OneTimeCallRet CheckSevFeature >>>   +; Check if Secure Encrypted Virtualization - Encrypted State >>> (SEV-ES) feature >>> +; is enabled. >>> +; >>> +; Modified:  EAX, EBX, ECX >>> +; >>> +; If SEV-ES is enabled then EAX will be non-zero. >>> +; If SEV-ES is disabled then EAX will be zero. >>> +; >>> +CheckSevEsFeature: >>> +    xor       eax, eax >>> + >>> +    ; SEV-ES can't be enabled if SEV isn't, so first check the >>> encryption >>> +    ; mask. >>> +    test      edx, edx >>> +    jz        NoSevEs >>> + >>> +    ; Save current value of encryption mask >>> +    mov       ebx, edx >>> + >>> +    ; Check if SEV-ES is enabled >>> +    ;  MSR_0xC0010131 - Bit 1 (SEV-ES enabled) >>> +    mov       ecx, 0xc0010131 >>> +    rdmsr >>> +    and       eax, 2 >>> + >>> +    ; Restore encryption mask >>> +    mov       edx, ebx >>> + >>> +NoSevEs: >>> +    OneTimeCallRet CheckSevEsFeature >>> + >>>   ; >>>   ; Modified:  EAX, EBX, ECX, EDX >>>   ; >>> @@ -139,6 +175,46 @@ pageTableEntriesLoop: >>>       mov     [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx >>>       loop    pageTableEntriesLoop >>>   +    OneTimeCall   CheckSevEsFeature >>> +    test    eax, eax >>> +    jz      SetCr3 >>> + >>> +    ; >>> +    ; The initial GHCB will live at GHCB_BASE and needs to be >>> un-encrypted. >>> +    ; This requires the 2MB page for this range be broken down into >>> 512 4KB >>> +    ; pages.  All will be marked encrypted, except for the GHCB. >>> +    ; >>> +    mov     ecx, (GHCB_BASE >> 21) >>> +    mov     eax, GHCB_PT_ADDR + PAGE_PDP_ATTR >>> +    mov     [ecx * 8 + PT_ADDR (0x2000)], eax >>> + >>> +    ; >>> +    ; Page Table Entries (512 * 4KB entries => 2MB) >>> +    ; >>> +    mov     ecx, 512 >>> +pageTableEntries4kLoop: >>> +    mov     eax, ecx >>> +    dec     eax >>> +    shl     eax, 12 >>> +    add     eax, GHCB_BASE & 0xFFE0_0000 >>> +    add     eax, PAGE_4K_PDE_ATTR >>> +    mov     [ecx * 8 + GHCB_PT_ADDR - 8], eax >>> +    mov     [(ecx * 8 + GHCB_PT_ADDR - 8) + 4], edx >>> +    loop    pageTableEntries4kLoop >>> + >>> +    ; >>> +    ; Clear the encryption bit from the GHCB entry >>> +    ; >>> +    mov     ecx, (GHCB_BASE & 0x1F_FFFF) >> 12 >>> +    mov     [ecx * 8 + GHCB_PT_ADDR + 4], strict dword 0 >>> + >>> +    mov     ecx, GHCB_SIZE / 4 >>> +    xor     eax, eax >>> +clearGhcbMemoryLoop: >>> +    mov     dword[ecx * 4 + GHCB_BASE - 4], eax >>> +    loop    clearGhcbMemoryLoop >>> + >> >> This patch is now identical to v6, modulo some (welcome) commit message >> updates, and the PCD token value updates. Therefore, we can re-add my: >> >> Reviewed-by: Laszlo Ersek >> >> from v6. >> >> However, in the v8 discussion of the patch, you indicated that the >> clearing loop had been in the wrong spot (in v6) -- it should have been >> placed after the CR3 setting, apparently: >> >> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2F1177217e-74d9-ddb5-fd38-c5ffb02de3f3%40amd.com&data=02%7C01%7Cthomas.lendacky%40amd.com%7C38f855613c974b9f23e108d80dedc415%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637274662138263584&sdata=lV7Ou2%2F047pUTLpbutO3sXwCoxDWQrKIPDXhRh9aOqM%3D&reserved=0 >> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F60284&data=02%7C01%7Cthomas.lendacky%40amd.com%7C38f855613c974b9f23e108d80dedc415%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637274662138263584&sdata=GwGXhDbxjafSgjYLP8kDI7fvkijNKgZBbgqgVuVVHOM%3D&reserved=0 >> >> >> But in this update (v9), the clearing loop has not been moved, relative >> to v6; it's been re-instated in the same spot. (IIUC.) >> >> So what is the right spot for the clearing loop after all? > > After looking at the code more closely, even though the CR3 register is > loaded, paging hasn't been enabled (it's enabled on return from this > function/area). So the location of the clearing loop didn't matter in > the end and so I left it in the original location. The net effect is > that per-CPU page for the DR7 value still ends up zeroed out. Awesome, thanks! Laszlo