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

["Anthony PERARD" <anthony.perard@citrix.com>]

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