Re: [edk2-devel] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Better support for dynamic PcdFSBClock

["joeyli" <jlee@suse.com>]

On Tue, Nov 08, 2022 at 02:31:48PM +0000, Anthony PERARD via groups.io wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> The PcdFSBClock can be a dynamic PCD. This can be an issue when
> InternalX86GetTimerFrequency() tries to get the value while been
> called with TPL been set to TPL_HIGH_LEVEL.
> 
> When the PCD is dynamic, PcdGet*() calls a function that try to grab a
> lock which set TPL to TPL_NOTIFY. If TPL is already at TPL_HIGH_LEVEL,
> then an assert() in RaiseTPL() fails (in debug build).
> 
> To try to avoid the issue, we'll store the current PcdFSBClock value
> in a local variable the first time we need it.
> 
> The issue was discovered while attempting to boot a Windows guest with
> OvmfXen platform. The issue appear while executing the Windows's boot
> loader EFI application.
> 
> The down side of this change is that when the PCD is FixedAtBuild, the
> value is loaded from a variable rather than been a constant.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

I have tested this patch with edk2-stable202202 and edk2-stable202211. It
works to me to fix problem on bto#4340:

Bug 4340 - Running windows 2k22 on OvmfXen got ASSERT /root/source-git/edk2/MdeModulePkg/Core/Dxe/Event/Tpl.c(66): ((BOOLEAN)(0==1))
https://bugzilla.tianocore.org/show_bug.cgi?id=4340

Thanks a lot!
Joey Lee

> ---
> 
> InternalX86GetTimerFrequency() is called by
> - MicroSecondDelay()
> - NanoSecondDelay()
> - GetPerformanceCounterProperties()
> 
> If one of those functions is called for the first time with a TPL too
> high, the work around in this patch would not work. But it seems to
> workaround the issue in my test case. So maybe that's enough, unless
> someone have a better idea?
> ---
>  MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c b/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c
> index c60589607fde..28ff77ad0c1f 100644
> --- a/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c
> +++ b/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c
> @@ -90,8 +90,16 @@ InternalX86GetTimerFrequency (
>    IN      UINTN  ApicBase
>    )
>  {
> +  STATIC UINT32 mFSBClock;
> +
> +  if (mFSBClock == 0) {
> +      //
> +      // Cache current value of PcdFSBClock in case it's a dynamic PCD.
> +      //
> +      mFSBClock = PcdGet32 (PcdFSBClock);
> +  }
>    return
> -    PcdGet32 (PcdFSBClock) /
> +    mFSBClock /
>      mTimerLibLocalApicDivisor[MmioBitFieldRead32 (ApicBase + APIC_TDCR, 0, 3)];
>  }
>  
> -- 
> Anthony PERARD
> 
> 
> 
> 
> 
>