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

["Marvin Häuser" <Marvin.Haeuser@outlook.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