Re: [PATCH RFC 04/14] OvmfPkg: Introduce XenPlatformPei

[Anthony PERARD <anthony.perard@citrix.com>]

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 <anthony.perard@citrix.com>
> > ---
> >  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