Jordan,
I like the idea of an internal function in these libs to determine if the
currently executing CPU supports MSR_IA32_APIC_BASE_ADDRESS.
I will break the comment line up and resend with patches inline.
Mike
-----Original Message-----
From: Justen, Jordan L
Sent: Wednesday, April 15, 2015 12:17 AM
To: Kinney, Michael D; edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [Patch 7/8] UefiCPuPkg: ApicLibs: Add Quark SoC
compatibility
UefiCPuPkg: CPu => Cpu
Really wish I didn't have to go through the extra steps to quote the
attached patch. :\ (Ie, I like inline patches. :)
> Index: Library/BaseXApicLib/BaseXApicLib.c
> ===================================================================
> --- Library/BaseXApicLib/BaseXApicLib.c (revision 17178)
> +++ Library/BaseXApicLib/BaseXApicLib.c (working copy)
> @@ -3,7 +3,7 @@
>
> This local APIC library instance supports xAPIC mode only.
>
> - Copyright (c) 2010 - 2014, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2010 - 2015, 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
> @@ -21,7 +21,10 @@
> #include <Library/LocalApicLib.h>
> #include <Library/IoLib.h>
> #include <Library/TimerLib.h>
> +#include <Library/PcdLib.h>
>
> +#define QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING 0x590
> +
> //
> // Library internal functions
> //
> @@ -38,8 +41,17 @@
> VOID
> )
> {
> - MSR_IA32_APIC_BASE ApicBaseMsr;
> -
> + UINT32 RegEax;
> + MSR_IA32_APIC_BASE ApicBaseMsr;
> +
> + AsmCpuid (1, &RegEax, NULL, NULL, NULL);
> + if ((RegEax & 0xfff) == QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING) {
This seems pretty hackish. I'm still wondering if a PCD might be worth
considering.
What about at least a helper IsQuark function?
Or, what about a new library class that allows the various modules to
read these CPU specific things? CpuHasClflush, CpuCacheLineSize, etc.
Then we could map a Dxe version of the library that only had to find
out once and could store off these bits rather than querying cpuid
each time.
> + //
> + // If CPU does not support APIC Base Address MSR, then retrieve APIC
> Base Address from PCD
Would be nice to split for < 80 columns.
-Jordan
> + //
> + return PcdGet32(PcdCpuLocalApicBaseAddress);
> + }
> +
> ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE_ADDRESS);
>
> return (UINTN)(LShiftU64 ((UINT64) ApicBaseMsr.Bits.ApicBaseHigh, 32)) +
> @@ -60,10 +72,19 @@
> IN UINTN BaseAddress
> )
> {
> - MSR_IA32_APIC_BASE ApicBaseMsr;
> + UINT32 RegEax;
> + MSR_IA32_APIC_BASE ApicBaseMsr;
>
> ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
>
> + AsmCpuid (1, &RegEax, NULL, NULL, NULL);
> + if ((RegEax & 0xfff) == QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING) {
> + //
> + // Ignore set request if the CPU does not support APIC Base Address MSR
> + //
> + return;
> + }
> +
> ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE_ADDRESS);
>
> ApicBaseMsr.Bits.ApicBaseLow = (UINT32) (BaseAddress >> 12);
> @@ -202,14 +223,21 @@
> {
> DEBUG_CODE (
> {
> - MSR_IA32_APIC_BASE ApicBaseMsr;
> + UINT32 RegEax;
> + MSR_IA32_APIC_BASE ApicBaseMsr;
>
> - ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE_ADDRESS);
> //
> - // Local APIC should have been enabled
> + // Check to see if the CPU supports the APIC Base Address MSR
> //
> - ASSERT (ApicBaseMsr.Bits.En != 0);
> - ASSERT (ApicBaseMsr.Bits.Extd == 0);
> + AsmCpuid (1, &RegEax, NULL, NULL, NULL);
> + if ((RegEax & 0xfff) != QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING) {
> + ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE_ADDRESS);
> + //
> + // Local APIC should have been enabled
> + //
> + ASSERT (ApicBaseMsr.Bits.En != 0);
> + ASSERT (ApicBaseMsr.Bits.Extd == 0);
> + }
> }
> );
> return LOCAL_APIC_MODE_XAPIC;
> Index: Library/BaseXApicLib/BaseXApicLib.inf
> ===================================================================
> --- Library/BaseXApicLib/BaseXApicLib.inf (revision 17178)
> +++ Library/BaseXApicLib/BaseXApicLib.inf (working copy)
> @@ -21,7 +21,7 @@
> MODULE_UNI_FILE = BaseXApicLib.uni
> FILE_GUID = D87CA0A8-1AC2-439b-90F8-EF4A2AC88DAF
> MODULE_TYPE = BASE
> - VERSION_STRING = 1.0
> + VERSION_STRING = 1.1
> LIBRARY_CLASS = LocalApicLib
>
> #
> @@ -42,6 +42,8 @@
> DebugLib
> TimerLib
> IoLib
> + PcdLib
>
> [Pcd]
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress ##
> SOMETIMES_CONSUMES
> Index: Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> ===================================================================
> --- Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c (revision 17178)
> +++ Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c (working copy)
> @@ -22,7 +22,10 @@
> #include <Library/LocalApicLib.h>
> #include <Library/IoLib.h>
> #include <Library/TimerLib.h>
> +#include <Library/PcdLib.h>
>
> +#define QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING 0x590
> +
> //
> // Library internal functions
> //
> @@ -39,8 +42,17 @@
> VOID
> )
> {
> - MSR_IA32_APIC_BASE ApicBaseMsr;
> -
> + UINT32 RegEax;
> + MSR_IA32_APIC_BASE ApicBaseMsr;
> +
> + AsmCpuid (1, &RegEax, NULL, NULL, NULL);
> + if ((RegEax & 0xfff) == QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING) {
> + //
> + // If CPU does not support APIC Base Address MSR, then retrieve APIC
> Base Address from PCD
> + //
> + return PcdGet32(PcdCpuLocalApicBaseAddress);
> + }
> +
> ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE_ADDRESS);
>
> return (UINTN)(LShiftU64 ((UINT64) ApicBaseMsr.Bits.ApicBaseHigh, 32)) +
> @@ -61,10 +73,19 @@
> IN UINTN BaseAddress
> )
> {
> - MSR_IA32_APIC_BASE ApicBaseMsr;
> + UINT32 RegEax;
> + MSR_IA32_APIC_BASE ApicBaseMsr;
>
> ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
>
> + AsmCpuid (1, &RegEax, NULL, NULL, NULL);
> + if ((RegEax & 0xfff) == QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING) {
> + //
> + // Ignore set request of the CPU does not support APIC Base Address MSR
> + //
> + return;
> + }
> +
> ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE_ADDRESS);
>
> ApicBaseMsr.Bits.ApicBaseLow = (UINT32) (BaseAddress >> 12);
> @@ -257,8 +278,17 @@
> VOID
> )
> {
> - MSR_IA32_APIC_BASE ApicBaseMsr;
> + UINT32 RegEax;
> + MSR_IA32_APIC_BASE ApicBaseMsr;
>
> + AsmCpuid (1, &RegEax, NULL, NULL, NULL);
> + if ((RegEax & 0xfff) == QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING) {
> + //
> + // If CPU does not support APIC Base Address MSR, then return XAPIC mode
> + //
> + return LOCAL_APIC_MODE_XAPIC;
> + }
> +
> ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE_ADDRESS);
> //
> // Local APIC should have been enabled
> @@ -288,9 +318,18 @@
> IN UINTN ApicMode
> )
> {
> - UINTN CurrentMode;
> - MSR_IA32_APIC_BASE ApicBaseMsr;
> + UINT32 RegEax;
> + UINTN CurrentMode;
> + MSR_IA32_APIC_BASE ApicBaseMsr;
>
> + AsmCpuid (1, &RegEax, NULL, NULL, NULL);
> + if ((RegEax & 0xfff) == QUARK_SOC_CPUID_FAMILY_MODEL_STEPPING) {
> + //
> + // Ignore set request if the CPU does not support APIC Base Address MSR
> + //
> + return;
> + }
> +
> CurrentMode = GetApicMode ();
> if (CurrentMode == LOCAL_APIC_MODE_XAPIC) {
> switch (ApicMode) {
> Index: Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> ===================================================================
> --- Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf (revision 17178)
> +++ Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf (working copy)
> @@ -21,7 +21,7 @@
> MODULE_UNI_FILE = BaseXApicX2ApicLib.uni
> FILE_GUID = 967B6E05-F10D-4c10-8BF7-365291CA143F
> MODULE_TYPE = BASE
> - VERSION_STRING = 1.0
> + VERSION_STRING = 1.1
> LIBRARY_CLASS = LocalApicLib
>
> #
> @@ -42,7 +42,8 @@
> DebugLib
> TimerLib
> IoLib
> + PcdLib
>
> [Pcd]
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds ##
> SOMETIMES_CONSUMES
> -
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress ##
> SOMETIMES_CONSUMES
On 2015-04-14 09:49:50, Kinney, Michael D wrote:
> Please review the attached patch that adds Quark SoC compatibility to the
> APIC Libraries in the UefiCpuPkg.
>
>
>
> Some EDK II libraries assume capabilities that are not available on Quark
> SoC. With these changes, applications and drivers built for IA32 are
> compatible with Quark SoC.
>
> These patches do not modify X64 specific sources or SSE/SSE2 specific
> sources.
>
>
>
> Change Summary:
>
> ===============
>
> 1) Remove use of CMOVx from IA32 assembly. This matches compiler flags for
> all supported C compilers.
>
> 2) Use CPUID Leaf 01 to detect CPU capabilities for CLFLUSH, Cache Line
> Size, FXSAVE/FXTSTOR, CR4.DE, and CR4.OSFXR
>
> 3) Use CPUID Family/Model/Stepping to detect support for
> MSR_IA32_APIC_BASE_ADDRESS. Quark SoC does not support this MSR.
>
> 4) Add stride PCD to MdeModulePkg to support 16550 UARTs with a register
> stride that is not 1 byte. Quark SoC uses a stride of 4 bytes.
>
>
>
> Contributed-under: TianoCore Contribution Agreement 1.0
>
> Signed-off by: Michael Kinney <michael.d.kin...@intel.com>
>
>
------------------------------------------------------------------------------
BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT
Develop your own process in accordance with the BPMN 2 standard
Learn Process modeling best practices with Bonita BPM through live exercises
http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_
source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
