From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: citrix.com, ip: 216.71.145.155, mailfrom: roger.pau@citrix.com) Received: from esa3.hc3370-68.iphmx.com (esa3.hc3370-68.iphmx.com [216.71.145.155]) by groups.io with SMTP; Mon, 15 Jul 2019 04:47:08 -0700 Authentication-Results: esa3.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=roger.pau@citrix.com; spf=Pass smtp.mailfrom=roger.pau@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa3.hc3370-68.iphmx.com: no sender authenticity information available from domain of roger.pau@citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa3.hc3370-68.iphmx.com; envelope-from="roger.pau@citrix.com"; x-sender="roger.pau@citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa3.hc3370-68.iphmx.com: domain of roger.pau@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa3.hc3370-68.iphmx.com; envelope-from="roger.pau@citrix.com"; x-sender="roger.pau@citrix.com"; x-conformance=sidf_compatible; x-record-type="v=spf1"; x-record-text="v=spf1 ip4:209.167.231.154 ip4:178.63.86.133 ip4:195.66.111.40/30 ip4:85.115.9.32/28 ip4:199.102.83.4 ip4:192.28.146.160 ip4:192.28.146.107 ip4:216.52.6.88 ip4:216.52.6.188 ip4:162.221.158.21 ip4:162.221.156.83 ~all" Received-SPF: None (esa3.hc3370-68.iphmx.com: no sender authenticity information available from domain of postmaster@mail.citrix.com) identity=helo; client-ip=162.221.158.21; receiver=esa3.hc3370-68.iphmx.com; envelope-from="roger.pau@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: vmn/VJS6z2hBijZ21I5klOubD6H4KW8PDD95WBAKoMDrCJ/5vjRH2gqt1ITV9q0YtOYMOsWVpR Wd7cW8paphaeb0+AaBRCleygIwn7u2oeWT6MofBs4yOU6K2oJESk5BLLZRLEyisA5/BgiLyW61 svNjOxzwIGYi9by2hTX3yO0//n2tMDagPX68HwsW/YF0ZOHljo7fRy5lOTOMkDKVkIBKi1KXPX aMQ/HRr/Gn766w5LIa2459FQNX3SaKCrlzbFPGm7vmQRjQcx1ekxCTQAi3rO1eaiIzsPBWPV6z VTM= X-SBRS: 2.7 X-MesageID: 2975771 X-Ironport-Server: esa3.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.63,493,1557201600"; d="scan'208";a="2975771" Date: Mon, 15 Jul 2019 13:46:57 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: Anthony PERARD CC: , , Ard Biesheuvel , Jordan Justen , Laszlo Ersek , Julien Grall , "Andrew Cooper" Subject: Re: [PATCH v3 06/35] OvmfPkg/XenResetVector: Add new entry point for Xen PVH Message-ID: <20190715114657.kct664fsiupfbftf@MacBook-Air-de-Roger.local> References: <20190704144233.27968-1-anthony.perard@citrix.com> <20190704144233.27968-7-anthony.perard@citrix.com> MIME-Version: 1.0 In-Reply-To: <20190704144233.27968-7-anthony.perard@citrix.com> User-Agent: NeoMutt/20180716 Return-Path: roger.pau@citrix.com X-ClientProxiedBy: AMSPEX02CAS02.citrite.net (10.69.22.113) To AMSPEX02CL02.citrite.net (10.69.22.126) Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline 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 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.
Extraneous
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.