From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: citrix.com, ip: 162.221.156.55, mailfrom: prvs=0011d4d25=anthony.perard@citrix.com) Received: from SMTP03.CITRIX.COM (SMTP03.CITRIX.COM [162.221.156.55]) by groups.io with SMTP; Mon, 15 Apr 2019 04:25:55 -0700 X-IronPort-AV: E=Sophos;i="5.60,353,1549929600"; d="scan'208";a="83567470" Date: Mon, 15 Apr 2019 12:25:47 +0100 From: "Anthony PERARD" To: Andrew Cooper CC: , Ard Biesheuvel , Jordan Justen , Julien Grall , , Laszlo Ersek Subject: Re: [Xen-devel] [PATCH v2 06/31] OvmfPkg/XenResetVector: Add new entry point for Xen PVH Message-ID: <20190415112547.GB1354@perard.uk.xensource.com> References: <20190409110844.14746-1-anthony.perard@citrix.com> <20190409110844.14746-7-anthony.perard@citrix.com> <2fbfe0f2-5c34-9faa-bda5-88223770c936@citrix.com> MIME-Version: 1.0 In-Reply-To: <2fbfe0f2-5c34-9faa-bda5-88223770c936@citrix.com> User-Agent: Mutt/1.11.4 (2019-03-13) Return-Path: anthony.perard@citrix.com Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: 8bit On Thu, Apr 11, 2019 at 01:52:27PM +0100, Andrew Cooper wrote: > On 09/04/2019 12:08, Anthony PERARD wrote: > > diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm > > new file mode 100644 > > index 0000000000..c4802bf4d1 > > --- /dev/null > > +++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm > > @@ -0,0 +1,47 @@ > > +;------------------------------------------------------------------------------ > > +; @file > > +; An entry point use by Xen when a guest is started in PVH mode. > > +; > > +; Copyright (c) 2019, Citrix Systems, Inc. > > +; > > +; This program and the accompanying materials are licensed and made available > > +; under the terms and conditions of the BSD License which accompanies this > > +; distribution. The full text of the license may be found at > > +; http://opensource.org/licenses/bsd-license.php > > +; > > +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > > +; WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > +; > > +;------------------------------------------------------------------------------ > > + > > +BITS 32 > > + > > +xenPVHMain: > > + mov di, 'BP' > > + > > + ; ESP - Initial value of the EAX register (BIST: Built-in Self Test) > > + mov esp, eax > > Where is the ABI described?  Xen has no BIST, so this will always have > the value 0. Stashing it in the stack pointer seems like a weird choice, > and a recipe for subtle bugs. I've just copied over what was done in the HVM case. I'll change that and ignore the value in `eax'. > > + > > + cli > > Interrupts are guaranteed to be off at this point. OK, I'll remove the instruction. > > + > > + mov ebx, ADDR_OF(gdtr) > > + lgdt [ebx] > > lgdt ADDR_OF(gdtr), presumably? > > This is 32bit code - there is no need for any indirection through > registers for memory operands. I'll change this, if it works. > > + > > + 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 > > + > > + ; return to the Main16 > > + OneTimeCallRet TransitionFromReal16To32BitFlat > > Is there any description of what OneTimeCallRet is, and why a simple jmp > wont do? That's is used to return to where the `OneTimeCall' macro has been used. In this assembly, they don't use `call' and `ret', instead there is a set of two macros: %macro OneTimeCall 1 jmp %1 %1 %+ OneTimerCallReturn: %endmacro %macro OneTimeCallRet 1 jmp %1 %+ OneTimerCallReturn %endmacro > Irrespective of that, you're moving to a function whose name suggests it > is in 16bit mode, while you are currently in 32bit flat mode.  > (SEC_DEFAULT_CR0 has PE set, and LINEAR_SEL is 32bit flat.  This clearly > isn't correct, but surely we want to skip all the 16bit setup, as well. I think I need to change the comment. Main16 is just a label name for something that makes several "calls" and transition from 16bits to 32 or 64 before calling the next module in the firmware. That return simply jmp to the part of the main where we are supposed to be in 32bit flat mode. Thanks for the review, -- Anthony PERARD