Re: [Xen-devel] [PATCH v2 06/31] OvmfPkg/XenResetVector: Add new entry point for Xen PVH

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/04/2019 12:08, Anthony PERARD wrote:
> diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> new file mode 100644
> index 0000000000..c4802bf4d1
> --- /dev/null
> +++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> @@ -0,0 +1,47 @@
> +;------------------------------------------------------------------------------
> +; @file
> +; An entry point use by Xen when a guest is started in PVH mode.
> +;
> +; Copyright (c) 2019, Citrix Systems, Inc.
> +;
> +; 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
> +
> +xenPVHMain:
> +    mov     di, 'BP'
> +
> +    ; ESP -  Initial value of the EAX register (BIST: Built-in Self Test)
> +    mov     esp, eax

Where is the ABI described?  Xen has no BIST, so this will always have
the value 0. Stashing it in the stack pointer seems like a weird choice,
and a recipe for subtle bugs.

> +
> +    cli

Interrupts are guaranteed to be off at this point.

> +
> +    mov     ebx, ADDR_OF(gdtr)
> +    lgdt    [ebx]

lgdt ADDR_OF(gdtr), presumably?

This is 32bit code - there is no need for any indirection through
registers for memory operands.

> +
> +    mov     eax, SEC_DEFAULT_CR0
> +    mov     cr0, eax
> +
> +    jmp     LINEAR_CODE_SEL:ADDR_OF(.jmpToNewCodeSeg)
> +.jmpToNewCodeSeg:
> +
> +    mov     eax, SEC_DEFAULT_CR4
> +    mov     cr4, eax
> +
> +    mov     ax, LINEAR_SEL
> +    mov     ds, ax
> +    mov     es, ax
> +    mov     fs, ax
> +    mov     gs, ax
> +    mov     ss, ax
> +
> +    ; return to the Main16
> +    OneTimeCallRet TransitionFromReal16To32BitFlat

Is there any description of what OneTimeCallRet is, and why a simple jmp
wont do?

Irrespective of that, you're moving to a function whose name suggests it
is in 16bit mode, while you are currently in 32bit flat mode. 
(SEC_DEFAULT_CR0 has PE set, and LINEAR_SEL is 32bit flat.  This clearly
isn't correct, but surely we want to skip all the 16bit setup, as well.

~Andrew