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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jul 04, 2019 at 03:42:04PM +0100, 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>

Thanks for doing this! My knowledge of nasm is very limited, so some
of the above comments might be completely wrong.

> ---
> 
> Notes:
>     v3:
>     - rebased, SPDX
>     - remove `cli' as via PVH the interrupts are guaranteed to be off
>     - rewrite some comments
> 
>  .../XenResetVector/Ia16/ResetVectorVtf0.asm   | 81 +++++++++++++++++++
>  OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm    | 49 +++++++++++
>  OvmfPkg/XenResetVector/XenResetVector.nasmb   |  1 +
>  3 files changed, 131 insertions(+)
>  create mode 100644 OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
>  create mode 100644 OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> 
> 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
> @@ -0,0 +1,81 @@
> +;------------------------------------------------------------------------------
> +; @file
> +; First code executed by processor after resetting.
> +;
> +; Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.<BR>

Extraneous <BR> tag?

> +; Copyright (c) 2019, Citrix Systems, Inc.
> +;
> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> +;
> +;------------------------------------------------------------------------------
> +
> +BITS    16
> +
> +ALIGN   16

Do you need the BITS and ALIGN here?

Isn't it enough with the BITS 32 below for the entry point, since DB
is already explicitly sized?

> +
> +;
> +; Pad the image size to 4k when page tables are in VTF0
> +;
> +; If the VTF0 image has page tables built in, then we need to make
> +; sure the end of VTF0 is 4k above where the page tables end.
> +;
> +; This is required so the page tables will be 4k aligned when VTF0 is
> +; located just below 0x100000000 (4GB) in the firmware device.
> +;
> +%ifdef ALIGN_TOP_TO_4K_FOR_PAGING
> +    TIMES (0x1000 - ($ - EndOfPageTables) - (fourGigabytes - xenPVHEntryPoint)) DB 0

What's the meaning of 0x1000 here?

> +%endif
> +
> +BITS    32
> +xenPVHEntryPoint:
> +;
> +; Entry point to use when running as a Xen PVH guest. (0xffffffd0)

Shouldn't this positioning be set on the linker script instead?

> +;
> +; Description of 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
> +;
> +    jmp     xenPVHMain
> +
> +BITS    16
> +ALIGN   16

Is it really needed to specify both?

I would assume that setting BITS 16 will already set a suitable
alignment.

> +
> +applicationProcessorEntryPoint:
> +;
> +; Application Processors entry point
> +;
> +; GenFv generates code aligned on a 4k boundary which will jump to this
> +; location.  (0xffffffe0)  This allows the Local APIC Startup IPI to be

Also, if xenPVHEntryPoint is at 0x...d0, how can
applicationProcessorEntryPoint be at 0x...e0, I guess there's some
other code I'm missing that either adds padding between both, or
places them in different sections on the resulting binary image?

> +; used to wake up the application processors.
> +;
> +    jmp     EarlyApInitReal16
> +
> +ALIGN   8
> +
> +    DD      0

Can you remove this DD...

> +
> +;
> +; The VTF signature
> +;
> +; VTF-0 means that the VTF (Volume Top File) code does not require
> +; any fixups.
> +;
> +vtfSignature:
> +    DB      'V', 'T', 'F', 0

And instead do DB 0, 0, 0, 0, 'V',...?

In any case I'm not sure of the point of setting align to 8 and then
writing 32bits of 0s (but again maybe I'm just misreading the code).

Maybe you just want to set align to 32 and write the vtf signature?

> +
> +ALIGN   16
> +
> +resetVector:
> +;
> +; Reset Vector
> +;
> +; This is where the processor will begin execution
> +;
> +    nop
> +    nop
> +    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
> +    ;
> +    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]
> +
> +    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
> +
> +    ;
> +    ; 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

Since PVH already starts in flat 32bit mode, I'm not sure I see the
point of this routine, since it seems to be used exclusively to switch
from 16 to 32b flat mode. The comment mentions skipping that part, but
I'm not sure I see how that's achieved.

Thanks, Roger.