From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.8562.1602755165339772136 for ; Thu, 15 Oct 2020 02:46:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Jim24Cub; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602755164; 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=MHR20d+4MdLdEeHIT3Il0MEYVt6ERJt4N+ryl7VbnN8=; b=Jim24CubuMDRIwC7pW0peyoQ1L+W68y5oqz1ilInbnFDYRrZMn6hXwQRVGEuiCojbsylrn ZJEACjFw6X+IU3YNWeZqyPXe4tMUY/pq1bKsozXscBMRdC5rC62wU9764qdceqBPDMizu4 H84s0qkkEnlUZIvvD/zDge4pmgo0uEQ= 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-251-xtmGCImRPPi-VeObxZRORQ-1; Thu, 15 Oct 2020 05:46:02 -0400 X-MC-Unique: xtmGCImRPPi-VeObxZRORQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 70DD08797F0; Thu, 15 Oct 2020 09:46:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-119.ams2.redhat.com [10.36.113.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id F0E5E6CE4D; Thu, 15 Oct 2020 09:45:59 +0000 (UTC) Subject: Re: [PATCH 8/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using GHCB To: Tom Lendacky , devel@edk2.groups.io Cc: Brijesh Singh , Jordan Justen , Ard Biesheuvel References: <4f66dc48ae127d8b42313e7a0a8c7cf10667dcb3.1602346027.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 15 Oct 2020 11:45:58 +0200 MIME-Version: 1.0 In-Reply-To: <4f66dc48ae127d8b42313e7a0a8c7cf10667dcb3.1602346027.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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 On 10/10/20 18:07, Tom Lendacky wrote: > From: Tom Lendacky > > 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 (1) please clarify what kind of interrupt you've seen in practice (my guess: timer interrupt) > between VmgInit() and VmgDone(), (2) VmgDone() is currently an empty function (both library instances) -- did you mean VmgExit()? > the Dr7 read in the > interrupt handler (3) please clarify what interrupt handler you have in mind (function name and file with full path would be helpful) > will generate a #VC, which can overwrite information in > the GHCB that QemuFlashPtrWrite() has set. Prevent this from occurring by > disabling interrupts around the usage of the GHCB. (4) I like the last sentence, because it seems to support my suspicion that the problem is generic. Should we push the interrupt disablement / re-enablement logic into VmgInit() and VmgDone()? For that, the pre-VmgInit() interrupt state would have to be saved (for restoration) somewhere. (5) I wonder if raising the TPL to TPL_HIGH_LEVEL, rather than messing with interrupts explicitly, works too. (Search the UEFI spec for "TPL_HIGH_LEVEL".) Managing the TPL feels cleaner. ... Either way, VmgInit() would have to output either the "old TPL", or the "old interrupt state", for VmgDone() to restore. ... Hm wait, VmgInit() is called from ApWakeupFunction() [UefiCpuPkg/Library/MpInitLib/MpLib.c], which is built for PEI too -- there is no "TPL" concept in PEI. So I guess we should indeed manipulate the interrupts briefly, but I believe we should still have that logic occur every time we are setting up a VmgExit(). And so VmgInit() / VmgDone() look like the perfect bracket for that. What's your take? Thanks! Laszlo > > Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES") > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > index 5d5a117c48e0..872e58db7cc0 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > @@ -9,6 +9,7 @@ > > **/ > > +#include > #include > #include > #include > @@ -54,6 +55,7 @@ QemuFlashPtrWrite ( > GHCB *Ghcb; > UINT32 ScratchIndex; > UINT32 ScratchBit; > + BOOLEAN InterruptsEnabled; > > Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > Ghcb = Msr.Ghcb; > @@ -61,6 +63,15 @@ QemuFlashPtrWrite ( > ScratchIndex = GhcbSwScratch / 8; > ScratchBit = GhcbSwScratch & 0x07; > > + // > + // Be sure that an interrupt can't cause a #VC while the GHCB is > + // being used. > + // > + InterruptsEnabled = GetInterruptState (); > + if (InterruptsEnabled) { > + DisableInterrupts (); > + } > + > // > // Writing to flash is emulated by the hypervisor through the use of write > // protection. This won't work for an SEV-ES guest because the write won't > @@ -74,6 +85,10 @@ QemuFlashPtrWrite ( > Ghcb->SaveArea.ValidBitmap[ScratchIndex] |= (1 << ScratchBit); > VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1); > VmgDone (Ghcb); > + > + if (InterruptsEnabled) { > + EnableInterrupts (); > + } > } else { > *Ptr = Value; > } >