From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from SMTP.CITRIX.COM (smtp.citrix.com [66.165.176.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 462928196F for ; Tue, 10 Jan 2017 08:21:44 -0800 (PST) X-IronPort-AV: E=Sophos;i="5.33,344,1477958400"; d="scan'208";a="398973748" Date: Tue, 10 Jan 2017 16:08:23 +0000 From: Anthony PERARD To: Laszlo Ersek CC: , Message-ID: <20170110160823.GE1903@perard.uk.xensource.com> References: <20161208153340.2285-1-anthony.perard@citrix.com> <20161208153340.2285-5-anthony.perard@citrix.com> <18f5f2d0-7b42-8fdf-3342-f98b186337d8@redhat.com> MIME-Version: 1.0 In-Reply-To: <18f5f2d0-7b42-8fdf-3342-f98b186337d8@redhat.com> User-Agent: Mutt/1.7.2 (2016-11-26) Subject: Re: [PATCH RFC 04/14] OvmfPkg: Introduce XenPlatformPei X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Jan 2017 16:21:44 -0000 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline On Thu, Jan 05, 2017 at 10:59:24AM +0100, Laszlo Ersek wrote: > On 12/08/16 16:33, Anthony PERARD wrote: > > A copy of OvmfPkg/PlatformPei without some of QEMU specific > > initialization, Xen does not support QemuFwCfg. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Anthony PERARD > > --- > > OvmfPkg/XenOvmf.dsc | 2 +- > > OvmfPkg/XenOvmf.fdf | 2 +- > > OvmfPkg/XenPlatformPei/Cmos.c | 64 ++++ > > OvmfPkg/XenPlatformPei/Cmos.h | 56 ++++ > > OvmfPkg/XenPlatformPei/Fv.c | 100 ++++++ > > OvmfPkg/XenPlatformPei/MemDetect.c | 449 +++++++++++++++++++++++++ > > OvmfPkg/XenPlatformPei/Platform.c | 536 ++++++++++++++++++++++++++++++ > > OvmfPkg/XenPlatformPei/Platform.h | 104 ++++++ > > OvmfPkg/XenPlatformPei/Xen.c | 231 +++++++++++++ > > OvmfPkg/XenPlatformPei/Xen.h | 45 +++ > > OvmfPkg/XenPlatformPei/XenPlatformPei.inf | 110 ++++++ > > 11 files changed, 1697 insertions(+), 2 deletions(-) > > create mode 100644 OvmfPkg/XenPlatformPei/Cmos.c > > create mode 100644 OvmfPkg/XenPlatformPei/Cmos.h > > create mode 100644 OvmfPkg/XenPlatformPei/Fv.c > > create mode 100644 OvmfPkg/XenPlatformPei/MemDetect.c > > create mode 100644 OvmfPkg/XenPlatformPei/Platform.c > > create mode 100644 OvmfPkg/XenPlatformPei/Platform.h > > create mode 100644 OvmfPkg/XenPlatformPei/Xen.c > > create mode 100644 OvmfPkg/XenPlatformPei/Xen.h > > create mode 100644 OvmfPkg/XenPlatformPei/XenPlatformPei.inf > > (1) You might want to add Citrix copyright notices to new files. > > (2) This module is a good example where the moved Xen functionality > (even for HVM guests) should be possible to remove from the original > PlatformPei module. (In a later, final patch.) Is that right? Yes, that should be possible. > (3) In the commit message, please list the removed fw_cfg-dependent > functionality in more detail (all of which is dynamically skipped when > the current PlatformPei module runs on Xen): Will do. > - GetFirstNonAddress(): controlling the 64-bit PCI MMIO aperture via the > (experimental) "opt/ovmf/X-PciMmio64Mb" file > > - GetFirstNonAddress(): honoring the hotplug DIMM area > ("etc/reserved-memory-end") in the placement of the 64-bit PCI MMIO aperture > > - NoexecDxeInitialization() is removed, so PcdPropertiesTableEnable and > PcdSetNxForStack are left constant FALSE (not set dynamically from > "opt/ovmf/PcdXxxx") > > - MaxCpuCountInitialization(), PublishPeiMemory(): the max CPU count is > not taken from the QemuFwCfgItemSmpCpuCount fw_cfg key; > PcdCpuMaxLogicalProcessorNumber is used intact and > PcdCpuApInitTimeOutInMicroSeconds is never changed or used. > > - InitializeXenPlatform(), S3Verification(): S3 is assumed disabled (not > consulting "etc/system-states" via QemuFwCfgS3Enabled()). > > - InstallFeatureControlCallback(): the feature control MSR is not set > from "etc/msr_feature_control" > > Also removed: > - SMRAM/TSEG-related low mem size adjusting (PcdSmmSmramRequire is > assumed FALSE) in PublishPeiMemory(), > - QemuInitializeRam() entirely, > > (4) I think the removal of PcdSmmSmramRequire is incomplete. IMO this > PCD should either be removed completely (all uses, including the INF > reference), assuming a FALSE value in its place, or the PCD should not > be touched at all. The FeaturePcdGet() call in PublishPeiMemory() -- > gating the "TSEG is chipped from the end of low RAM" stuff -- seems to > be singled out for removal for no good reason. Yes, I think I will remove all of if. There is not much reason to keep something that is not tested, and I don't know if it can work on Xen. I guess SMM can be readded later. > (5) It would be nice to hint at the reason (which is implemented in a > later patch) why a separate module is a good idea here. That is, why the > expected PVH2 functionality is best not added to the current PlatformPei > module. Will do. -- Anthony PERARD