| Message ID | 20210325154713.670104-8-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OvmfXen: Set PcdFSBClock at runtime | expand |
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@citrix.com> > Reviewed-by: Laszlo Ersek <lersek@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@redhat.com> Thanks Laszlo
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); + ASSERT_RETURN_ERROR (Status); + UnmapXenPage (SharedInfo); }