From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 80A4181E89 for ; Tue, 22 Nov 2016 19:44:06 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga104.fm.intel.com with ESMTP; 22 Nov 2016 19:44:05 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,684,1473145200"; d="scan'208";a="8246242" Received: from mswamyee-mobl2.amr.corp.intel.com (HELO localhost) ([10.252.141.16]) by orsmga002.jf.intel.com with ESMTP; 22 Nov 2016 19:44:05 -0800 MIME-Version: 1.0 To: Laszlo Ersek , "edk2-devel-01" Message-ID: <147987264544.21600.17139942187851594260@jljusten-ivb> From: Jordan Justen In-Reply-To: <20161118135249.26018-5-lersek@redhat.com> Cc: "Michael Kinney" , "Jeff Fan" , "Paolo Bonzini" References: <20161118135249.26018-1-lersek@redhat.com> <20161118135249.26018-5-lersek@redhat.com> User-Agent: alot/0.3.7 Date: Tue, 22 Nov 2016 19:44:05 -0800 Subject: Re: [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: , X-List-Received-Date: Wed, 23 Nov 2016 03:44:06 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2016-11-18 05:52:49, Laszlo Ersek wrote: > 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=3D230 > 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(-) > = > diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmContr= ol2Dxe/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/SmmControl= 2Dxe/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 S= MI 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 S3Save= State > +// protocol installation callback. > +// > +STATIC BOOLEAN mSmiBroadcast; I think you are relying on uninitialized static data being zero'd. Does it hurt to actually set it to FALSE? I'm glad we'll be using a mechanism that broadcasts to all the processors like the real hardware. It is a bit unfortunate that it doesn't go through the b2 port for it. Did you get a chance to test variable writes on Windows with this change? I know Windows can be more picky about port accesses... Series Reviewed-by: Jordan Justen Thanks! > + > +// > // 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 j= ust > - // yet. Then write to the control register. > + // QEMU utilizes the status register for feature negotiation, therefor= e we > + // can't accept external data. > // > - IoWrite8 (ICH9_APM_STS, DataPort =3D=3D NULL ? 0 : *DataPort); > + ASSERT (DataPort =3D=3D NULL); > IoWrite8 (ICH9_APM_CNT, CommandPort =3D=3D 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 =3D IoRead8 (ICH9_APM_STS); > + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) !=3D 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) =3D=3D 0) { > + DEBUG ((DEBUG_VERBOSE, "%a: SMI broadcast unavailable\n", __FUNCTION= __)); > + } else { > + // > + // Request the broadcast feature, and nothing else. Check for confir= mation. > + // > + IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_F_BCAST_SMI); > + ApmStatusVal =3D IoRead8 (ICH9_APM_STS); > + if ((ApmStatusVal & QEMU_ICH9_APM_STS_GET_SET_FEAT) !=3D 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, f= rom > + // which the original QEMU behavior (i.e., unicast SMI) used to di= ffer. > + // > + if (RETURN_ERROR (PcdSet64S (PcdCpuSmmApSyncTimeout, 1000000)) || > + RETURN_ERROR (PcdSet8S (PcdCpuSmmSyncMode, 0x00))) { > + DEBUG ((DEBUG_ERROR, "%a: PiSmmCpuDxeSmm PCD configuration faile= d\n", > + __FUNCTION__)); > + goto FatalError; > + } > + mSmiBroadcast =3D 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 =3D QEMU_ICH9_APM_STS_F_BCAST_SMI; > + Status =3D 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 =3D NULL; > -- = > 2.9.2 > = > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel