On 10/15/15 00:18, Michael Kinney wrote:
> The update to the LocalApicLib instances to make sure the
> Local APIC is initialize before use generates an ASSERT()
> when SOURCE_DEBUG_ENABLE is enabled for OVMF.
>
> The fix is to initialize the Local APIC Timer and mask it
> before initialing the DebugAgent.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney <[email protected]>
> ---
> OvmfPkg/Sec/SecMain.c | 10 +++++++++-
> OvmfPkg/Sec/SecMain.inf | 3 ++-
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index b7df549..2d5e51b 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -1,7 +1,7 @@
> /** @file
> Main SEC phase code. Transitions to PEI.
>
> - Copyright (c) 2008 - 2013, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2008 - 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
> @@ -28,6 +28,7 @@
> #include <Library/PeCoffGetEntryPointLib.h>
> #include <Library/PeCoffExtraActionLib.h>
> #include <Library/ExtractGuidedSectionLib.h>
> +#include <Library/LocalApicLib.h>
>
> #include <Ppi/TemporaryRamSupport.h>
>
> @@ -767,6 +768,13 @@ SecCoreStartupWithStack (
> //
> IoWrite8 (0x21, 0xff);
> IoWrite8 (0xA1, 0xff);
> +
> + //
> + // Initialize Local APIC Timer hardware and disable Local APIC Timer
> interrupts
> + // before initializing the Debug Agent and the debug timer is enabled
> + //
> + InitializeApicTimer (0, MAX_UINT32, TRUE, 5);
> + DisableApicTimerInterrupt ();
So, reviewing this took forever :) I'll make several points.
(1) This addition is consistent with the IoWrite8() calls just above it.
The context doesn't show, but there's a comment up there explaining the
IoWrite8() calls. Those do the same thing, just for the i8259 PIC.
(2) The assertion is triggered on the following path:
SecCoreStartupWithStack()
InitializeDebugAgent(DEBUG_AGENT_INIT_PREMEM_SEC)
InitializeDebugTimer()
GetApicTimerState()
ASSERT()
(3) The InitializeApicTimer() call sets the exact bit that the ASSERT()
checks.
(4) There will be a small window where the APIC timer is ticking /
armed, but the timer interrupt is disabled very soon after. There is no
library API that allows one to initialize the timer without arming it,
so this is what we have to do.
(5) Regarding the arguments in the InitializeApicTimer() call.
DivideValue=0 means "use the current divisor", MAX_UINT32 is the safest
InitCount (I guess the counting is downwards), PeriodicMode=TRUE is
irrelevant, and the timer interrupt vector (5) is also irrelevant. I
grepped the source for similar calls, and this looks to be a copy-paste
more or less, which is fine.
On the following path, the timer is also initialized (if we pass the
ASSERT()):
SecCoreStartupWithStack()
InitializeDebugAgent(DEBUG_AGENT_INIT_PREMEM_SEC)
InitializeDebugTimer()
GetApicTimerState()
InitializeApicTimer()
and in *that* call, the vector number (DEBUG_TIMER_VECTOR=32) matters,
and is sane.
Therefore,
Reviewed-by: Laszlo Ersek <[email protected]>
(6) I lightly regression tested this change, it doesn't seem to break
things.
Regression-tested-by: Laszlo Ersek <[email protected]>
I committed this patch to SVN (r18622) with the following tweaks:
- I rewrapped the commit message to 74 chars and fixed a typo in it.
- I rewrapped the new code comment at 79 chars and added a period.
- I located the exact patch in the history that had introduced the
ASSERT(), and added a reference to it in the commit message.
Thanks!
Laszlo
>
> //
> // Initialize Debug Agent to support source level debug in SEC/PEI phases
> before memory ready.
> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
> index fce99fb..2f78f3c 100644
> --- a/OvmfPkg/Sec/SecMain.inf
> +++ b/OvmfPkg/Sec/SecMain.inf
> @@ -1,7 +1,7 @@
> ## @file
> # SEC Driver
> #
> -# Copyright (c) 2008 - 2013, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2008 - 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
> @@ -55,6 +55,7 @@
> PeCoffGetEntryPointLib
> PeCoffExtraActionLib
> ExtractGuidedSectionLib
> + LocalApicLib
>
> [Ppis]
> gEfiTemporaryRamSupportPpiGuid # PPI ALWAYS_PRODUCED
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
