Re: [Xen-devel] [edk2] [PATCH RFC 03/14] OvmfPkg/XenOvmf.dsc: Introduce XenResetVector

[Anthony PERARD]

On Wed, Jan 04, 2017 at 08:49:15PM +0100, Laszlo Ersek wrote:
> > diff --git a/OvmfPkg/XenResetVector/Ia32/PageTables64.asm 
> > b/OvmfPkg/XenResetVector/Ia32/PageTables64.asm
> > new file mode 100644
> > index 000..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.
> > +;
> > +;--
> > +
> > +BITS32
> > +
> > +%define PAGE_PRESENT0x01
> > +%define PAGE_READ_WRITE 0x02
> > +%define PAGE_USER_SUPERVISOR0x04
> > +%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_MBO0x080
> > +%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 0x80-0x806000.
> 
> (6) Are you intentionally undoing, in the copy, commit 73d66c5871cc? If
> so, why?

No, I just need to use --find-copies-harder to find out about recent
changes.

> 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.

I don't think I've customize this file, this could be a link, or I could
try to have the build system use the original file from
OvmfPkg/ResetVector.

Thanks,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [edk2] [PATCH RFC 03/14] OvmfPkg/XenOvmf.dsc: Introduce XenResetVector

[Andrew Cooper]

On 04/01/17 19:49, Laszlo Ersek wrote:
> (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?

For reasons best known to the original authors of the code, Xen respects
CR0.CD unconditionally for AMD, but respects CR0.CD on Intel only when
the guest has a piece of real hardware passed through, leaving caches
enabled in the general case.

This manifests as a massive performance difference between Intel and AMD.

IMO this behaviour should be fixed in Xen for AMD hardware (to match
Intel), but equally, firmware in a virtual environment has no real
business setting CR0.CD in the first place.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [edk2] [PATCH RFC 03/14] OvmfPkg/XenOvmf.dsc: Introduce XenResetVector

[Laszlo Ersek]

(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 000..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  0x0023
> +%define SEC_DEFAULT_CR4  0x640
> +
> +BITS16
> +
> +;
> +; 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)
> +BITS32
> +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)