From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-x243.google.com (mail-qk0-x243.google.com [IPv6:2607:f8b0:400d:c09::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 61157803DF for ; Thu, 23 Mar 2017 08:05:07 -0700 (PDT) Received: by mail-qk0-x243.google.com with SMTP id p22so3173637qka.0 for ; Thu, 23 Mar 2017 08:05:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=B5yvlcciWdPXwk3si2AfJWInSWqfI0Udz1jf7hpihHI=; b=qRo+bIAQjGGr+xvUnsBhM4YG8HZtqF/OiJYgrG0zwGXCkzyBl+YkRpkh8SsEqwKbhU 6LKYdnwr1y67kwBmOiQJ37zAUJOdV2Fr+Y4uGdkz0Q8f/9Vivydf+GbNTpSfirfsM52A m+FXBHU8qCKaZq3zg9OYswflWeI88Bvd4BgL7gRkoioTuIDHuDbLw5T8JP9SdSTcrsUR qoMcxEm9QdNQM5V62lbbF3etT5ChZn6JCd4K+J3Vif0wlYCCigGw4BIUDID80RC4kJkT YfDk0uVvPlos1zLUnB2CMNQF2lpNVfsJZW6AhK1mpswR2lCgILBcl7GtXnKLG6+V3m1G RNVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=B5yvlcciWdPXwk3si2AfJWInSWqfI0Udz1jf7hpihHI=; b=HD0yjrAaKRGAzf/j3A2MA23PXM1jeT9I1mtY29JtJ3m40vsDwQJi9xBLiX4k7p9Iyx 783yLsDYuyWV4IlTHeBSXyk42O4kawFA9XRHevRYs5EfMcX+agxcbDjnW+sZ4Ka4Tjod X4gkLljo9wGFMTetb7HxNiloffsXDBSWs5bNzTuXRu9B9CupvCA+X8l9bvD56+8/2OGp VuvQtjFTIh1SgafnLA2IMg2g4eW7kImz8KswtOZd1Wno6BA6sByK8Kmsm1GxPCTqBhWs eXTNQuFh9T8XAPifOBpt795P1WR5oJvCP63fSx8SG041dJvmwjqojQs4iXen7Pxa5Q3/ TJRA== X-Gm-Message-State: AFeK/H1p6pnO4KhxTQvHq7SdP0/BDJkmEtLUQEbzQ3gybpmuNvoeTqmyajMk6wIVV3WXosWXRSU++6/KF2Vmjg== X-Received: by 10.55.16.168 with SMTP id 40mr2411953qkq.289.1490281506239; Thu, 23 Mar 2017 08:05:06 -0700 (PDT) MIME-Version: 1.0 Received: by 10.12.182.65 with HTTP; Thu, 23 Mar 2017 08:05:05 -0700 (PDT) In-Reply-To: <15681e6f-dd58-957a-067f-f1036b31c62d@redhat.com> References: <149013076154.27235.10725020825643505862.stgit@brijesh-build-machine> <149013077498.27235.15379321048646409782.stgit@brijesh-build-machine> <15681e6f-dd58-957a-067f-f1036b31c62d@redhat.com> From: Brijesh Singh Date: Thu, 23 Mar 2017 10:05:05 -0500 Message-ID: To: Laszlo Ersek Cc: "Kinney, Michael D" , "Justen, Jordan L" , edk2-devel@ml01.01.org, "Gao, Liming" , brijesh.singh@amd.com, Leo Duran , Tom Lendacky X-Content-Filtered-By: Mailman/MimeDel 2.1.22 Subject: Re: [RFC PATCH v2 02/10] OvmfPkg/ResetVector: add memory encryption mask when SEV is enabled X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Mar 2017 15:05:07 -0000 Content-Type: text/plain; charset=UTF-8 On Wed, Mar 22, 2017 at 3:20 PM, Laszlo Ersek wrote: > On 03/21/17 22:12, Brijesh Singh wrote: > > SEV guest VMs have the concept of private and shared memory. Private > > memory is encrypted with the guest-specific key, while shared memory > > may be encrypted with hypervisor key. Certain types of memory (namely > > instruction pages and guest page tables) are always treated as private > > memory by the hardware. The C-bit in PTE indicate whether the page is > > private or shared. The C-bit position for the PTE can be obtained from > > CPUID Fn8000_001F[EBX]. > > > > When SEV is active, the BIOS is pre-encrypted by the Qemu launch > sequence, > > we must set the C-bit when building the page table for 64-bit or 32-bit > > PAE mode. > > > > Signed-off-by: Brijesh Singh > > --- > > OvmfPkg/ResetVector/Ia32/PageTables64.asm | 62 > +++++++++++++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > > > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > > index 6201cad..7083f6b 100644 > > --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > > +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > > @@ -37,6 +37,47 @@ BITS 32 > > PAGE_READ_WRITE + \ > > PAGE_PRESENT) > > > > +; Check if Secure Encrypted Virtualization (SEV) feature > > +; > > +; If SEV is enabled, then EAX will contain Memory encryption bit > position > > I suggest to say, in the comment here, that a valid eax will be at least > 32, and 0 on "SEV unavailable or disabled". > > WIll do. > > +; > > +CheckSevFeature: > > + xor eax, eax > > I think this xor may not be necessary (there's no path out of this > function that wouldn't set eax intentionally), but it doesn't hurt. > > (Sorry if I should have noticed this in v1 -- there's no need to > resubmit just because of this.) > > In the below logic, which branch exactly (to NoSev) will be taken on > Intel processors? > > The below check should branch to NoSev on Intel processor (please note that 0x8000_001F is new leaf and may not exist on older AMD processor hence we will also branch to NoSev on AMD processor which does not support this leaf) *; Check if we have a valid (0x8000_001F) CPUID leaf **mov eax, 0x80000000 **cpuid **cmp eax, 0x8000001f **jl NoSev* > + > + ; Check if we have a valid (0x8000_001F) CPUID leaf > > + mov eax, 0x80000000 > > + cpuid > > + cmp eax, 0x8000001f > > + jl NoSev > > + > > + ; Check for memory encryption feature: > > + ; CPUID Fn8000_001F[EAX] - Bit 1 > > + ; > > + mov eax, 0x8000001f > > + cpuid > > + bt eax, 1 > > + jnc NoSev > > + > > + ; Check if memory encryption is enabled > > + ; MSR_0xC0010131 - Bit 0 (SEV enabled) > > + ; MSR_0xC0010131 - Bit 1 (SEV-ES enabled) > > + mov ecx, 0xc001013. > > + rdmsr > > + bt eax, 0 > > + jnc NoSev > > Do we need to check SEV-ES specifically? bit1 is not tested. > > We don't need to check SEV-ES bit, I just added it here for documentation purposes. In current patch series I am not adding any of the ES funtionality and will be revisit later. > + > > + ; Get pte bit position to enable memory encryption > > + ; CPUID Fn8000_001F[EBX] - Bits 5:0 > > + ; > > + mov eax, ebx > > + and eax, 0x3f > > + jmp SevExit > > + > > +NoSev: > > + xor eax, eax > > + > > +SevExit: > > + OneTimeCallRet CheckSevFeature > > > > ; > > ; Modified: EAX, ECX > > Can you please update (or delete) this comment? I think EBX and EDX may > be modified too. > > Will do. Since cpuid clobbers these registers I can push/pop these inside the CheckSevFeature. > > @@ -60,18 +101,38 @@ clearPageTablesMemoryLoop: > > mov dword[ecx * 4 + PT_ADDR (0) - 4], eax > > loop clearPageTablesMemoryLoop > > > > + ; Check if its SEV-enabled Guest > > + ; > > + OneTimeCall CheckSevFeature > > + xor edx, edx > > + test eax, eax > > + jz SevNotActive > > + > > + ; If SEV is enabled, Memory encryption bit is always above 31 > > + mov ebx, 32 > > + sub ebx, eax > > I apologize if I'm mistaken, but I think we want to decrease eax with 32 > (i.e., with ebx), so that we get the C bit's position within the high > address (= more significant) DWORD of the PTE. > > But, isn't the argument order for SUB reversed? To me the above seems to > imply EBX := EBX - EAX, which appears wrong. > > ... I think we could even say > > sub eax, 32 > > Ah good catch. You are right that I should be using "sub eax, ebx" or "sub eax, 32". I am scratching my head to figure out why SEV guest was booting with this bug. If the mask calculation is wrong then we should be failing to boot the SEV guest. But looking at "bts instruction" [1] indicates that some assembler support bit offset larger than 31. Since eax still contains the original bit position hence the mask creation logic was still working. I will fix it. [1] http://x86.renejeschke.de/html/file_module_x86_id_25.html Thank you. -Brijesh > The rest looks good to me. > > Thanks > Laszlo > > > + bts edx, eax > > + > > +SevNotActive: > > + > > + ; > > ; > > ; Top level Page Directory Pointers (1 * 512GB entry) > > ; > > mov dword[PT_ADDR (0)], PT_ADDR (0x1000) + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (4)], edx > > > > ; > > ; Next level Page Directory Pointers (4 * 1GB entries => 4GB) > > ; > > mov dword[PT_ADDR (0x1000)], PT_ADDR (0x2000) + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0x1004)], edx > > mov dword[PT_ADDR (0x1008)], PT_ADDR (0x3000) + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0x100C)], edx > > mov dword[PT_ADDR (0x1010)], PT_ADDR (0x4000) + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0x1014)], edx > > mov dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + PAGE_PDP_ATTR > > + mov dword[PT_ADDR (0x101C)], edx > > > > ; > > ; Page Table Entries (2048 * 2MB entries => 4GB) > > @@ -83,6 +144,7 @@ pageTableEntriesLoop: > > shl eax, 21 > > add eax, PAGE_2M_PDE_ATTR > > mov [ecx * 8 + PT_ADDR (0x2000 - 8)], eax > > + mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx > > loop pageTableEntriesLoop > > > > ; > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > -- Confusion is always the most honest response.