Re: [PATCH v3 06/35] OvmfPkg/XenResetVector: Add new entry point for Xen PVH

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

On 04/07/2019 15:42, Anthony PERARD wrote:
> Add a new entry point for Xen PVH that enter directly in 32bits.
>
> Information on the expected state of the machine when this entry point
> is used can be found at:
> https://xenbits.xenproject.org/docs/unstable/misc/pvh.html
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>
> Notes:
>     v3:
>     - rebased, SPDX
>     - remove `cli' as via PVH the interrupts are guaranteed to be off
>     - rewrite some comments

Thanks - this is easier to follow.  Some further questions.

> diff --git a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> new file mode 100644
> index 0000000000..958195bc5e
> --- /dev/null
> +++ b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> +vtfSignature:
> +    DB      'V', 'T', 'F', 0
> +
> +ALIGN   16
> +
> +resetVector:
> +;
> +; Reset Vector
> +;
> +; This is where the processor will begin execution
> +;
> +    nop
> +    nop

Why two nops?

> +    jmp     EarlyBspInitReal16
> +
> +ALIGN   16
> +
> +fourGigabytes:
> +
> diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> new file mode 100644
> index 0000000000..2a17fed52f
> --- /dev/null
> +++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> @@ -0,0 +1,49 @@
> +;------------------------------------------------------------------------------
> +; @file
> +; An entry point use by Xen when a guest is started in PVH mode.
> +;
> +; Copyright (c) 2019, Citrix Systems, Inc.
> +;
> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> +;
> +;------------------------------------------------------------------------------
> +
> +BITS    32
> +
> +xenPVHMain:
> +    ;
> +    ; 'BP' to indicate boot-strap processor

Indicate to what?

> +    ;
> +    mov     di, 'BP'
> +
> +    ;
> +    ; ESP will be used as initial value of the EAX register
> +    ; in Main.asm
> +    ;
> +    xor     esp, esp
> +
> +    mov     ebx, ADDR_OF(gdtr)
> +    lgdt    [ebx]

lgdt [ADDR_OF(gdtr)]

should work fine, because you're in 32bit mode.

More importantly for PVH however, you don't clobber the start_info pointer.

> +
> +    mov     eax, SEC_DEFAULT_CR0
> +    mov     cr0, eax
> +
> +    jmp     LINEAR_CODE_SEL:ADDR_OF(.jmpToNewCodeSeg)
> +.jmpToNewCodeSeg:

Does 1f (or some equivalent) not work, or is this against the coding style?

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

Use eax rather than ax.  The instruction decode will be much happier
with the result, and it results in shorter assembled code.

> +
> +    ;
> +    ; Jump to the main routine of the pre-SEC code
> +    ; skiping the 16-bit part of the routine and
> +    ; into the 32-bit flat mode part
> +    ;
> +    OneTimeCallRet TransitionFromReal16To32BitFlat

Thanks.  This is far easier to follow.

~Andrew