Re: [PATCH v3 04/35] OvmfPkg: Introduce XenPlatformPei

["Laszlo Ersek" <lersek@redhat.com>]

Hi Anthony,

On 07/04/19 16:42, Anthony PERARD wrote:
> Introduce XenPlatformPei, a copy of OvmfPkg/PlatformPei without some
> of QEMU specific initialization, Xen does not support QemuFwCfg.
> 
> This new module will be adjusted to accommodate Xen PVH.
> 
> fw_cfg dependents that have been removed, which are dynamically skipped
> when running PlatformPei on Xen:
> - 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
> fw_cfg "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 FeatureControl.c as there is nothing been executed)
> 
> Also removed:
> - SMRAM/TSEG-related low mem size adjusting (PcdSmmSmramRequire is
> assumed FALSE) in PublishPeiMemory(),
> - QemuInitializeRam() entirely,
> 
> Xen related changes:
> - Have removed the module variable mXen, as it should be always true.
> - Have the platform PEI initialization fails if Xen has not been
>   detected.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v3:
>       - fix coding style in new code
>         (use DEBUG_xxx, add ASSERT before CpuDeadLoop)
>       - rebased, SPDX

In my v2 review at
<http://mid.mail-archive.com/994d01f3-bae7-c447-6e28-5a61a84464ac@redhat.com>,
point (2), I meant that you should please replace *all* EFI_D_xxx macros
with DEBUG_xxx macros, in new code.

By "new code" I didn't mean code that you wrote afresh, but simply
source code that appeared as new -- i.e., as an addition -- in the
patch. That is, all the EFI_D_xxx macros that you inherit from
PlatformPei by virtue of copying should be replaced as well.

This v3 patch continues to add 12 instances of EFI_D_xxx, by my count.
Please replace all of those, with DEBUG_xxx.

The rest of the updates in v3 are good, and sufficient.

Thanks
Laszlo