From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C096881944 for ; Wed, 4 Jan 2017 11:49:18 -0800 (PST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3E66A7F7CE; Wed, 4 Jan 2017 19:49:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-72.phx2.redhat.com [10.3.116.72]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v04JnHii006414; Wed, 4 Jan 2017 14:49:17 -0500 To: Anthony PERARD , edk2-devel@ml01.01.org, xen-devel@lists.xenproject.org References: <20161208153340.2285-1-anthony.perard@citrix.com> <20161208153340.2285-4-anthony.perard@citrix.com> From: Laszlo Ersek Message-ID: <0ec7949b-924f-edec-5d0c-fad0976cf572@redhat.com> Date: Wed, 4 Jan 2017 20:49:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20161208153340.2285-4-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 04 Jan 2017 19:49:19 +0000 (UTC) Subject: Re: [PATCH RFC 03/14] OvmfPkg/XenOvmf.dsc: Introduce XenResetVector X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Jan 2017 19:49:18 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit (1) I think the subject line should just say: OvmfPkg: Introduce XenResetVector (2) New files are added in this patch; you might want to tag them with a Citrix copyright notice. (3) When formatting the next version of this series for posting, please pass the "-C --find-copies-harder" options to git-format-patch. Those will automatically format each patch in the series as a (copy, diff) pair, whenever appropriate, and that way we can compare the changed copies more easily against the originals. On 12/08/16 16:33, Anthony PERARD wrote: > Copy of OvmfPkg/ResetVector, with one changes: > - default_cr0: enable cache (4) Please mention SEC_DEFAULT_CR0 and the bit that is flipped, specifically. > > Xen copy the OVMF code to RAM, there is no need for cache. Also, it > makes OVMF slow to start on AMD. (5) Wait, is the slow start already an issue (with QEMU/KVM) on AMD? Because, in the past, we saw that happen: refer to commit 98f378a7be12. Are you saying 98f378a7be12 was not entirely right for QEMU/KVM (considering recent AMD processors, I guess?)? Or that the SEC_DEFAULT_CR0 value is not right on AMD only with Xen? > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Anthony PERARD > --- > OvmfPkg/XenOvmf.dsc | 2 +- > OvmfPkg/XenOvmf.fdf | 2 +- > OvmfPkg/XenResetVector/Ia16/Real16ToFlat32.asm | 133 +++++++++++++++++++++++++ > OvmfPkg/XenResetVector/Ia32/PageTables64.asm | 93 +++++++++++++++++ > OvmfPkg/XenResetVector/XenResetVector.inf | 37 +++++++ > OvmfPkg/XenResetVector/XenResetVector.nasmb | 66 ++++++++++++ > 6 files changed, 331 insertions(+), 2 deletions(-) > create mode 100644 OvmfPkg/XenResetVector/Ia16/Real16ToFlat32.asm > create mode 100644 OvmfPkg/XenResetVector/Ia32/PageTables64.asm > create mode 100644 OvmfPkg/XenResetVector/XenResetVector.inf > create mode 100644 OvmfPkg/XenResetVector/XenResetVector.nasmb > > diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc > index 5b3550d..0a7ea21 100644 > --- a/OvmfPkg/XenOvmf.dsc > +++ b/OvmfPkg/XenOvmf.dsc > @@ -471,7 +471,7 @@ > # > ################################################################################ > [Components] > - OvmfPkg/ResetVector/ResetVector.inf > + OvmfPkg/XenResetVector/XenResetVector.inf > > # > # SEC Phase modules > diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf > index d01454e..f4609b0 100644 > --- a/OvmfPkg/XenOvmf.fdf > +++ b/OvmfPkg/XenOvmf.fdf > @@ -123,7 +123,7 @@ READ_LOCK_STATUS = TRUE > # > INF OvmfPkg/Sec/SecMain.inf > > -INF RuleOverride=RESET_VECTOR OvmfPkg/ResetVector/ResetVector.inf > +INF RuleOverride=RESET_VECTOR OvmfPkg/XenResetVector/XenResetVector.inf > > ################################################################################ > [FV.PEIFV] > diff --git a/OvmfPkg/XenResetVector/Ia16/Real16ToFlat32.asm b/OvmfPkg/XenResetVector/Ia16/Real16ToFlat32.asm > new file mode 100644 > index 0000000..d746427 > --- /dev/null > +++ b/OvmfPkg/XenResetVector/Ia16/Real16ToFlat32.asm > @@ -0,0 +1,133 @@ > +;------------------------------------------------------------------------------ > +; @file > +; Transition from 16 bit real mode into 32 bit flat protected mode > +; > +; Copyright (c) 2008 - 2010, Intel Corporation. All rights reserved.
> +; This program and the accompanying materials > +; are licensed and made available under the terms and conditions of the BSD License > +; which accompanies this distribution. The full text of the license may be found at > +; http://opensource.org/licenses/bsd-license.php > +; > +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +; > +;------------------------------------------------------------------------------ > + > +%define SEC_DEFAULT_CR0 0x00000023 > +%define SEC_DEFAULT_CR4 0x640 > + > +BITS 16 > + > +; > +; Modified: EAX, EBX > +; > +TransitionFromReal16To32BitFlat: > + > + debugShowPostCode POSTCODE_16BIT_MODE > + > + cli > + > + mov bx, 0xf000 > + mov ds, bx > + > + mov bx, ADDR16_OF(gdtr) > + > +o32 lgdt [cs:bx] > + > + mov eax, SEC_DEFAULT_CR0 > + mov cr0, eax > + > + jmp LINEAR_CODE_SEL:dword ADDR_OF(jumpTo32BitAndLandHere) > +BITS 32 > +jumpTo32BitAndLandHere: > + > + mov eax, SEC_DEFAULT_CR4 > + mov cr4, eax > + > + debugShowPostCode POSTCODE_32BIT_MODE > + > + mov ax, LINEAR_SEL > + mov ds, ax > + mov es, ax > + mov fs, ax > + mov gs, ax > + mov ss, ax > + > + OneTimeCallRet TransitionFromReal16To32BitFlat > + > +ALIGN 2 > + > +gdtr: > + dw GDT_END - GDT_BASE - 1 ; GDT limit > + dd ADDR_OF(GDT_BASE) > + > +ALIGN 16 > + > +; > +; Macros for GDT entries > +; > + > +%define PRESENT_FLAG(p) (p << 7) > +%define DPL(dpl) (dpl << 5) > +%define SYSTEM_FLAG(s) (s << 4) > +%define DESC_TYPE(t) (t) > + > +; Type: data, expand-up, writable, accessed > +%define DATA32_TYPE 3 > + > +; Type: execute, readable, expand-up, accessed > +%define CODE32_TYPE 0xb > + > +; Type: execute, readable, expand-up, accessed > +%define CODE64_TYPE 0xb > + > +%define GRANULARITY_FLAG(g) (g << 7) > +%define DEFAULT_SIZE32(d) (d << 6) > +%define CODE64_FLAG(l) (l << 5) > +%define UPPER_LIMIT(l) (l) > + > +; > +; The Global Descriptor Table (GDT) > +; > + > +GDT_BASE: > +; null descriptor > +NULL_SEL equ $-GDT_BASE > + DW 0 ; limit 15:0 > + DW 0 ; base 15:0 > + DB 0 ; base 23:16 > + DB 0 ; sys flag, dpl, type > + DB 0 ; limit 19:16, flags > + DB 0 ; base 31:24 > + > +; linear data segment descriptor > +LINEAR_SEL equ $-GDT_BASE > + DW 0xffff ; limit 15:0 > + DW 0 ; base 15:0 > + DB 0 ; base 23:16 > + DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(DATA32_TYPE) > + DB GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf) > + DB 0 ; base 31:24 > + > +; linear code segment descriptor > +LINEAR_CODE_SEL equ $-GDT_BASE > + DW 0xffff ; limit 15:0 > + DW 0 ; base 15:0 > + DB 0 ; base 23:16 > + DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE32_TYPE) > + DB GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf) > + DB 0 ; base 31:24 > + > +%ifdef ARCH_X64 > +; linear code (64-bit) segment descriptor > +LINEAR_CODE64_SEL equ $-GDT_BASE > + DW 0xffff ; limit 15:0 > + DW 0 ; base 15:0 > + DB 0 ; base 23:16 > + DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE64_TYPE) > + DB GRANULARITY_FLAG(1)|DEFAULT_SIZE32(0)|CODE64_FLAG(1)|UPPER_LIMIT(0xf) > + DB 0 ; base 31:24 > +%endif > + > +GDT_END: > + > diff --git a/OvmfPkg/XenResetVector/Ia32/PageTables64.asm b/OvmfPkg/XenResetVector/Ia32/PageTables64.asm > new file mode 100644 > index 0000000..b5a4cf8 > --- /dev/null > +++ b/OvmfPkg/XenResetVector/Ia32/PageTables64.asm > @@ -0,0 +1,93 @@ > +;------------------------------------------------------------------------------ > +; @file > +; Sets the CR3 register for 64-bit paging > +; > +; Copyright (c) 2008 - 2013, Intel Corporation. All rights reserved.
> +; This program and the accompanying materials > +; are licensed and made available under the terms and conditions of the BSD License > +; which accompanies this distribution. The full text of the license may be found at > +; http://opensource.org/licenses/bsd-license.php > +; > +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +; > +;------------------------------------------------------------------------------ > + > +BITS 32 > + > +%define PAGE_PRESENT 0x01 > +%define PAGE_READ_WRITE 0x02 > +%define PAGE_USER_SUPERVISOR 0x04 > +%define PAGE_WRITE_THROUGH 0x08 > +%define PAGE_CACHE_DISABLE 0x010 > +%define PAGE_ACCESSED 0x020 > +%define PAGE_DIRTY 0x040 > +%define PAGE_PAT 0x080 > +%define PAGE_GLOBAL 0x0100 > +%define PAGE_2M_MBO 0x080 > +%define PAGE_2M_PAT 0x01000 > + > +%define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \ > + PAGE_ACCESSED + \ > + PAGE_DIRTY + \ > + PAGE_READ_WRITE + \ > + PAGE_PRESENT) > + > +%define PAGE_PDP_ATTR (PAGE_ACCESSED + \ > + PAGE_READ_WRITE + \ > + PAGE_PRESENT) > + > + > +; > +; Modified: EAX, ECX > +; > +SetCr3ForPageTables64: > + > + ; > + ; For OVMF, build some initial page tables at 0x800000-0x806000. (6) Are you intentionally undoing, in the copy, commit 73d66c5871cc? If so, why? For now I suspect that you originally created this patch before commit 73d66c5871cc came into existence. If that's the case, then please rebase (re-copy and customize) this patch to the most recent ResetVector state, including commit 73d66c5871cc. > + ; > + ; This range should match with PcdOvmfSecPageTablesBase and > + ; PcdOvmfSecPageTablesSize which are declared in the FDF files. > + ; > + ; At the end of PEI, the pages tables will be rebuilt into a > + ; more permanent location by DxeIpl. > + ; > + > + mov ecx, 6 * 0x1000 / 4 > + xor eax, eax > +clearPageTablesMemoryLoop: > + mov dword[ecx * 4 + 0x800000 - 4], eax > + loop clearPageTablesMemoryLoop > + > + ; > + ; Top level Page Directory Pointers (1 * 512GB entry) > + ; > + mov dword[0x800000], 0x801000 + PAGE_PDP_ATTR > + > + ; > + ; Next level Page Directory Pointers (4 * 1GB entries => 4GB) > + ; > + mov dword[0x801000], 0x802000 + PAGE_PDP_ATTR > + mov dword[0x801008], 0x803000 + PAGE_PDP_ATTR > + mov dword[0x801010], 0x804000 + PAGE_PDP_ATTR > + mov dword[0x801018], 0x805000 + PAGE_PDP_ATTR > + > + ; > + ; Page Table Entries (2048 * 2MB entries => 4GB) > + ; > + mov ecx, 0x800 > +pageTableEntriesLoop: > + mov eax, ecx > + dec eax > + shl eax, 21 > + add eax, PAGE_2M_PDE_ATTR > + mov [ecx * 8 + 0x802000 - 8], eax > + loop pageTableEntriesLoop > + > + ; > + ; Set CR3 now that the paging structures are available > + ; > + mov eax, 0x800000 > + mov cr3, eax > + > + OneTimeCallRet SetCr3ForPageTables64 > diff --git a/OvmfPkg/XenResetVector/XenResetVector.inf b/OvmfPkg/XenResetVector/XenResetVector.inf > new file mode 100644 > index 0000000..ebfab12 > --- /dev/null > +++ b/OvmfPkg/XenResetVector/XenResetVector.inf > @@ -0,0 +1,37 @@ > +## @file > +# Reset Vector > +# > +# Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
> +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = XenResetVector > + FILE_GUID = 1BA0062E-C779-4582-8566-336AE8F78F09 This FILE_GUID was not changed, but for the reset vector, that's actually the right thing to do -- this FILE_GUID is special. So it's OK. Thanks Laszlo > + MODULE_TYPE = SEC > + VERSION_STRING = 1.1 > + > +# > +# The following information is for reference only and not required by the build tools. > +# > +# VALID_ARCHITECTURES = IA32 X64 > +# > + > +[Sources] > + XenResetVector.nasmb > + > +[Packages] > + MdePkg/MdePkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > + > +[BuildOptions] > + *_*_IA32_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ > + *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/ > diff --git a/OvmfPkg/XenResetVector/XenResetVector.nasmb b/OvmfPkg/XenResetVector/XenResetVector.nasmb > new file mode 100644 > index 0000000..31ac06a > --- /dev/null > +++ b/OvmfPkg/XenResetVector/XenResetVector.nasmb > @@ -0,0 +1,66 @@ > +;------------------------------------------------------------------------------ > +; @file > +; This file includes all other code files to assemble the reset vector code > +; > +; Copyright (c) 2008 - 2013, Intel Corporation. All rights reserved.
> +; This program and the accompanying materials > +; are licensed and made available under the terms and conditions of the BSD License > +; which accompanies this distribution. The full text of the license may be found at > +; http://opensource.org/licenses/bsd-license.php > +; > +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +; > +;------------------------------------------------------------------------------ > + > +; > +; If neither ARCH_IA32 nor ARCH_X64 are defined, then try to include > +; Base.h to use the C pre-processor to determine the architecture. > +; > +%ifndef ARCH_IA32 > + %ifndef ARCH_X64 > + #include > + #if defined (MDE_CPU_IA32) > + %define ARCH_IA32 > + #elif defined (MDE_CPU_X64) > + %define ARCH_X64 > + #endif > + %endif > +%endif > + > +%ifdef ARCH_IA32 > + %ifdef ARCH_X64 > + %error "Only one of ARCH_IA32 or ARCH_X64 can be defined." > + %endif > +%elifdef ARCH_X64 > +%else > + %error "Either ARCH_IA32 or ARCH_X64 must be defined." > +%endif > + > +%include "CommonMacros.inc" > + > +%include "PostCodes.inc" > + > +%ifdef DEBUG_PORT80 > + %include "Port80Debug.asm" > +%elifdef DEBUG_SERIAL > + %include "SerialDebug.asm" > +%else > + %include "DebugDisabled.asm" > +%endif > + > +%include "Ia32/SearchForBfvBase.asm" > +%include "Ia32/SearchForSecEntry.asm" > + > +%ifdef ARCH_X64 > +%include "Ia32/Flat32ToFlat64.asm" > +%include "Ia32/PageTables64.asm" > +%endif > + > +%include "Ia16/Real16ToFlat32.asm" > +%include "Ia16/Init16.asm" > + > +%include "Main.asm" > + > +%include "Ia16/ResetVectorVtf0.asm" > + >