Hi Marvin, We should not add a dependency on the UefiCpuPkg to the MdePkg. I agree that a better location of the local APIC based timer lib would be the UefiCpuPkg, and there is already one there called SecPeiDxeTimerLibUefiCpu.
The TimeLib in the MdePkg was created before the UefiCpuPkg was created, which explains the current state of the MdePkg. The MdePkg version has other limitations noted in the INF and recommends the UefiCpuPkg version be used. Thanks, Mike > -----Original Message----- > From: Marvin Häuser [mailto:[email protected]] > Sent: Sunday, July 23, 2017 3:19 AM > To: [email protected] > Cc: Kinney, Michael D <[email protected]>; Gao, Liming > <[email protected]> > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > UefiCpuPkg LAPIC code. > > Dear Michael and Liming, > > I submited the patch as the changes need to be done anyway, > though I think the library might be better moved to UefiCpuPkg. > Also, is my understanding of the mask value being incorrect > right? I was confused by the 'ULL' suffix, which makes it look > like it was intended. Is it? > > Regards, > Marvin. > > > -----Original Message----- > > From: edk2-devel [mailto:[email protected]] On > Behalf Of > > Marvin Häuser > > Sent: Sunday, July 23, 2017 12:12 PM > > To: [email protected] > > Cc: [email protected]; [email protected] > > Subject: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > UefiCpuPkg LAPIC code. > > > > X86TimerLib is changed to use UefiCpuPkg LAPIC register > definitions and > > LocalApicLib to remove duplicated code. An implicite change is > the value > > returned by InternalX86GetApicBase() as it now returns the > result of > > GetLocalApicBaseAddress(), which is the full LAPIC address. > This also > > implicitely fixes the incorrect mask value used previously, > which did not only > > mask AcpiBase, but also the first nibble of AcpiBaseHi. This > does not apply to > > 32-bit platforms. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Marvin Haeuser <[email protected]> > > --- > > MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c | > 35 +++++++-- > > ----------- > > MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf | > 4 ++- > > MdePkg/MdePkg.dsc | > 3 ++ > > 3 files changed, 18 insertions(+), 24 deletions(-) > > > > diff --git a/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c > > b/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c > > index 76c66fbce6fb..fa6e6f213029 100644 > > --- a/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c > > +++ b/MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c > > @@ -1,7 +1,7 @@ > > /** @file > > Timer Library functions built upon local APIC on IA32/x64. > > > > - Copyright (c) 2006 - 2015, Intel Corporation. All rights > reserved.<BR> > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights > > + reserved.<BR> > > This program and the accompanying materials > > are licensed and made available under the terms and > conditions of the BSD > > License > > which accompanies this distribution. The full text of the > license may be > > found at @@ -13,18 +13,14 @@ **/ > > > > #include <Base.h> > > +#include <Register/LocalApic.h> > > +#include <Library/LocalApicLib.h> > > #include <Library/TimerLib.h> > > #include <Library/BaseLib.h> > > #include <Library/IoLib.h> > > #include <Library/PcdLib.h> > > #include <Library/DebugLib.h> > > > > -#define APIC_SVR 0x0f0 > > -#define APIC_LVTERR 0x370 > > -#define APIC_TMICT 0x380 > > -#define APIC_TMCCT 0x390 > > -#define APIC_TDCR 0x3e0 > > - > > // > > // The following array is used in calculating the frequency of > local APIC // > > timer. Refer to IA-32 developers' manual for more details. > > @@ -54,30 +50,21 @@ InternalX86GetApicBase ( > > VOID > > ) > > { > > - UINTN MsrValue; > > UINTN ApicBase; > > > > - MsrValue = (UINTN) AsmReadMsr64 (27); > > - ApicBase = MsrValue & 0xffffff000ULL; > > - > > // > > - // Check the APIC Global Enable bit (bit 11) in > IA32_APIC_BASE MSR. > > - // This bit will be 1, if local APIC is globally enabled. > > + // Verify local APIC is under XAPIC mode. > > // > > - ASSERT ((MsrValue & BIT11) != 0); > > + ASSERT (GetApicMode () == LOCAL_APIC_MODE_XAPIC); > > > > - // > > - // Check the APIC Extended Mode bit (bit 10) in > IA32_APIC_BASE MSR. > > - // This bit will be 0, if local APIC is under XAPIC mode. > > - // > > - ASSERT ((MsrValue & BIT10) == 0); > > + ApicBase = GetLocalApicBaseAddress (); > > > > // > > // Check the APIC Software Enable/Disable bit (bit 8) in > Spurious-Interrupt > > // Vector Register. > > // This bit will be 1, if local APIC is software enabled. > > // > > - ASSERT ((MmioRead32 (ApicBase + APIC_SVR) & BIT8) != 0); > > + ASSERT ((MmioRead32 (ApicBase + > XAPIC_SPURIOUS_VECTOR_OFFSET) & > > BIT8) > > + != 0); > > > > return ApicBase; > > } > > @@ -98,7 +85,9 @@ InternalX86GetTimerFrequency ( { > > return > > PcdGet32(PcdFSBClock) / > > - mTimerLibLocalApicDivisor[MmioBitFieldRead32 (ApicBase + > APIC_TDCR, > > 0, 3)]; > > + mTimerLibLocalApicDivisor[ > > + MmioBitFieldRead32 (ApicBase + > > XAPIC_TIMER_DIVIDE_CONFIGURATION_OFFSET, 0, 3) > > + ]; > > } > > > > /** > > @@ -115,7 +104,7 @@ InternalX86GetTimerTick ( > > IN UINTN ApicBase > > ) > > { > > - return MmioRead32 (ApicBase + APIC_TMCCT); > > + return MmioRead32 (ApicBase + > > XAPIC_TIMER_CURRENT_COUNT_OFFSET); > > } > > > > /** > > @@ -131,7 +120,7 @@ InternalX86GetInitTimerCount ( > > IN UINTN ApicBase > > ) > > { > > - return MmioRead32 (ApicBase + APIC_TMICT); > > + return MmioRead32 (ApicBase + > XAPIC_TIMER_INIT_COUNT_OFFSET); > > } > > > > /** > > diff --git > > a/MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf > > b/MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf > > index a00ebb0eeb64..286da09db174 100644 > > --- > a/MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf > > +++ > b/MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf > > @@ -13,7 +13,7 @@ > > # Note that for IA-32 and x64, this library only supports > xAPIC mode. If > > x2APIC # support is desired, the SecPeiDxeTimerLibUefiCpu > library can be > > used. > > # > > -# Copyright (c) 2007 - 2014, Intel Corporation. All rights > reserved.<BR> > > +# Copyright (c) 2007 - 2017, Intel Corporation. All rights > > +reserved.<BR> > > # > > # This program and the accompanying materials # are > licensed and made > > available under the terms and conditions of the BSD License @@ > -48,6 +48,7 > > @@ [Sources.IPF] > > > > [Packages] > > MdePkg/MdePkg.dec > > + UefiCpuPkg/UefiCpuPkg.dec > > > > > > [LibraryClasses] > > @@ -57,6 +58,7 @@ [LibraryClasses.IA32, LibraryClasses.X64] > > PcdLib > > IoLib > > DebugLib > > + LocalApicLib > > > > [LibraryClasses.IPF] > > PalLib > > diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc index > > 010ce533d7ea..8988d1947566 100644 > > --- a/MdePkg/MdePkg.dsc > > +++ b/MdePkg/MdePkg.dsc > > @@ -35,6 +35,9 @@ [PcdsFixedAtBuild] > > [PcdsFixedAtBuild.IPF] > > > gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc0000 > 00 > > > > +[LibraryClasses] > > + > > > +LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2Ap > icLib.i > > +nf > > + > > > > ########################################################## > > ######################################### > > # > > # Components Section - list of the modules and components that > will be > > processed by compilation > > -- > > 2.12.2.windows.2 > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
