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.web12.3683.1583242169879482740 for ; Tue, 03 Mar 2020 05:29:30 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=elQPvEJW; 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=1583242169; 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=xnKOZ0QaetIgaEUzldIggvcDXaGJt/71Bof5LQYblbE=; b=elQPvEJWXz1YRpcp4BnT3Fb4q0pxEComtwRq+3VnMwd+hAa1hG/YG04gagquvHdO6q6bwe s+l0x44uKEqJp6qL2mumppjOQpbaGD1fSK7lr1pG6p7TlNushRdwATqJDzcju4UuVuqXGY PuLpoCk9Ju1NXJalVJDt1YmZo68YAhU= 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-127-FijkcPBiNMa0tUCHb3M3OQ-1; Tue, 03 Mar 2020 08:29:23 -0500 X-MC-Unique: FijkcPBiNMa0tUCHb3M3OQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 408BD1005510; Tue, 3 Mar 2020 13:29:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-34.ams2.redhat.com [10.36.117.34]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0919990782; Tue, 3 Mar 2020 13:29:18 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v5 34/42] OvmfPkg/Sec: Add #VC exception handling for Sec phase To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: <555d15ee95133cce0617edbd5500c05785847a4c.1583190432.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 3 Mar 2020 14:29:18 +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: <555d15ee95133cce0617edbd5500c05785847a4c.1583190432.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Hi Tom, On 03/03/20 00:07, 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Reviewed-by: Laszlo Ersek this patch has changed significantly since the last version (v4); I think my R-b given for v4 should not have been picked up in v5. AFAICS the new stuff is SevEsProtocolFailure() and SevEsProtocolCheck(), which -- per v5 blurb -- have moved from UefiCpuPkg to OvmfPkg: > Signed-off-by: Tom Lendacky > --- > OvmfPkg/Sec/SecMain.inf | 4 ++ > OvmfPkg/Sec/SecMain.c | 151 ++++++++++++++++++++++++++++++++++++---- > 2 files changed, 142 insertions(+), 13 deletions(-) > > diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf > index 63ba4cb555fb..7f78dcee2772 100644 > --- a/OvmfPkg/Sec/SecMain.inf > +++ b/OvmfPkg/Sec/SecMain.inf > @@ -50,15 +50,19 @@ [LibraryClasses] > PeCoffExtraActionLib > ExtractGuidedSectionLib > LocalApicLib > + CpuExceptionHandlerLib > > [Ppis] > gEfiTemporaryRamSupportPpiGuid # PPI ALWAYS_PRODUCED > > [Pcd] > + gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c > index bae9764577f0..577596a949f9 100644 > --- a/OvmfPkg/Sec/SecMain.c > +++ b/OvmfPkg/Sec/SecMain.c > @@ -24,6 +24,9 @@ > #include > #include > #include > +#include > +#include > +#include > > #include > > @@ -34,6 +37,10 @@ typedef struct _SEC_IDT_TABLE { > IA32_IDT_GATE_DESCRIPTOR IdtTable[SEC_IDT_ENTRY_COUNT]; > } SEC_IDT_TABLE; > > +typedef struct _SEC_SEV_ES_WORK_AREA { > + UINT8 SevEsEnabled; > +} SEC_SEV_ES_WORK_AREA; > + > VOID > EFIAPI > SecStartupPhase2 ( > @@ -712,6 +719,90 @@ FindAndReportEntryPoints ( > return; > } > > +STATIC > +VOID > +SevEsProtocolFailure ( > + IN UINT8 ReasonCode > + ) > +{ > + MSR_SEV_ES_GHCB_REGISTER Msr; > + > + // > + // Use the GHCB MSR Protocol to request termination by the hypervisor > + // > + Msr.GhcbPhysicalAddress = 0; > + Msr.GhcbTerminate.Function = GHCB_INFO_TERMINATE_REQUEST; > + Msr.GhcbTerminate.ReasonCodeSet = GHCB_TERMINATE_GHCB; > + Msr.GhcbTerminate.ReasonCode = ReasonCode; > + AsmWriteMsr64 (MSR_SEV_ES_GHCB, Msr.GhcbPhysicalAddress); > + > + AsmVmgExit (); > + > + ASSERT (0); (1) The argument should be FALSE, not 0. > + CpuDeadLoop (); > +} > + > +STATIC > +VOID > +SevEsProtocolCheck ( > + VOID > + ) > +{ > + MSR_SEV_ES_GHCB_REGISTER Msr; > + GHCB *Ghcb; > + > + // > + // Use the GHCB MSR Protocol to obtain the GHCB SEV-ES Information for > + // protocol checking > + // > + Msr.GhcbPhysicalAddress = 0; > + Msr.GhcbInfo.Function = GHCB_INFO_SEV_INFO_GET; > + AsmWriteMsr64 (MSR_SEV_ES_GHCB, Msr.GhcbPhysicalAddress); > + > + AsmVmgExit (); > + > + Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > + > + if (Msr.GhcbInfo.Function != GHCB_INFO_SEV_INFO) { > + SevEsProtocolFailure (GHCB_TERMINATE_GHCB_GENERAL); > + } > + > + if (Msr.GhcbProtocol.SevEsProtocolMin > Msr.GhcbProtocol.SevEsProtocolMax) { > + SevEsProtocolFailure (GHCB_TERMINATE_GHCB_PROTOCOL); > + } > + > + if ((Msr.GhcbProtocol.SevEsProtocolMin > GHCB_VERSION_MAX) || > + (Msr.GhcbProtocol.SevEsProtocolMax < GHCB_VERSION_MIN)) { > + SevEsProtocolFailure (GHCB_TERMINATE_GHCB_PROTOCOL); > + } > + > + // > + // SEV-ES protocol checking succeeded, set the initial GHCB address > + // > + Msr.GhcbPhysicalAddress = FixedPcdGet32 (PcdOvmfSecGhcbBase); > + AsmWriteMsr64(MSR_SEV_ES_GHCB, Msr.GhcbPhysicalAddress); > + > + Ghcb = Msr.Ghcb; > + SetMem (Ghcb, sizeof (*Ghcb), 0); > + > + /* Set the version to the maximum that can be supported */ (2) The comment style is not consistent with the rest. With the style warts (1) and (2) fixed: Reviewed-by: Laszlo Ersek Thanks Laszlo > + Ghcb->ProtocolVersion = MIN (Msr.GhcbProtocol.SevEsProtocolMax, GHCB_VERSION_MAX); > + Ghcb->GhcbUsage = GHCB_STANDARD_USAGE; > +} > + > +STATIC > +BOOLEAN > +SevEsIsEnabled ( > + VOID > + ) > +{ > + SEC_SEV_ES_WORK_AREA *SevEsWorkArea; > + > + SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase); > + > + return ((SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0)); > +} > + > VOID > EFIAPI > SecCoreStartupWithStack ( > @@ -737,8 +828,55 @@ SecCoreStartupWithStack ( > Table[Index] = 0; > } > > + // > + // Initialize IDT - Since this is before library constructors are called, > + // we use a loop rather than CopyMem. > + // > + IdtTableInStack.PeiService = NULL; > + for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) { > + UINT8 *Src, *Dst; > + UINTN Byte; > + > + Src = (UINT8 *) &mIdtEntryTemplate; > + Dst = (UINT8 *) &IdtTableInStack.IdtTable[Index]; > + for (Byte = 0; Byte < sizeof (mIdtEntryTemplate); Byte++) { > + Dst[Byte] = Src[Byte]; > + } > + } > + > + IdtDescriptor.Base = (UINTN)&IdtTableInStack.IdtTable; > + IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1); > + > + if (SevEsIsEnabled ()) { > + SevEsProtocolCheck (); > + > + // > + // For SEV-ES guests, the exception handler is needed before calling > + // ProcessLibraryConstructorList() because some of the library constructors > + // perform some functions that result in #VC exceptions being generated. > + // > + // Due to this code executing before library constructors, *all* library > + // API calls are theoretically interface contract violations. However, > + // because this is SEC (executing in flash), those constructors cannot > + // write variables with static storage duration anyway. Furthermore, only > + // a small, restricted set of APIs, such as AsmWriteIdtr() and > + // InitializeCpuExceptionHandlers(), are called, where we require that the > + // underlying library not require constructors to have been invoked and > + // that the library instance not trigger any #VC exceptions. > + // > + AsmWriteIdtr (&IdtDescriptor); > + InitializeCpuExceptionHandlers (NULL); > + } > + > ProcessLibraryConstructorList (NULL, NULL); > > + if (!SevEsIsEnabled ()) { > + // > + // For non SEV-ES guests, just load the IDTR. > + // > + AsmWriteIdtr (&IdtDescriptor); > + } > + > DEBUG ((EFI_D_INFO, > "SecCoreStartupWithStack(0x%x, 0x%x)\n", > (UINT32)(UINTN)BootFv, > @@ -751,19 +889,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 >