From patchwork Fri Nov 18 13:52:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laszlo Ersek X-Patchwork-Id: 82911 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp87631qge; Fri, 18 Nov 2016 05:53:07 -0800 (PST) X-Received: by 10.99.137.66 with SMTP id v63mr19651564pgd.117.1479477186934; Fri, 18 Nov 2016 05:53:06 -0800 (PST) Return-Path: Received: from ml01.01.org (ml01.01.org. [2001:19d0:306:5::1]) by mx.google.com with ESMTPS id f10si8360689pge.120.2016.11.18.05.53.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 18 Nov 2016 05:53:06 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of edk2-devel-bounces@lists.01.org designates 2001:19d0:306:5::1 as permitted sender) client-ip=2001:19d0:306:5::1; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of edk2-devel-bounces@lists.01.org designates 2001:19d0:306:5::1 as permitted sender) smtp.mailfrom=edk2-devel-bounces@lists.01.org Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id 14DC981F25; Fri, 18 Nov 2016 05:53:01 -0800 (PST) X-Original-To: edk2-devel@ml01.01.org Delivered-To: edk2-devel@ml01.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3022C81F25 for ; Fri, 18 Nov 2016 05:52:59 -0800 (PST) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 41EFD4E048; Fri, 18 Nov 2016 13:53:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-70.phx2.redhat.com [10.3.116.70]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAIDqrU5013644; Fri, 18 Nov 2016 08:53:02 -0500 From: Laszlo Ersek To: edk2-devel-01 Date: Fri, 18 Nov 2016 14:52:49 +0100 Message-Id: <20161118135249.26018-5-lersek@redhat.com> In-Reply-To: <20161118135249.26018-1-lersek@redhat.com> References: <20161118135249.26018-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Fri, 18 Nov 2016 13:53:04 +0000 (UTC) Subject: [edk2] [PATCH v2 4/4] OvmfPkg/SmmControl2Dxe: select broadcast SMI if available X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Michael Kinney , Jordan Justen , Jeff Fan , Paolo Bonzini MIME-Version: 1.0 Errors-To: edk2-devel-bounces@lists.01.org Sender: "edk2-devel" When writing to IO port 0xB2 (ICH9_APM_CNT), QEMU by default injects an SMI only on the VCPU that is writing the port. This has exposed corner cases and strange behavior with edk2 code, which generally expects a software SMI to affect all CPUs at once. We've experienced instability despite the fact that OVMF sets PcdCpuSmmApSyncTimeout and PcdCpuSmmSyncMode differently from the UefiCpuPkg defaults, such that they match QEMU's unicast SMIs better. (Refer to edk2 commits 9b1e378811ff and bb0f18b0bce6.) Using the feature negotiation specified in QEMU's "docs/specs/q35-apm-sts.txt", we can ask QEMU to broadcast SMIs. Extensive testing from earlier proves that broadcast SMIs are only reliable if we use the UefiCpuPkg defaults for the above PCDs. With those settings however, the broadcast is very reliable -- the most reliable configuration encountered thus far. Therefore negotiate broadcast SMIs with QEMU, and if the negotiation is successful, dynamically revert the PCDs to the UefiCpuPkg defaults. Setting the PCDs in this module is safe: - only PiSmmCpuDxeSmm consumes them, - PiSmmCpuDxeSmm is a DXE_SMM_DRIVER, launched by the SMM_CORE (MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf), - the SMM_CORE is launched by the SMM IPL runtime DXE driver (MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf), - the SMM IPL has a DEPEX on EFI_SMM_CONTROL2_PROTOCOL, - OvmfPkg/SmmControl2Dxe produces that protocol. The end result is that PiSmmCpuDxeSmm cannot be dispatched before SmmControl2Dxe installs EFI_SMM_CONTROL2_PROTOCOL and returns from its entry point. Hence we can set the PCD's consumed by PiSmmCpuDxeSmm in SmmControl2Dxe. Cc: Jeff Fan Cc: Jordan Justen Cc: Michael Kinney Cc: Paolo Bonzini Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=230 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek --- Notes: v2: - negotiate the broadcast SMI feature - make the S3 boot script re-select it if it's available at first boot - set PiSmmCpuDxeSmm's PCD's dynamically OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 5 ++ OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 73 +++++++++++++++++++- 2 files changed, 75 insertions(+), 3 deletions(-) -- 2.9.2 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf index 0e9f98c2871c..c28832435eed 100644 --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf @@ -44,6 +44,7 @@ [Sources] [Packages] MdePkg/MdePkg.dec OvmfPkg/OvmfPkg.dec + UefiCpuPkg/UefiCpuPkg.dec [LibraryClasses] BaseLib @@ -59,6 +60,10 @@ [Protocols] gEfiS3SaveStateProtocolGuid ## SOMETIMES_CONSUMES gEfiSmmControl2ProtocolGuid ## PRODUCES +[Pcd] + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## SOMETIMES_PRODUCES + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## SOMETIMES_PRODUCES + [FeaturePcd] gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c index 82549b0a7e35..6f05797979de 100644 --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c @@ -56,6 +56,15 @@ OnS3SaveStateInstalled ( STATIC UINTN mSmiEnable; // +// The indicator whether we have negotiated with QEMU to broadcast the SMI to +// all VCPUs whenever we write to ICH9_APM_CNT in our +// EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation. This variable is only +// used to carry information from the entry point function to the S3SaveState +// protocol installation callback. +// +STATIC BOOLEAN mSmiBroadcast; + +// // Event signaled when an S3SaveState protocol interface is installed. // STATIC EFI_EVENT mS3SaveStateInstalled; @@ -107,10 +116,10 @@ SmmControl2DxeTrigger ( // report about hardware status, while this register is fully governed by // software. // - // Write to the status register first, as this won't trigger the SMI just - // yet. Then write to the control register. + // QEMU utilizes the status register for feature negotiation, therefore we + // can't accept external data. // - IoWrite8 (ICH9_APM_STS, DataPort == NULL ? 0 : *DataPort); + ASSERT (DataPort == NULL); IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort); return EFI_SUCCESS; } @@ -177,6 +186,7 @@ SmmControl2DxeEntryPoint ( { UINT32 PmBase; UINT32 SmiEnableVal; + UINT8 ApmStatusVal; EFI_STATUS Status; // @@ -229,6 +239,43 @@ SmmControl2DxeEntryPoint ( goto FatalError; } + // + // Negotiate broadcast SMI with QEMU. + // + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_GET_SET_FEAT); + ApmStatusVal = IoRead8 (ICH9_APM_STS); + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) { + DEBUG ((DEBUG_VERBOSE, "%a: SMI feature negotiation unavailable\n", + __FUNCTION__)); + IoWrite8 (ICH9_APM_STS, 0); + } else if ((ApmStatusVal & QEMU_ICH9_APM_STS_F_BCAST_SMI) == 0) { + DEBUG ((DEBUG_VERBOSE, "%a: SMI broadcast unavailable\n", __FUNCTION__)); + } else { + // + // Request the broadcast feature, and nothing else. Check for confirmation. + // + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_F_BCAST_SMI); + ApmStatusVal = IoRead8 (ICH9_APM_STS); + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) != 0) { + DEBUG ((DEBUG_VERBOSE, "%a: failed to negotiate SMI broadcast\n", + __FUNCTION__)); + } else { + // + // Configure the traditional AP sync / SMI delivery mode for + // PiSmmCpuDxeSmm. Effectively, restore the UefiCpuPkg defaults, from + // which the original QEMU behavior (i.e., unicast SMI) used to differ. + // + if (RETURN_ERROR (PcdSet64S (PcdCpuSmmApSyncTimeout, 1000000)) || + RETURN_ERROR (PcdSet8S (PcdCpuSmmSyncMode, 0x00))) { + DEBUG ((DEBUG_ERROR, "%a: PiSmmCpuDxeSmm PCD configuration failed\n", + __FUNCTION__)); + goto FatalError; + } + mSmiBroadcast = TRUE; + DEBUG ((DEBUG_INFO, "%a: using SMI broadcast\n", __FUNCTION__)); + } + } + if (QemuFwCfgS3Enabled ()) { VOID *Registration; @@ -360,6 +407,26 @@ OnS3SaveStateInstalled ( CpuDeadLoop (); } + if (mSmiBroadcast) { + UINT8 ApmStatusVal; + + ApmStatusVal = QEMU_ICH9_APM_STS_F_BCAST_SMI; + Status = S3SaveState->Write ( + S3SaveState, + EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, + EfiBootScriptWidthUint8, + (UINT64)ICH9_APM_STS, + (UINTN)1, + &ApmStatusVal + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n", + __FUNCTION__, Status)); + ASSERT (FALSE); + CpuDeadLoop (); + } + } + DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__)); gBS->CloseEvent (Event); mS3SaveStateInstalled = NULL;