Re: [PATCH] MdePkg/SecPeiDxeTimerLibCpu: Consume UefiCpuPkg LAPIC code.

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

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