From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web09.6869.1604421531859319746 for ; Tue, 03 Nov 2020 08:38:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RPg8+RWQ; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1604421531; 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=6kMpizkGVzoH5rGtOeAMxX+KpsY6dcM9lxkDmf0xBL0=; b=RPg8+RWQq74yTkzLzczudBBL9HDDbVswQ6gPNyVrMr5GNhJGyI2pM7hcA6QHQOrvsQoPJ3 /wJLJ7PRvXPHmKwsc5MdIFtkrAYHd+tUKx3kjvZFJfWM6CltFPSJcszCOtdn216Wh1IzU5 mV7cU1GpTOokMqk5nCGslaRnzOHO4Ok= 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-6-N0a5ts8FMP-0nt_QWW5G8A-1; Tue, 03 Nov 2020 11:38:48 -0500 X-MC-Unique: N0a5ts8FMP-0nt_QWW5G8A-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AD3B86414C; Tue, 3 Nov 2020 16:38:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-129.ams2.redhat.com [10.36.112.129]) by smtp.corp.redhat.com (Postfix) with ESMTP id A1D1921EFB; Tue, 3 Nov 2020 16:38:44 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB To: Eric Dong , Ray Ni Cc: devel@edk2.groups.io, thomas.lendacky@amd.com, Brijesh Singh , Rahul Kumar , Jordan Justen , Ard Biesheuvel References: <0f3f11bec870bc4f10843beb53c6a919ac549ef4.1603981082.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <567da606-8763-c2a5-a2df-ddcc1648a00d@redhat.com> Date: Tue, 3 Nov 2020 17:38:43 +0100 MIME-Version: 1.0 In-Reply-To: <0f3f11bec870bc4f10843beb53c6a919ac549ef4.1603981082.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Ray, Eric, can one of you please ACK this patch as well, for the UefiCpuPkg parts? I've reviewed the patch already (in full), but your ACK would be appreciated, process-wise. I intend to merge the series on Thursday at the latest (Nov 5th). (Eric stated earlier that he was OK with modifying both UefiCpuPkg and OvmfPkg: .) Thank you. Laszlo On 10/29/20 15:18, Lendacky, Thomas wrote: > From: Tom Lendacky > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008 > > The QemuFlashPtrWrite() flash services runtime uses the GHCB and VmgExit() > directly to perform the flash write when running as an SEV-ES guest. If an > interrupt arrives between VmgInit() and VmgExit(), the Dr7 read in the > interrupt handler will generate a #VC, which can overwrite information in > the GHCB that QemuFlashPtrWrite() has set. This has been seen with the > timer interrupt firing and the CpuExceptionHandlerLib library code, > UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ > Xcode5ExceptionHandlerAsm.nasm and > ExceptionHandlerAsm.nasm > reading the Dr7 register while QemuFlashPtrWrite() is using the GHCB. In > general, it is necessary to protect the GHCB whenever it is used, not just > in QemuFlashPtrWrite(). > > Disable interrupts around the usage of the GHCB by modifying the VmgInit() > and VmgDone() interfaces: > - VmgInit() will take an extra parameter that is a pointer to a BOOLEAN > that will hold the interrupt state at the time of invocation. VmgInit() > will get and save this interrupt state before updating the GHCB. > - VmgDone() will take an extra parameter that is used to indicate whether > interrupts are to be (re)enabled. Before exiting, VmgDone() will enable > interrupts if that is requested. > > Fixes: 437eb3f7a8db7681afe0e6064d3a8edb12abb766 > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Tom Lendacky > Cc: Brijesh Singh > Reviewed-by: Laszlo Ersek > Signed-off-by: Tom Lendacky > --- > UefiCpuPkg/Include/Library/VmgExitLib.h | 14 ++++++++--- > OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 26 +++++++++++++++++--- > OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 5 ++-- > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 5 ++-- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 5 ++-- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 7 +++--- > UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c | 18 ++++++++------ > 7 files changed, 55 insertions(+), 25 deletions(-) > > diff --git a/UefiCpuPkg/Include/Library/VmgExitLib.h b/UefiCpuPkg/Include/Library/VmgExitLib.h > index 07e8af6450b9..061948cf840d 100644 > --- a/UefiCpuPkg/Include/Library/VmgExitLib.h > +++ b/UefiCpuPkg/Include/Library/VmgExitLib.h > @@ -50,13 +50,16 @@ VmgExit ( > Performs the necessary steps in preparation for invoking VMGEXIT. Must be > called before setting any fields within the GHCB. > > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] InterruptState A pointer to hold the current interrupt > + state, used for restoring in VmgDone () > > **/ > VOID > EFIAPI > VmgInit ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN OUT BOOLEAN *InterruptState > ); > > /** > @@ -65,13 +68,16 @@ VmgInit ( > Performs the necessary steps to cleanup after invoking VMGEXIT. Must be > called after obtaining needed fields within the GHCB. > > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in] InterruptState An indicator to conditionally (re)enable > + interrupts > > **/ > VOID > EFIAPI > VmgDone ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN BOOLEAN InterruptState > ); > > /** > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c > index 0540df8a04d4..bc5cd61d751f 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c > @@ -132,15 +132,27 @@ VmgExit ( > Performs the necessary steps in preparation for invoking VMGEXIT. Must be > called before setting any fields within the GHCB. > > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] InterruptState A pointer to hold the current interrupt > + state, used for restoring in VmgDone () > > **/ > VOID > EFIAPI > VmgInit ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN OUT BOOLEAN *InterruptState > ) > { > + // > + // Be sure that an interrupt can't cause a #VC while the GHCB is > + // being used. > + // > + *InterruptState = GetInterruptState (); > + if (*InterruptState) { > + DisableInterrupts (); > + } > + > SetMem (&Ghcb->SaveArea, sizeof (Ghcb->SaveArea), 0); > } > > @@ -150,15 +162,21 @@ VmgInit ( > Performs the necessary steps to cleanup after invoking VMGEXIT. Must be > called after obtaining needed fields within the GHCB. > > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in] InterruptState An indicator to conditionally (re)enable > + interrupts > > **/ > VOID > EFIAPI > VmgDone ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN BOOLEAN InterruptState > ) > { > + if (InterruptState) { > + EnableInterrupts (); > + } > } > > /** > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > index 9bf9d160179c..1671db3a01b1 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > @@ -1568,6 +1568,7 @@ VmgExitHandleVc ( > SEV_ES_INSTRUCTION_DATA InstructionData; > UINT64 ExitCode, Status; > EFI_STATUS VcRet; > + BOOLEAN InterruptState; > > VcRet = EFI_SUCCESS; > > @@ -1578,7 +1579,7 @@ VmgExitHandleVc ( > Regs = SystemContext.SystemContextX64; > Ghcb = Msr.Ghcb; > > - VmgInit (Ghcb); > + VmgInit (Ghcb, &InterruptState); > > ExitCode = Regs->ExceptionData; > switch (ExitCode) { > @@ -1662,7 +1663,7 @@ VmgExitHandleVc ( > VcRet = EFI_PROTOCOL_ERROR; > } > > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > > return VcRet; > } > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > index f9b21b54137d..1b0742967f71 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > @@ -52,6 +52,7 @@ QemuFlashPtrWrite ( > if (MemEncryptSevEsIsEnabled ()) { > MSR_SEV_ES_GHCB_REGISTER Msr; > GHCB *Ghcb; > + BOOLEAN InterruptState; > > Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > Ghcb = Msr.Ghcb; > @@ -63,12 +64,12 @@ QemuFlashPtrWrite ( > // #VC exception. Instead, use the the VMGEXIT MMIO write support directly > // to perform the update. > // > - VmgInit (Ghcb); > + VmgInit (Ghcb, &InterruptState); > Ghcb->SharedBuffer[0] = Value; > Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer; > VmgSetOffsetValid (Ghcb, GhcbSwScratch); > VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1); > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > } else { > *Ptr = Value; > } > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index 2c00d72ddefe..7839c249760e 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -171,6 +171,7 @@ GetSevEsAPMemory ( > EFI_PHYSICAL_ADDRESS StartAddress; > MSR_SEV_ES_GHCB_REGISTER Msr; > GHCB *Ghcb; > + BOOLEAN InterruptState; > > // > // Allocate 1 page for AP jump table page > @@ -192,9 +193,9 @@ GetSevEsAPMemory ( > Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > Ghcb = Msr.Ghcb; > > - VmgInit (Ghcb); > + VmgInit (Ghcb, &InterruptState); > VmgExit (Ghcb, SVM_EXIT_AP_JUMP_TABLE, 0, (UINT64) (UINTN) StartAddress); > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > > return (UINTN) StartAddress; > } > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 6d977d45bcdd..1f47ff3f73b5 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -884,6 +884,7 @@ ApWakeupFunction ( > GHCB *Ghcb; > UINT64 Status; > BOOLEAN DoDecrement; > + BOOLEAN InterruptState; > > DoDecrement = (BOOLEAN) (CpuMpData->InitFlag == ApInitConfig); > > @@ -891,7 +892,7 @@ ApWakeupFunction ( > Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > Ghcb = Msr.Ghcb; > > - VmgInit (Ghcb); > + VmgInit (Ghcb, &InterruptState); > > if (DoDecrement) { > DoDecrement = FALSE; > @@ -905,11 +906,11 @@ ApWakeupFunction ( > > Status = VmgExit (Ghcb, SVM_EXIT_AP_RESET_HOLD, 0, 0); > if ((Status == 0) && (Ghcb->SaveArea.SwExitInfo2 != 0)) { > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > break; > } > > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > } > > // > diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c > index b47e282aff82..89b065cb3ff3 100644 > --- a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c > +++ b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c > @@ -57,15 +57,16 @@ VmgExit ( > Performs the necessary steps in preparation for invoking VMGEXIT. Must be > called before setting any fields within the GHCB. > > - The base library function does nothing. > - > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] InterruptState A pointer to hold the current interrupt > + state, used for restoring in VmgDone () > > **/ > VOID > EFIAPI > VmgInit ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN OUT BOOLEAN *InterruptState > ) > { > } > @@ -76,15 +77,16 @@ VmgInit ( > Performs the necessary steps to cleanup after invoking VMGEXIT. Must be > called after obtaining needed fields within the GHCB. > > - The base library function does nothing. > - > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in] InterruptState An indicator to conditionally (re)enable > + interrupts > > **/ > VOID > EFIAPI > VmgDone ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN BOOLEAN InterruptState > ) > { > } >