| Message ID | 20200129121235.1814563-6-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | OvmfXen: Set PcdFSBClock at runtime | expand |
On 01/29/20 13:12, Anthony PERARD 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. > > 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 > > 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. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490 > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > 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 8c11efe9b709..190d7400c148 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 > - > ################################################################################ > # > # Pcd Dynamic Section - list of all EDK II PCD Entries defined by this Platform > @@ -468,6 +465,7 @@ [PcdsDynamicDefault] > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000 > > + gEfiMdePkgTokenSpaceGuid.PcdFSBClock (1) This syntax looks strange; I thought it was mandatory to provide a default value too. https://edk2-docs.gitbooks.io/edk-ii-dsc-specification/content/2_dsc_overview/28_pcd_sections.html --------- 2.8.3.1 PcdsDynamicDefault [...] The format for a boolean or numeric datum type PCD entry in this section is: PcdTokenSpaceGuidCName.PcdCName|Value --------- I'm not sure if the "build" utility accepts this intentionally, or by mistake. Can you simply keep the "|100000000" part too? Otherwise, I'm OK with the argument laid out in the commit message. (Thank you for the detailed commit message!) With (1) fixed: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo > gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0 > > # Set video resolution for text setup. > diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > index 335a442538c2..177200f3b7e5 100644 > --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > @@ -83,6 +83,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 d6cdc9a8e31c..fc990462dccc 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -504,6 +504,10 @@ CalibrateLapicTimer ( > / (TscTick2 - TscTick); > DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq)); > > + ASSERT (Freq <= MAX_UINT32); > + Status = PcdSet32S (PcdFSBClock, Freq); > + ASSERT_RETURN_ERROR (Status); > + > UnmapXenPage (SharedInfo); > > Exit: >
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc index 8c11efe9b709..190d7400c148 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 - ################################################################################ # # Pcd Dynamic Section - list of all EDK II PCD Entries defined by this Platform @@ -468,6 +465,7 @@ [PcdsDynamicDefault] gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0 gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000 + gEfiMdePkgTokenSpaceGuid.PcdFSBClock gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0 # Set video resolution for text setup. diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf index 335a442538c2..177200f3b7e5 100644 --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf @@ -83,6 +83,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 d6cdc9a8e31c..fc990462dccc 100644 --- a/OvmfPkg/XenPlatformPei/Xen.c +++ b/OvmfPkg/XenPlatformPei/Xen.c @@ -504,6 +504,10 @@ CalibrateLapicTimer ( / (TscTick2 - TscTick); DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq)); + ASSERT (Freq <= MAX_UINT32); + Status = PcdSet32S (PcdFSBClock, Freq); + ASSERT_RETURN_ERROR (Status); + UnmapXenPage (SharedInfo); Exit:
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. 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 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. Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490 Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- OvmfPkg/OvmfXen.dsc | 4 +--- OvmfPkg/XenPlatformPei/XenPlatformPei.inf | 1 + OvmfPkg/XenPlatformPei/Xen.c | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)