From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.6004.1574427139485061180 for ; Fri, 22 Nov 2019 04:52:20 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YIT1d3vj; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1574427138; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HA7g/0oN/PqTgAMSPoCSLQiCVt5+RlXYtRaGgSE+VqM=; b=YIT1d3vjd+9rEbkD3lBS7x+GUuq52jlxHlh5axZLwfFC4Qn/mZGHH+2sZt6m1DlU8mo+HA 7YzaoMOLhOu9g6y+Vb5Jz+shSEvVQ5Zf8OOaNEprg3+54EP6S3UN5/65QOEH2mTIksG58B 8nT2OT4U17HoVeyo1diOcxdtnatZTtE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-131-7f4NJz7MMUyf5SHu40I8SQ-1; Fri, 22 Nov 2019 07:52:13 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A08921800D41; Fri, 22 Nov 2019 12:52:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id 68C0D61F37; Fri, 22 Nov 2019 12:52:09 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v3 30/43] OvmfPkg/Sec: Add #VC exception handling for Sec phase To: Tom Lendacky , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: <041cb6f6352c9e0f9982d316cd842c2daf57e7fa.1574280425.git.thomas.lendacky@amd.com> <12e35ff5-ff14-87d4-f4ac-604448289688@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 22 Nov 2019 13:52:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: 7f4NJz7MMUyf5SHu40I8SQ-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable 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://bugzilla.tianocore.org/show_bug.cgi?id=3D2198 >>> >>> 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 >>> Cc: Laszlo Ersek >>> Cc: Ard Biesheuvel >>> Signed-off-by: Tom Lendacky >>> --- >>> 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 >>> #include >>> #include >>> +#include >>> >>> #include >>> >>> @@ -737,6 +738,21 @@ SecCoreStartupWithStack ( >>> Table[Index] =3D 0; >>> } >>> >>> + // >>> + // Initialize IDT >>> + // >>> + IdtTableInStack.PeiService =3D NULL; >>> + for (Index =3D 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) { >>> + CopyMem (&IdtTableInStack.IdtTable[Index], &mIdtEntryTemplate, siz= eof (mIdtEntryTemplate)); >>> + } >>> + >>> + IdtDescriptor.Base =3D (UINTN)&IdtTableInStack.IdtTable; >>> + IdtDescriptor.Limit =3D (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. 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(), (b) I'd still like to see this pre-constructor logic restricted to SEV-ES. (More on that below.) 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))->SevEs= Enabled; 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.) 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 =3D NULL; >>> - for (Index =3D 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) { >>> - CopyMem (&IdtTableInStack.IdtTable[Index], &mIdtEntryTemplate, siz= eof (mIdtEntryTemplate)); >>> - } >>> - >>> - IdtDescriptor.Base =3D (UINTN)&IdtTableInStack.IdtTable; >>> - IdtDescriptor.Limit =3D (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 >>> >> >