* [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. @ 2017-07-23 10:12 Marvin Häuser 2017-07-23 10:19 ` Marvin Häuser 0 siblings, 1 reply; 10+ messages in thread From: Marvin Häuser @ 2017-07-23 10:12 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: michael.d.kinney@intel.com, liming.gao@intel.com 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 <Marvin.Haeuser@outlook.com> --- 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|0x0ffffc000000 +[LibraryClasses] + LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf + ################################################################################################### # # Components Section - list of the modules and components that will be processed by compilation -- 2.12.2.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-23 10:12 [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code Marvin Häuser @ 2017-07-23 10:19 ` Marvin Häuser 2017-07-24 16:39 ` Kinney, Michael D 0 siblings, 1 reply; 10+ messages in thread From: Marvin Häuser @ 2017-07-23 10:19 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: michael.d.kinney@intel.com, liming.gao@intel.com 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:edk2-devel-bounces@lists.01.org] On Behalf Of > Marvin Häuser > Sent: Sunday, July 23, 2017 12:12 PM > To: edk2-devel@lists.01.org > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > 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 <Marvin.Haeuser@outlook.com> > --- > 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|0x0ffffc000000 > > +[LibraryClasses] > + > +LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.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 > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-23 10:19 ` Marvin Häuser @ 2017-07-24 16:39 ` Kinney, Michael D 2017-07-24 16:57 ` Marvin Häuser 0 siblings, 1 reply; 10+ messages in thread From: Kinney, Michael D @ 2017-07-24 16:39 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org, Kinney, Michael D Cc: Gao, Liming 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:Marvin.Haeuser@outlook.com] > Sent: Sunday, July 23, 2017 3:19 AM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > <liming.gao@intel.com> > 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:edk2-devel-bounces@lists.01.org] On > Behalf Of > > Marvin Häuser > > Sent: Sunday, July 23, 2017 12:12 PM > > To: edk2-devel@lists.01.org > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > 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 <Marvin.Haeuser@outlook.com> > > --- > > 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 > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-24 16:39 ` Kinney, Michael D @ 2017-07-24 16:57 ` Marvin Häuser 2017-07-26 2:28 ` Fan, Jeff 0 siblings, 1 reply; 10+ messages in thread From: Marvin Häuser @ 2017-07-24 16:57 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: Kinney, Michael D Hi Michael, Sorry, I did not notice that UefiCpuPkg already has a similiar library. Could you please answer my question regarding the mask value which I considered incorrect nevertheless? I am getting compile-time errors as the mask promotes the value to UINT64, which is, on 32-bit platforms, implicitely casted to UINT32 (UINTN), resulting in the appropiate data loss warning. Thanks for your answer! Regards, Marvin. > -----Original Message----- > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] > Sent: Monday, July 24, 2017 6:40 PM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > Cc: Gao, Liming <liming.gao@intel.com> > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg > LAPIC code. > > 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:Marvin.Haeuser@outlook.com] > > Sent: Sunday, July 23, 2017 3:19 AM > > To: edk2-devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > <liming.gao@intel.com> > > 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:edk2-devel-bounces@lists.01.org] On > > Behalf Of > > > Marvin Häuser > > > Sent: Sunday, July 23, 2017 12:12 PM > > > To: edk2-devel@lists.01.org > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > 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 <Marvin.Haeuser@outlook.com> > > > --- > > > 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 > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-24 16:57 ` Marvin Häuser @ 2017-07-26 2:28 ` Fan, Jeff 2017-07-26 8:33 ` Marvin Häuser 0 siblings, 1 reply; 10+ messages in thread From: Fan, Jeff @ 2017-07-26 2:28 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org; +Cc: Kinney, Michael D Marvin, Could the following updating fix your issue? ApicBase = MsrValue & ~0xfff; Thanks! Jeff -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin Häuser Sent: Tuesday, July 25, 2017 12:57 AM To: edk2-devel@lists.01.org Cc: Kinney, Michael D Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. Hi Michael, Sorry, I did not notice that UefiCpuPkg already has a similiar library. Could you please answer my question regarding the mask value which I considered incorrect nevertheless? I am getting compile-time errors as the mask promotes the value to UINT64, which is, on 32-bit platforms, implicitely casted to UINT32 (UINTN), resulting in the appropiate data loss warning. Thanks for your answer! Regards, Marvin. > -----Original Message----- > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] > Sent: Monday, July 24, 2017 6:40 PM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > Cc: Gao, Liming <liming.gao@intel.com> > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg > LAPIC code. > > 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:Marvin.Haeuser@outlook.com] > > Sent: Sunday, July 23, 2017 3:19 AM > > To: edk2-devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > <liming.gao@intel.com> > > 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:edk2-devel-bounces@lists.01.org] On > > Behalf Of > > > Marvin Häuser > > > Sent: Sunday, July 23, 2017 12:12 PM > > > To: edk2-devel@lists.01.org > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > 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 <Marvin.Haeuser@outlook.com> > > > --- > > > 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 > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-26 2:28 ` Fan, Jeff @ 2017-07-26 8:33 ` Marvin Häuser 2017-07-27 13:19 ` Gao, Liming 0 siblings, 1 reply; 10+ messages in thread From: Marvin Häuser @ 2017-07-26 8:33 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: Fan, Jeff, Kinney, Michael D Hey Jeff, That would definitely fix the compilation issue. Do I see it right that for 32-bit platforms, it's just ApicBase and for 64-bit platforms, it's ApicBase and ApicBaseHi? If yes, maybe ~(UINTN)0xFFF should be used to play safe? Thanks, Marvin. > -----Original Message----- > From: Fan, Jeff [mailto:jeff.fan@intel.com] > Sent: Wednesday, July 26, 2017 4:28 AM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg > LAPIC code. > > Marvin, > > Could the following updating fix your issue? > > ApicBase = MsrValue & ~0xfff; > > Thanks! > Jeff > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Marvin Häuser > Sent: Tuesday, July 25, 2017 12:57 AM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > UefiCpuPkg LAPIC code. > > Hi Michael, > > Sorry, I did not notice that UefiCpuPkg already has a similiar library. > Could you please answer my question regarding the mask value which I > considered incorrect nevertheless? > I am getting compile-time errors as the mask promotes the value to UINT64, > which is, on 32-bit platforms, implicitely casted to UINT32 (UINTN), resulting > in the appropiate data loss warning. > > Thanks for your answer! > > Regards, > Marvin. > > > -----Original Message----- > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] > > Sent: Monday, July 24, 2017 6:40 PM > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > > Cc: Gao, Liming <liming.gao@intel.com> > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > UefiCpuPkg > > LAPIC code. > > > > 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:Marvin.Haeuser@outlook.com] > > > Sent: Sunday, July 23, 2017 3:19 AM > > > To: edk2-devel@lists.01.org > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > > <liming.gao@intel.com> > > > 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:edk2-devel-bounces@lists.01.org] On > > > Behalf Of > > > > Marvin Häuser > > > > Sent: Sunday, July 23, 2017 12:12 PM > > > > To: edk2-devel@lists.01.org > > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > > 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 <Marvin.Haeuser@outlook.com> > > > > --- > > > > 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 > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-26 8:33 ` Marvin Häuser @ 2017-07-27 13:19 ` Gao, Liming 2017-07-27 13:49 ` Marvin Häuser 0 siblings, 1 reply; 10+ messages in thread From: Gao, Liming @ 2017-07-27 13:19 UTC (permalink / raw) To: Marvin.Haeuser@outlook.com, edk2-devel@lists.01.org Cc: Kinney, Michael D, Fan, Jeff Marvin: Do you meet with the compiler issue on the old VS tool chain? Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin H?user > Sent: Wednesday, July 26, 2017 4:34 PM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com> > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. > > Hey Jeff, > > That would definitely fix the compilation issue. > Do I see it right that for 32-bit platforms, it's just ApicBase and for 64-bit platforms, it's ApicBase and ApicBaseHi? > If yes, maybe ~(UINTN)0xFFF should be used to play safe? > > Thanks, > Marvin. > > > -----Original Message----- > > From: Fan, Jeff [mailto:jeff.fan@intel.com] > > Sent: Wednesday, July 26, 2017 4:28 AM > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg > > LAPIC code. > > > > Marvin, > > > > Could the following updating fix your issue? > > > > ApicBase = MsrValue & ~0xfff; > > > > Thanks! > > Jeff > > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Marvin Häuser > > Sent: Tuesday, July 25, 2017 12:57 AM > > To: edk2-devel@lists.01.org > > Cc: Kinney, Michael D > > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > UefiCpuPkg LAPIC code. > > > > Hi Michael, > > > > Sorry, I did not notice that UefiCpuPkg already has a similiar library. > > Could you please answer my question regarding the mask value which I > > considered incorrect nevertheless? > > I am getting compile-time errors as the mask promotes the value to UINT64, > > which is, on 32-bit platforms, implicitely casted to UINT32 (UINTN), resulting > > in the appropiate data loss warning. > > > > Thanks for your answer! > > > > Regards, > > Marvin. > > > > > -----Original Message----- > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] > > > Sent: Monday, July 24, 2017 6:40 PM > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > > > Cc: Gao, Liming <liming.gao@intel.com> > > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > UefiCpuPkg > > > LAPIC code. > > > > > > 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:Marvin.Haeuser@outlook.com] > > > > Sent: Sunday, July 23, 2017 3:19 AM > > > > To: edk2-devel@lists.01.org > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > > > <liming.gao@intel.com> > > > > 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:edk2-devel-bounces@lists.01.org] On > > > > Behalf Of > > > > > Marvin Häuser > > > > > Sent: Sunday, July 23, 2017 12:12 PM > > > > > To: edk2-devel@lists.01.org > > > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > > > 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 <Marvin.Haeuser@outlook.com> > > > > > --- > > > > > 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 > > > > > edk2-devel@lists.01.org > > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-27 13:19 ` Gao, Liming @ 2017-07-27 13:49 ` Marvin Häuser 2017-07-27 15:08 ` Gao, Liming 0 siblings, 1 reply; 10+ messages in thread From: Marvin Häuser @ 2017-07-27 13:49 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: Gao, Liming, michael.d.kinney@intel.com, jeff.fan@intel.com Hey Liming, Old and new because sizeof (UINTN) is what matters. Thanks, Marvin. > -----Original Message----- > From: Gao, Liming [mailto:liming.gao@intel.com] > Sent: Thursday, July 27, 2017 3:19 PM > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > <jeff.fan@intel.com> > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg > LAPIC code. > > Marvin: > Do you meet with the compiler issue on the old VS tool chain? > > Thanks > Liming > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Marvin H?user > > Sent: Wednesday, July 26, 2017 4:34 PM > > To: edk2-devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > > <jeff.fan@intel.com> > > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > UefiCpuPkg LAPIC code. > > > > Hey Jeff, > > > > That would definitely fix the compilation issue. > > Do I see it right that for 32-bit platforms, it's just ApicBase and for 64-bit > platforms, it's ApicBase and ApicBaseHi? > > If yes, maybe ~(UINTN)0xFFF should be used to play safe? > > > > Thanks, > > Marvin. > > > > > -----Original Message----- > > > From: Fan, Jeff [mailto:jeff.fan@intel.com] > > > Sent: Wednesday, July 26, 2017 4:28 AM > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > devel@lists.01.org > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > UefiCpuPkg > > > LAPIC code. > > > > > > Marvin, > > > > > > Could the following updating fix your issue? > > > > > > ApicBase = MsrValue & ~0xfff; > > > > > > Thanks! > > > Jeff > > > > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > > > Of Marvin Häuser > > > Sent: Tuesday, July 25, 2017 12:57 AM > > > To: edk2-devel@lists.01.org > > > Cc: Kinney, Michael D > > > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > > UefiCpuPkg LAPIC code. > > > > > > Hi Michael, > > > > > > Sorry, I did not notice that UefiCpuPkg already has a similiar library. > > > Could you please answer my question regarding the mask value which I > > > considered incorrect nevertheless? > > > I am getting compile-time errors as the mask promotes the value to > > > UINT64, which is, on 32-bit platforms, implicitely casted to UINT32 > > > (UINTN), resulting in the appropiate data loss warning. > > > > > > Thanks for your answer! > > > > > > Regards, > > > Marvin. > > > > > > > -----Original Message----- > > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] > > > > Sent: Monday, July 24, 2017 6:40 PM > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > > > > Cc: Gao, Liming <liming.gao@intel.com> > > > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > > UefiCpuPkg > > > > LAPIC code. > > > > > > > > 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:Marvin.Haeuser@outlook.com] > > > > > Sent: Sunday, July 23, 2017 3:19 AM > > > > > To: edk2-devel@lists.01.org > > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > > > > <liming.gao@intel.com> > > > > > 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:edk2-devel-bounces@lists.01.org] On > > > > > Behalf Of > > > > > > Marvin Häuser > > > > > > Sent: Sunday, July 23, 2017 12:12 PM > > > > > > To: edk2-devel@lists.01.org > > > > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > > > > 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 <Marvin.Haeuser@outlook.com> > > > > > > --- > > > > > > 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|0x0ffffc000 > > > > > 0 > > > > > 00 > > > > > > > > > > > > +[LibraryClasses] > > > > > > + > > > > > > > > > > > > +LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2A > > > > > +LocalApicLib|p > > > > > 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 > > > > > > edk2-devel@lists.01.org > > > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > > _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-27 13:49 ` Marvin Häuser @ 2017-07-27 15:08 ` Gao, Liming 2017-07-27 15:19 ` Marvin Häuser 0 siblings, 1 reply; 10+ messages in thread From: Gao, Liming @ 2017-07-27 15:08 UTC (permalink / raw) To: Marvin.Haeuser@outlook.com, edk2-devel@lists.01.org Cc: Kinney, Michael D, Fan, Jeff Marvin: I build this library on IA32 with VS2013x86 and VS2015x86. They can pass build. Could you let me know how to reproduce the compiler issue? Then, we can further provide the patch to fix it. Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin H?user > Sent: Thursday, July 27, 2017 9:49 PM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com>; Gao, Liming <liming.gao@intel.com> > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. > > Hey Liming, > > Old and new because sizeof (UINTN) is what matters. > > Thanks, > Marvin. > > > -----Original Message----- > > From: Gao, Liming [mailto:liming.gao@intel.com] > > Sent: Thursday, July 27, 2017 3:19 PM > > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > > <jeff.fan@intel.com> > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg > > LAPIC code. > > > > Marvin: > > Do you meet with the compiler issue on the old VS tool chain? > > > > Thanks > > Liming > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > > Marvin H?user > > > Sent: Wednesday, July 26, 2017 4:34 PM > > > To: edk2-devel@lists.01.org > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > > > <jeff.fan@intel.com> > > > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > UefiCpuPkg LAPIC code. > > > > > > Hey Jeff, > > > > > > That would definitely fix the compilation issue. > > > Do I see it right that for 32-bit platforms, it's just ApicBase and for 64-bit > > platforms, it's ApicBase and ApicBaseHi? > > > If yes, maybe ~(UINTN)0xFFF should be used to play safe? > > > > > > Thanks, > > > Marvin. > > > > > > > -----Original Message----- > > > > From: Fan, Jeff [mailto:jeff.fan@intel.com] > > > > Sent: Wednesday, July 26, 2017 4:28 AM > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > > devel@lists.01.org > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > > > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > UefiCpuPkg > > > > LAPIC code. > > > > > > > > Marvin, > > > > > > > > Could the following updating fix your issue? > > > > > > > > ApicBase = MsrValue & ~0xfff; > > > > > > > > Thanks! > > > > Jeff > > > > > > > > -----Original Message----- > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > > > > Of Marvin Häuser > > > > Sent: Tuesday, July 25, 2017 12:57 AM > > > > To: edk2-devel@lists.01.org > > > > Cc: Kinney, Michael D > > > > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > > > UefiCpuPkg LAPIC code. > > > > > > > > Hi Michael, > > > > > > > > Sorry, I did not notice that UefiCpuPkg already has a similiar library. > > > > Could you please answer my question regarding the mask value which I > > > > considered incorrect nevertheless? > > > > I am getting compile-time errors as the mask promotes the value to > > > > UINT64, which is, on 32-bit platforms, implicitely casted to UINT32 > > > > (UINTN), resulting in the appropiate data loss warning. > > > > > > > > Thanks for your answer! > > > > > > > > Regards, > > > > Marvin. > > > > > > > > > -----Original Message----- > > > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] > > > > > Sent: Monday, July 24, 2017 6:40 PM > > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > > > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > > > > > Cc: Gao, Liming <liming.gao@intel.com> > > > > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > > > UefiCpuPkg > > > > > LAPIC code. > > > > > > > > > > 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:Marvin.Haeuser@outlook.com] > > > > > > Sent: Sunday, July 23, 2017 3:19 AM > > > > > > To: edk2-devel@lists.01.org > > > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > > > > > <liming.gao@intel.com> > > > > > > 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:edk2-devel-bounces@lists.01.org] On > > > > > > Behalf Of > > > > > > > Marvin Häuser > > > > > > > Sent: Sunday, July 23, 2017 12:12 PM > > > > > > > To: edk2-devel@lists.01.org > > > > > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > > > > > 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 <Marvin.Haeuser@outlook.com> > > > > > > > --- > > > > > > > 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|0x0ffffc000 > > > > > > 0 > > > > > > 00 > > > > > > > > > > > > > > +[LibraryClasses] > > > > > > > + > > > > > > > > > > > > > > > +LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2A > > > > > > +LocalApicLib|p > > > > > > 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 > > > > > > > edk2-devel@lists.01.org > > > > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > > _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code. 2017-07-27 15:08 ` Gao, Liming @ 2017-07-27 15:19 ` Marvin Häuser 0 siblings, 0 replies; 10+ messages in thread From: Marvin Häuser @ 2017-07-27 15:19 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: Gao, Liming, michael.d.kinney@intel.com, jeff.fan@intel.com Hey Liming, Thanks for your answer! Sorry, it seems like I indeed had a typo in the build command and accidentially built with 2005 instead of 2015. It builds fine with VS2015, though I still think the mask value is incorrect. After all it includes the first nibble of ApicBaseHi on 64-bit platforms, rather than the entire value. Regards, Marvin. > -----Original Message----- > From: Gao, Liming [mailto:liming.gao@intel.com] > Sent: Thursday, July 27, 2017 5:09 PM > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > <jeff.fan@intel.com> > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg > LAPIC code. > > Marvin: > I build this library on IA32 with VS2013x86 and VS2015x86. They can pass > build. Could you let me know how to reproduce the compiler issue? Then, > we can further provide the patch to fix it. > > Thanks > Liming > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Marvin H?user > > Sent: Thursday, July 27, 2017 9:49 PM > > To: edk2-devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > > <jeff.fan@intel.com>; Gao, Liming <liming.gao@intel.com> > > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > UefiCpuPkg LAPIC code. > > > > Hey Liming, > > > > Old and new because sizeof (UINTN) is what matters. > > > > Thanks, > > Marvin. > > > > > -----Original Message----- > > > From: Gao, Liming [mailto:liming.gao@intel.com] > > > Sent: Thursday, July 27, 2017 3:19 PM > > > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > > > <jeff.fan@intel.com> > > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > UefiCpuPkg > > > LAPIC code. > > > > > > Marvin: > > > Do you meet with the compiler issue on the old VS tool chain? > > > > > > Thanks > > > Liming > > > > -----Original Message----- > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On > > > > Behalf Of Marvin H?user > > > > Sent: Wednesday, July 26, 2017 4:34 PM > > > > To: edk2-devel@lists.01.org > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff > > > > <jeff.fan@intel.com> > > > > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > > UefiCpuPkg LAPIC code. > > > > > > > > Hey Jeff, > > > > > > > > That would definitely fix the compilation issue. > > > > Do I see it right that for 32-bit platforms, it's just ApicBase > > > > and for 64-bit > > > platforms, it's ApicBase and ApicBaseHi? > > > > If yes, maybe ~(UINTN)0xFFF should be used to play safe? > > > > > > > > Thanks, > > > > Marvin. > > > > > > > > > -----Original Message----- > > > > > From: Fan, Jeff [mailto:jeff.fan@intel.com] > > > > > Sent: Wednesday, July 26, 2017 4:28 AM > > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > > > devel@lists.01.org > > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > > > > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > > UefiCpuPkg > > > > > LAPIC code. > > > > > > > > > > Marvin, > > > > > > > > > > Could the following updating fix your issue? > > > > > > > > > > ApicBase = MsrValue & ~0xfff; > > > > > > > > > > Thanks! > > > > > Jeff > > > > > > > > > > -----Original Message----- > > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On > > > > > Behalf Of Marvin Häuser > > > > > Sent: Tuesday, July 25, 2017 12:57 AM > > > > > To: edk2-devel@lists.01.org > > > > > Cc: Kinney, Michael D > > > > > Subject: Re: [edk2] [PATCH] MdePkg/SecPeiDxeTimerLibCpu: > Consume > > > > > UefiCpuPkg LAPIC code. > > > > > > > > > > Hi Michael, > > > > > > > > > > Sorry, I did not notice that UefiCpuPkg already has a similiar library. > > > > > Could you please answer my question regarding the mask value > > > > > which I considered incorrect nevertheless? > > > > > I am getting compile-time errors as the mask promotes the value > > > > > to UINT64, which is, on 32-bit platforms, implicitely casted to > > > > > UINT32 (UINTN), resulting in the appropiate data loss warning. > > > > > > > > > > Thanks for your answer! > > > > > > > > > > Regards, > > > > > Marvin. > > > > > > > > > > > -----Original Message----- > > > > > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] > > > > > > Sent: Monday, July 24, 2017 6:40 PM > > > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > > > > devel@lists.01.org; Kinney, Michael D > > > > > > <michael.d.kinney@intel.com> > > > > > > Cc: Gao, Liming <liming.gao@intel.com> > > > > > > Subject: RE: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume > > > > > UefiCpuPkg > > > > > > LAPIC code. > > > > > > > > > > > > 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:Marvin.Haeuser@outlook.com] > > > > > > > Sent: Sunday, July 23, 2017 3:19 AM > > > > > > > To: edk2-devel@lists.01.org > > > > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, > > > > > > > Liming <liming.gao@intel.com> > > > > > > > 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:edk2-devel-bounces@lists.01.org] > > > > > > > > On > > > > > > > Behalf Of > > > > > > > > Marvin Häuser > > > > > > > > Sent: Sunday, July 23, 2017 12:12 PM > > > > > > > > To: edk2-devel@lists.01.org > > > > > > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > > > > > > 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 > <Marvin.Haeuser@outlook.com> > > > > > > > > --- > > > > > > > > MdePkg/Library/SecPeiDxeTimerLibCpu/X86TimerLib.c | > > > > > > > 35 +++++++-- > > > > > > > > ----------- > > > > > > > > > > > > > > > > > MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.i > > > > > > > > nf > > > > > > > > | > > > > > > > 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.i > > > > > > > nf > > > > > > > > +++ > > > > > > > > b/MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.i > > > > > > > nf > > > > > > > > @@ -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|0x0ffffc000 > > > > > > > 0 > > > > > > > 00 > > > > > > > > > > > > > > > > +[LibraryClasses] > > > > > > > > + > > > > > > > > > > > > > > > > > > +LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2A > > > > > > > +LocalApicLib|p > > > > > > > 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 > > > > > > > > edk2-devel@lists.01.org > > > > > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > > _______________________________________________ > > > > > edk2-devel mailing list > > > > > edk2-devel@lists.01.org > > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-07-27 15:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-23 10:12 [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code Marvin Häuser 2017-07-23 10:19 ` Marvin Häuser 2017-07-24 16:39 ` Kinney, Michael D 2017-07-24 16:57 ` Marvin Häuser 2017-07-26 2:28 ` Fan, Jeff 2017-07-26 8:33 ` Marvin Häuser 2017-07-27 13:19 ` Gao, Liming 2017-07-27 13:49 ` Marvin Häuser 2017-07-27 15:08 ` Gao, Liming 2017-07-27 15:19 ` Marvin Häuser
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox