From: "Marvin Häuser" <Marvin.Haeuser@outlook.com>
To: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Gao, Liming" <liming.gao@intel.com>,
"michael.d.kinney@intel.com" <michael.d.kinney@intel.com>,
"jeff.fan@intel.com" <jeff.fan@intel.com>
Subject: Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code.
Date: Thu, 27 Jul 2017 13:49:02 +0000 [thread overview]
Message-ID: <AM4PR06MB1491DC8AF14ADB7894E1CE8080BE0@AM4PR06MB1491.eurprd06.prod.outlook.com> (raw)
In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75CAE9@shsmsx102.ccr.corp.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
next prev parent reply other threads:[~2017-07-27 13:47 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2017-07-27 15:08 ` Gao, Liming
2017-07-27 15:19 ` Marvin Häuser
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM4PR06MB1491DC8AF14ADB7894E1CE8080BE0@AM4PR06MB1491.eurprd06.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox