| 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 |
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
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>
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.
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
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.
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,
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,
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
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
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 --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"
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