[edk2-devel] [PATCH v2 7/7] OvmfPkg/OvmfXen: Set PcdFSBClock
Laszlo Ersek
lersek at redhat.com
Wed Apr 7 09:25:49 UTC 2021
On 03/25/21 16:47, Anthony PERARD via groups.io wrote:
> Update gEfiMdePkgTokenSpaceGuid.PcdFSBClock so it can have the correct
> value when SecPeiDxeTimerLibCpu start to use it for the APIC timer.
>
> Currently, nothing appear to use the value in PcdFSBClock before
> XenPlatformPei had a chance to set it even though TimerLib is included
> in modules runned before XenPlatformPei.
(1) s/runned/run/
>
> XenPlatformPei doesn't use any of the functions that would use that
> value. No other modules in the PEI phase seems to use the TimerLib
> before PcdFSBClock is set. There are currently two other modules in
> the PEI phase that needs the TimerLib:
> - S3Resume2Pei, but only because LocalApicLib needs it, but nothing is
> using the value from PcdFSBClock.
> - CpuMpPei, but I believe it only runs after XenPlatformPei
Effectively, yes.
* Assuming CpuMpPei were loaded / dispatched before XenPlatformPei, the
following would happen:
CpuMpPeimInit() [UefiCpuPkg/CpuMpPei/CpuMpPei.c] is the entry point
function of CpuMpPei, and all it does is register a notify for the
"memory discovered" PPI. The actual module initialization occurs in
MemoryDiscoveredPpiNotifyCallback().
The "memory discovered PPI" is produced as follows:
- XenPlatformPei calls PublishSystemMemory()
- XenPlatformPei exits
- the PEI Core relocates stuff (HOBs, PEIMs) to the permanent PEI
memory, and re-enters itself in the new area
- the PEI core installs the "memory discovered" PPI
- MemoryDiscoveredPpiNotifyCallback() is called.
* If XenPlatformPei is loaded / dispatched before CpuMpPei (and so the
"memory discovered" PPI exist at the time of CpuMpPei starting), then
MemoryDiscoveredPpiNotifyCallback() is immediately invoked in CpuMpPei,
when the PPI notify is registered.
>
> Before the PEI phase, there's the SEC phase, and SecMain needs
> TimerLib because of LocalApicLib. And it initialise the APIC timers
> for the debug agent. But I don't think any of the DebugLib that
> OvmfXen could use are actually using the *Delay functions in TimerLib,
> and so would not use the value from PcdFSBClock which would be
> uninitialised.
>
> A simple runtime test showed that TimerLib doesn't use PcdFSBClock
> value before it is set.
OK.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard at citrix.com>
> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
> ---
>
> Notes:
> v2:
> - keep the default value of PcdFSBClock because that is part of the syntax.
>
> OvmfPkg/OvmfXen.dsc | 4 +---
> OvmfPkg/XenPlatformPei/XenPlatformPei.inf | 1 +
> OvmfPkg/XenPlatformPei/Xen.c | 4 ++++
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 507029404f0b..faf3930ace04 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -442,9 +442,6 @@ [PcdsFixedAtBuild]
> # Point to the MdeModulePkg/Application/UiApp/UiApp.inf
> gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
>
> - ## Xen vlapic's frequence is 100 MHz
> - gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000
> -
> # We populate DXE IPL tables with 1G pages preferably on Xen
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
>
> @@ -471,6 +468,7 @@ [PcdsDynamicDefault]
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
>
> + gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000
> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>
> # Set video resolution for text setup.
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 5732d2188871..87dd4b24679a 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -85,6 +85,7 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> + gEfiMdePkgTokenSpaceGuid.PcdFSBClock
> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
> gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
>
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index 7524aaa11a29..a29b4e04086e 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -632,5 +632,9 @@ CalibrateLapicTimer (
> Freq = DivU64x64Remainder (Dividend, TscTick2 - TscTick, NULL);
> DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
>
> + ASSERT (Freq <= MAX_UINT32);
> + Status = PcdSet32S (PcdFSBClock, Freq);
(2) Please use an explicit cast here: (UINT32)Freq; I'm concerned about
VS emitting a warning (despite the ASSERT()).
> + ASSERT_RETURN_ERROR (Status);
(3) "Status" has type EFI_STATUS here; the assignment from PcdSet32S()
(RETURN_STATUS) is fine, but it's more idiomatic to use
ASSERT_EFI_ERROR(), given the type of "Status".
> +
> UnmapXenPage (SharedInfo);
> }
>
Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73772): https://edk2.groups.io/g/devel/message/73772
Mute This Topic: https://groups.io/mt/81605810/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list