On 11/21/19 6:06 AM, Laszlo Ersek wrote:
> On 11/20/19 21:06, Lendacky, Thomas wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> 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().
>
> (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.
>
>
> (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...
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 (#51112): https://edk2.groups.io/g/devel/message/51112
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]
-=-=-=-=-=-=-=-=-=-=-=-
