Re: [PATCH RFC 03/14] OvmfPkg/XenOvmf.dsc: Introduce XenResetVector

[Laszlo Ersek <lersek@redhat.com>]

(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 <anthony.perard@citrix.com>
> ---
>  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.<BR>
> +; 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.<BR>
> +; 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.<BR>
> +#
> +#  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.<BR>
> +; 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 <Base.h>
> +    #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"
> +
>