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

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

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,

-- 
Anthony PERARD