On 11/22/19 6:52 AM, Laszlo Ersek wrote:
On 11/21/19 21:46, Tom Lendacky wrote:
On 11/21/19 6:06 AM, Laszlo Ersek wrote:
On 11/20/19 21:06, Lendacky, Thomas wrote:
BZ:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C86943210aff44681d5b908d76f4acf49%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100239406633853&sdata=8APpMBall%2F2urZh6V3kpuWpsBjOh8GVqhWGNBjL%2B30U%3D&reserved=0
An SEV-ES guest will generate a #VC exception when it encounters a
non-automatic exit (NAE) event. It is expected that the #VC exception
handler will communicate with the hypervisor using the GHCB to handle
the NAE event.
NAE events can occur during the Sec phase, so initialize exception
handling early in the OVMF Sec support.
Cc: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
---
OvmfPkg/Sec/SecMain.inf | 1 +
OvmfPkg/Sec/SecMain.c | 29 ++++++++++++++++-------------
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
index 63ba4cb555fb..7f53845f5436 100644
--- a/OvmfPkg/Sec/SecMain.inf
+++ b/OvmfPkg/Sec/SecMain.inf
@@ -50,6 +50,7 @@ [LibraryClasses]
PeCoffExtraActionLib
ExtractGuidedSectionLib
LocalApicLib
+ CpuExceptionHandlerLib
[Ppis]
gEfiTemporaryRamSupportPpiGuid # PPI ALWAYS_PRODUCED
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index bae9764577f0..db319030ee58 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -24,6 +24,7 @@
#include <Library/PeCoffExtraActionLib.h>
#include <Library/ExtractGuidedSectionLib.h>
#include <Library/LocalApicLib.h>
+#include <Library/CpuExceptionHandlerLib.h>
#include <Ppi/TemporaryRamSupport.h>
@@ -737,6 +738,21 @@ SecCoreStartupWithStack (
Table[Index] = 0;
}
+ //
+ // Initialize IDT
+ //
+ IdtTableInStack.PeiService = NULL;
+ for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
+ CopyMem (&IdtTableInStack.IdtTable[Index], &mIdtEntryTemplate, sizeof
(mIdtEntryTemplate));
+ }
+
+ IdtDescriptor.Base = (UINTN)&IdtTableInStack.IdtTable;
+ IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1);
+
+ AsmWriteIdtr (&IdtDescriptor);
+
+ InitializeCpuExceptionHandlers (NULL);
+
ProcessLibraryConstructorList (NULL, NULL);
DEBUG ((EFI_D_INFO,
(1) The problem here is that we call multiple library APIs before
calling ProcessLibraryConstructorList() -- namely CopyMem(),
AsmWriteIdtr(), and InitializeCpuExceptionHandlers().
We can reduce this exposure a bit and replace the CopyMem() call with
something similar to the loop above it. I could also use assembler
code directly in here to load the IDTR.
That would leave just InitializeCpuExceptionHandlers(). Is there
something that can added so as to warn when a library has a
CONSTRUCTOR added to/part of the definition?
(See also the "SetMem" reference in the leading context, in the
source file -- it is not quoted in this patch.)
Thus, would it be possible to move all the "+" lines, quoted above,
just below the ProcessLibraryConstructorList() call?
Unfortunately, I can't. The invocation of
ProcessLibraryConstructorList() results in #VC exceptions and so the
exception handler needs to be in place before invoking
ProcessLibraryConstructorList(). It looks like there are some
SerialPort I/O writes to initialize the serial port and some PCI I/O
reads and writes from AcpiTimerLibConstructor() in
OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c.
I have to accept what you're saying, but this makes the code quite
brittle. It's a tenet that we don't call library APIs before the library
constructor had a chance to initialize whatever memory or hardware the
library APIs rely on.
So, in this case,
(a) please add a comment above this block that we're making an exception
with CopyMem(), AsmWriteIdtr() and InitializeCpuExceptionHandlers(),
Can do.
(b) I'd still like to see this pre-constructor logic restricted to
SEV-ES. (More on that below.)
Yup, propagating the SEV-ES status from the ResetVector seems to be the
way to go.
So something like:
if (SevEs) {
//
// We have to initialize the IDT and set up exception handlers here,
// i.e. before calling library constructors, because those library
// constructors may access hardware such that #VC exceptions are
// triggered.
//
// Due to this code executing before library constructors, *all*
// library API calls are theoretically interface contract
// violations. However, because we are in SEC (executing in flash),
// those constructors cannot write variables with static storage
// duration anyway. Furthermore, we call a small, restricted set of
// APIs, such as CopyMem(), AsmWriteIdtr(),
// InitializeCpuExceptionHandlers(), where we require that the
// underlying library instance not trigger any #VC exceptions.
//
InitIdt ();
InitExceptionHandlers ();
}
ProcessLibraryConstructorList ();
if (!SevEs) {
InitIdt ();
}
This makes me feel safer because:
- we're explicit about the principles we (have to) break,
- even our limited assumptions are restricted to SEV-ES.
(2) If possible I'd like to restrict the
InitializeCpuExceptionHandlers() call to SevEsIsEnabled().
(Unless you're implying that InitializeCpuExceptionHandlers() is
useful even without SEV-ES -- but then the commit message should be
reworded accordingly.)
It does give you earlier exception handling and displays the exception
information should an exception occur during SEC.
But, it might require some tricks to somehow communicate from the
ResetVector code to the SecMain code that SEV-ES is enabled. This is
because you need to do a CPUID instruction to determine if SEV-ES is
enabled, which will generate a #VC, which requires an exception
handler...
So even *checking* whether SEV-ES is enabled requires a #VC handler to
be set up. Thanks for the reminder. How about this idea: in
[edk2-devel] [RFC PATCH v3 37/43]
OvmfPkg: Reserve a page in memory for the SEV-ES AP reset vector
you are carving out a page at PcdSevEsResetRipBase -- only in
"OvmfPkgX64.fdf". And, a very small (leading) stretch of that page is
used as the SEV_ES_AP_JMP_FAR structure.
Now, could we implement the following?
(1) Append a UINT8 ("BOOLEAN") field to the SEV_ES_AP_JMP_FAR structure
(and possibly rename the structure),
(2) in the OvmfPkgX64 reset vector, where you determine SEV-ES anyway,
explicitly set this field to zero if SEV-ES is disabled, and set it to
one, if SEV-ES is enabled,
(3) In OvmfPkg/Sec, introduce a new (local) header file declaring the
function SevEsIsEnabled(),
(4) Provide two C-language implementations (under the Ia32 and X64
directories): in the 32-bit version, return constant FALSE; in the
64-bit version, return the value of the new field. Something like:
return ((SEV_ES_AP_JMP_FAR *)FixedPcdGet32
(PcdSevEsResetRipBase))->SevEsEnabled;
FixedPcdGet32() is explicitly safe to use without library constructors
having run.
Does this look viable? (It might require you to reshuffle patch 37 vs.
patch 30.)
I think this does. Since this is SEC and the reset vector page isn't
needed until PEI and later we could even just use the first byte (make a
union with an SEC usage field) and make this even simpler. Then we don't
have to worry about positioning it. Let me work on that and see where I
get. Anything after the #VC is established would use the current method
of determine SEV-ES status.
Thanks!
Tom
Thanks!
Laszlo
Thanks,
Tom
If you agree to that restriction, then I further suggest reordering this
patch against the next one:
[edk2-devel] [RFC PATCH v3 31/43]
OvmfPkg/Sec: Enable cache early to speed up booting
Namely, if you put that patch first, then in the present patch you can
extend the already existing SevEsIsEnabled()-dependent scope, with a
call to InitializeCpuExceptionHandlers().
The end result would be something like:
ProcessLibraryConstructorList();
//
// Initialize IDT
// ...
//
if (SevEsIsEnabled()) {
//
// non-automatic exit events (NAE) can occur during SEC ...
//
InitializeCpuExceptionHandlers (NULL);
//
// Under SEV-ES, the hypervisor can't modify CR0 ...
//
AsmEnableCache ();
}
What's your opinion?
Thanks!
Laszlo
@@ -751,19 +767,6 @@ SecCoreStartupWithStack (
//
InitializeFloatingPointUnits ();
- //
- // Initialize IDT
- //
- IdtTableInStack.PeiService = NULL;
- for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
- CopyMem (&IdtTableInStack.IdtTable[Index], &mIdtEntryTemplate, sizeof
(mIdtEntryTemplate));
- }
-
- IdtDescriptor.Base = (UINTN)&IdtTableInStack.IdtTable;
- IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1);
-
- AsmWriteIdtr (&IdtDescriptor);
-
#if defined (MDE_CPU_X64)
//
// ASSERT that the Page Tables were set by the reset vector code to
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51195): https://edk2.groups.io/g/devel/message/51195
Mute This Topic: https://groups.io/mt/60973137/21656
Mute #vc: https://groups.io/mk?hashtag=vc&subid=3846945
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
