diff mbox series

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

Message ID 20190704144233.27968-7-anthony.perard@citrix.com (mailing list archive)
State Superseded
Headers show
Series Specific platform to run OVMF in Xen PVH and HVM guests | expand

Commit Message

Anthony PERARD July 4, 2019, 2:42 p.m. UTC
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

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

Comments

Andrew Cooper July 5, 2019, 1:57 p.m. UTC | #1
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
Laszlo Ersek July 5, 2019, 2:14 p.m. UTC | #2
On 07/04/19 16: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
> 
>  .../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

Acked-by: Laszlo Ersek <lersek@redhat.com>
Roger Pau Monné July 15, 2019, 11:46 a.m. UTC | #3
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.
Andrew Cooper July 15, 2019, 11:50 a.m. UTC | #4
On 15/07/2019 12:46, Roger Pau Monné wrote:
>> 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.

Its some OVMF local magic.  This means "jmp
end_of_TransitionFromReal16To32BitFlat", which is the correct place to
go, but the code really is misleading to read.

~Andrew
Roger Pau Monné July 15, 2019, 2:25 p.m. UTC | #5
On Mon, Jul 15, 2019 at 12:50:29PM +0100, Andrew Cooper wrote:
> On 15/07/2019 12:46, Roger Pau Monné wrote:
> >> +    ;
> >> +    ; 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.
> 
> Its some OVMF local magic.  This means "jmp
> end_of_TransitionFromReal16To32BitFlat", which is the correct place to
> go, but the code really is misleading to read.

Oh right, it's OneTimeCallRet. I guess this is obvious if you are
familiar with OVMF code, which I'm not. Expanding the comment to
mention that jumping to the end of the routine is achieved by using
OneTimeCallRet would have helped me, but this might be too verbose.

Thanks, Roger.
Anthony PERARD July 19, 2019, 10:20 a.m. UTC | #6
On Fri, Jul 05, 2019 at 02:57:06PM +0100, Andrew Cooper wrote:
> On 04/07/2019 15:42, Anthony PERARD wrote:
> > 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?

I don't know, this is existing code that I duplicated to allow adding a
new entry point. (I wanted to use --find-copies-harder when sending the
patch, but forgot this time. This part of the chunk would not be there.)

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

According to UefiCpuPkg/ResetVector/Vtf0/ReadMe.txt, that's a parameter
for the SEC image that this ResetVector locates then run.

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

Yes, that worked fine, but a subsequent patch is going to want to modify
the gdtr address, so I've been lazy and didn't use lgdt [ADDR_OF()]
here.
See: OvmfPkg/XenResetVector: Allow jumpstart from either hvmloader or PVH
https://patchew.org/EDK2/20190704144233.27968-1-anthony.perard@citrix.com/20190704144233.27968-9-anthony.perard@citrix.com/

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

I will actually save the start_info pointer before setting the gdt, but
that's done in a different patch:
OvmfPkg/XenResetVector: Saving start of day pointer for PVH guests
https://patchew.org/EDK2/20190704144233.27968-1-anthony.perard@citrix.com/20190704144233.27968-8-anthony.perard@citrix.com/

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

I didn't find the ${label}f syntax when reading the NASM manual. But
using .${label} would be the closest. Those labels starting with a dot
are called local labels. The actual full label, if one want to use it
from anywhere, would be "XenPVHMain.jmpToNewCodeSeg" here.

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

I look into that.

Thanks,
Anthony PERARD July 19, 2019, 2 p.m. UTC | #7
On Mon, Jul 15, 2019 at 01:46:57PM +0200, Roger Pau Monné wrote:
> On Thu, Jul 04, 2019 at 03:42:04PM +0100, Anthony PERARD wrote:
> > 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?

Maybe, but I can't change that. Blame the copyright owner ;-). I think
"All rights reserved." could also be removed, or may not apply
(anymore), but that's not something that this patch series can do and
not something I'm going to do :).

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

Maybe, but those were already there, so I don't feel comfortable
removing/changing them, or investigating.

FYI, I wanted to send this patch series with --find-copies-harder, but
failed. That chunk would have been instead:

  diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
  similarity index 72%
  copy from UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
  copy to OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
  index 7538192876..958195bc5e 100644
  --- a/UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
  +++ b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
  @@ -3,6 +3,8 @@
   ; First code executed by processor after resetting.
   ;
   ; Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.<BR>
  +; Copyright (c) 2019, Citrix Systems, Inc.
  +;
   ; SPDX-License-Identifier: BSD-2-Clause-Patent
   ;
   ;------------------------------------------------------------------------------
  @@ -21,9 +23,23 @@ ALIGN   16
   ; located just below 0x100000000 (4GB) in the firmware device.
   ;
   %ifdef ALIGN_TOP_TO_4K_FOR_PAGING
  -    TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0
  +    TIMES (0x1000 - ($ - EndOfPageTables) - (fourGigabytes - xenPVHEntryPoint)) DB 0
   %endif
  
  +BITS    32
  +xenPVHEntryPoint:
  +;
  +; Entry point to use when running as a Xen PVH guest. (0xffffffd0)
  +;
  +; 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
  +
   applicationProcessorEntryPoint:
   ;
   ; Application Processors entry point


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

I don't know. I tried to figure out, but couldn't find a useful answer.
I don't know enough about the build system to figure out how this module
gets build and how it is place exactly where it needs to be.

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

There is no such thing, at least not in a position that would be useful
for us. That code might be built into an ELF, but then that ELF (or just
the code maybe) gets packaged into a module that gets packaged into a FV
(firmware volume I think), which gets packaged into a flash device
image. (Hopefully, I'm not to far from the reality.)

> > +;
> > +; 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 don't know, better safe than sorry.

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

I'm guessing they do have different meaning, one doesn't set the other.
I could try to find out in the NASM manual if you really want to know.

Now that I've read what ALIGN mean (see below), they are both needed.
BITS to switch to 16bits machine code, ALIGN so that the next
instruction will be aligned.

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

Maybe xenPVHEntryPoint isn't at 0x..d0 ... and I'm lucky that what is
before it is padding. applicationProcessorEntryPoint should be at
0x..e0.

After looking at the assembly generated by nasm, I had a look at the
documentation of ALIGN
https://www.nasm.us/doc/nasmdoc4.html#section-4.11.13

ALIGN 16 is where the magic happen. When that macro is used, the next
thing is going to be on 0xXXX0 address. So ALIGN 16 is the thing adding
padding between the jmp in xenPVHEntryPoint and the first instruction in
applicationProcessorEntryPoint.

> > +; 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 might have a different meaning that what you think it has, see
above. Also, I don't really want to change the code that was there
before without a good enough reason, see the new diff that I've copied
above, the VTF thing was already there.

Thanks,
Laszlo Ersek July 19, 2019, 2:33 p.m. UTC | #8
On 07/19/19 12:20, Anthony PERARD wrote:
> On Fri, Jul 05, 2019 at 02:57:06PM +0100, Andrew Cooper wrote:
>> On 04/07/2019 15:42, Anthony PERARD wrote:
>>> 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?
> 
> I don't know, this is existing code that I duplicated to allow adding a
> new entry point. (I wanted to use --find-copies-harder when sending the
> patch, but forgot this time.

Not a big problem; while reviewing v3, I did such comparisons myself, in
my local clone. Feel free to skip "--find-copies-harder" when posting v4
too; I think there isn't much churn going on in parallel right now.

However, a new request for v4: please make sure that the new modules /
paths introduced by this patch set are covered in Maintainers.txt.

> This part of the chunk would not be there.)

Regarding the NOPs: all I can tell you is that they originate from
commit 8332983e2e33 ("UefiCpuPkg: Replace the un-necessary WBINVD
instruction at the reset vector with two NOPs in VTF0.", 2011-08-04).

Whether that change made sense back then, let alone if it makes sense
now: no clue.

Thanks
Laszlo
Andrew Cooper July 19, 2019, 2:41 p.m. UTC | #9
On 19/07/2019 15:33, Laszlo Ersek wrote:
> On 07/19/19 12:20, Anthony PERARD wrote:
>> On Fri, Jul 05, 2019 at 02:57:06PM +0100, Andrew Cooper wrote:
>>> On 04/07/2019 15:42, Anthony PERARD wrote:
>>>> 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?
>> I don't know, this is existing code that I duplicated to allow adding a
>> new entry point. (I wanted to use --find-copies-harder when sending the
>> patch, but forgot this time.
> Not a big problem; while reviewing v3, I did such comparisons myself, in
> my local clone. Feel free to skip "--find-copies-harder" when posting v4
> too; I think there isn't much churn going on in parallel right now.
>
> However, a new request for v4: please make sure that the new modules /
> paths introduced by this patch set are covered in Maintainers.txt.
>
>> This part of the chunk would not be there.)
> Regarding the NOPs: all I can tell you is that they originate from
> commit 8332983e2e33 ("UefiCpuPkg: Replace the un-necessary WBINVD
> instruction at the reset vector with two NOPs in VTF0.", 2011-08-04).
>
> Whether that change made sense back then, let alone if it makes sense
> now: no clue.

Dropping wbinvd makes sense, because when virtualised, the caches (and
paging for that matter) are always up and running correctly.  Its an
unnecessary vmexit for something which the hypervisor will nop out anyway.

Leaving two nops behind makes no sense at all.

~Andrew
Anthony PERARD July 19, 2019, 3:51 p.m. UTC | #10
On Fri, Jul 19, 2019 at 03:41:52PM +0100, Andrew Cooper wrote:
> On 19/07/2019 15:33, Laszlo Ersek wrote:
> > On 07/19/19 12:20, Anthony PERARD wrote:
> >> On Fri, Jul 05, 2019 at 02:57:06PM +0100, Andrew Cooper wrote:
> >>> On 04/07/2019 15:42, Anthony PERARD wrote:
> >>>> 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?
> >> I don't know, this is existing code that I duplicated to allow adding a
> >> new entry point. (I wanted to use --find-copies-harder when sending the
> >> patch, but forgot this time.
> > Not a big problem; while reviewing v3, I did such comparisons myself, in
> > my local clone. Feel free to skip "--find-copies-harder" when posting v4
> > too; I think there isn't much churn going on in parallel right now.
> >
> > However, a new request for v4: please make sure that the new modules /
> > paths introduced by this patch set are covered in Maintainers.txt.

Will do.

> >> This part of the chunk would not be there.)
> > Regarding the NOPs: all I can tell you is that they originate from
> > commit 8332983e2e33 ("UefiCpuPkg: Replace the un-necessary WBINVD
> > instruction at the reset vector with two NOPs in VTF0.", 2011-08-04).
> >
> > Whether that change made sense back then, let alone if it makes sense
> > now: no clue.
> 
> Dropping wbinvd makes sense, because when virtualised, the caches (and
> paging for that matter) are always up and running correctly.  Its an
> unnecessary vmexit for something which the hypervisor will nop out anyway.
> 
> Leaving two nops behind makes no sense at all.

I'll remove the nops.

Thanks,
diff mbox series

Patch

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>
+; Copyright (c) 2019, Citrix Systems, Inc.
+;
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+BITS    16
+
+ALIGN   16
+
+;
+; 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
+%endif
+
+BITS    32
+xenPVHEntryPoint:
+;
+; Entry point to use when running as a Xen PVH guest. (0xffffffd0)
+;
+; 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
+
+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
+; used to wake up the application processors.
+;
+    jmp     EarlyApInitReal16
+
+ALIGN   8
+
+    DD      0
+
+;
+; The VTF signature
+;
+; VTF-0 means that the VTF (Volume Top File) code does not require
+; any fixups.
+;
+vtfSignature:
+    DB      'V', 'T', 'F', 0
+
+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
diff --git a/OvmfPkg/XenResetVector/XenResetVector.nasmb b/OvmfPkg/XenResetVector/XenResetVector.nasmb
index 89a4b08bc3..0dbc4f2c1d 100644
--- a/OvmfPkg/XenResetVector/XenResetVector.nasmb
+++ b/OvmfPkg/XenResetVector/XenResetVector.nasmb
@@ -63,6 +63,7 @@ 
 %include "Ia16/Init16.asm"
 
 %include "Main.asm"
+%include "Ia32/XenPVHMain.asm"
 
 %include "Ia16/ResetVectorVtf0.asm"