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.142, mailfrom: anthony.perard@citrix.com) Received: from esa1.hc3370-68.iphmx.com (esa1.hc3370-68.iphmx.com [216.71.145.142]) by groups.io with SMTP; Fri, 19 Jul 2019 07:01:06 -0700 Authentication-Results: esa1.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=anthony.perard@citrix.com; spf=Pass smtp.mailfrom=anthony.perard@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa1.hc3370-68.iphmx.com: no sender authenticity information available from domain of anthony.perard@citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa1.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="anthony.perard@citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa1.hc3370-68.iphmx.com: domain of anthony.perard@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa1.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="anthony.perard@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 (esa1.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=esa1.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: N7m4TikO+G94ksAe0HCgycO2LcavsJOj+Zj2330IM1LOMlDWqsCLkiihQmSEBa0JjFOdhKYhhy yVqmPH2wj6zZDQKSbn9rec0xjBY+YHhfPFoDSjXm6bnpwGTRZzTwJM3WcCDPV4eYoNcLBnzdlk m3rYLy5aLc+p9ivL55cxX/m5ofo7R0NX22hIhP3L18H4wASFS0nrouu3ctOBbs7g7EkAl170js PYhFt8odoi3aXxSHgrbFe6k8mNSEvYjDCPw+cqA00el5Fzi2ODV3rJid0xHRL8jdzkSVqadWHy z9Q= X-SBRS: 2.7 X-MesageID: 3219291 X-Ironport-Server: esa1.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.64,282,1559534400"; d="scan'208";a="3219291" Date: Fri, 19 Jul 2019 15:00:37 +0100 From: "Anthony PERARD" To: Roger Pau =?iso-8859-1?Q?Monn=E9?= 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: <20190719140037.GC1208@perard.uk.xensource.com> References: <20190704144233.27968-1-anthony.perard@citrix.com> <20190704144233.27968-7-anthony.perard@citrix.com> <20190715114657.kct664fsiupfbftf@MacBook-Air-de-Roger.local> MIME-Version: 1.0 In-Reply-To: <20190715114657.kct664fsiupfbftf@MacBook-Air-de-Roger.local> User-Agent: Mutt/1.12.1 (2019-06-15) Return-Path: anthony.perard@citrix.com Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: 8bit 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.
> > Extraneous
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.
+; 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