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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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