[PATCH] MdePkg/SecPeiDxeTimerLibCpu: Better support for dynamic PcdFSBClock

[PATCH] MdePkg/SecPeiDxeTimerLibCpu: Better support for dynamic PcdFSBClock

[Anthony PERARD]

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>
---

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

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

[joeyli]

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
> 
> 
> 
> 
> 
> 

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

[Michael D Kinney]

This library is intended to be compatible with XIP SEC and XIP PEIMs that are not
allowed to use writable global C variables.

I think storage for STATIC UINT32 mFSBClock is from the .data section and not
the stack, so this will break XIP use cases.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of joeyli via groups.io
> Sent: Thursday, February 9, 2023 5:27 PM
> To: devel@edk2.groups.io; anthony.perard@citrix.com
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Better support for dynamic PcdFSBClock
> 
> 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
> >
> >
> >
> >
> >
> >
> 
> 
> 
>