Laszlo, Minor comments included below.
Reviewed-by: Michael Kinney <michael.d.kin...@intel.com> Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Tuesday, November 3, 2015 1:01 PM > To: edk2-de...@ml01.01.org > Subject: [edk2] [PATCH v4 09/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL > with a DXE_RUNTIME_DRIVER > > The EFI_SMM_COMMUNICATION_PROTOCOL implementation that is provided by the SMM > core depends on > EFI_SMM_CONTROL2_PROTOCOL; see the > mSmmControl2->Trigger() call in the SmmCommunicationCommunicate() > mSmmControl2->function > [MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c]. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > > Notes: > v2: > - Set (APMC_EN|GBL_SMI_EN) in SMI_EN from the boot script at S3 resume. > Otherwise, because SMI_EN is cleared during warm reset, > SmmControl2DxeTrigger() would fail to trigger an SMI, and variable > access through the runtime services would fail. > > Set SMI_LOCK in GEN_PMCON_1 similarly. > > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/OvmfPkgIa32.fdf | 1 + > OvmfPkg/OvmfPkgIa32X64.fdf | 1 + > OvmfPkg/OvmfPkgX64.fdf | 1 + > OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 65 ++++ > OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 365 ++++++++++++++++++++ > 8 files changed, 436 insertions(+) > > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index > d7bc38d..c71a2f4 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -675,4 +675,5 @@ [Components] > > !if $(SMM_REQUIRE) == TRUE > OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > + OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > !endif > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index > e17cbe5..a1e8f0d 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -682,4 +682,5 @@ [Components.X64] > > !if $(SMM_REQUIRE) == TRUE > OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > + OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > !endif > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index > a748fb3..1176629 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -680,4 +680,5 @@ [Components] > > !if $(SMM_REQUIRE) == TRUE > OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > + OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > !endif > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index > 285720f..43c9c30 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -357,6 +357,7 @@ [FV.DXEFV] > > !if $(SMM_REQUIRE) == TRUE > INF OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > +INF OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > !endif > > > ################################################################################ > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf index > 02e8752..9446896 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -357,6 +357,7 @@ [FV.DXEFV] > > !if $(SMM_REQUIRE) == TRUE > INF OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > +INF OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > !endif > > > ################################################################################ > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index > f04c36b..b272b76 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -357,6 +357,7 @@ [FV.DXEFV] > > !if $(SMM_REQUIRE) == TRUE > INF OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > +INF OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > !endif > > > ################################################################################ > diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > new file mode 100644 > index 0000000..bc66a27 > --- /dev/null > +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf > @@ -0,0 +1,65 @@ > +## @file > +# A DXE_RUNTIME_DRIVER providing synchronous SMI activations via the # > +EFI_SMM_CONTROL2_PROTOCOL. > +# > +# We expect the PEI phase to have covered the following: > +# - ensure that the underlying QEMU machine type be Q35 > +# (responsible: OvmfPkg/SmmAccess/SmmAccessPei.inf) > +# - ensure that the ACPI PM IO space be configured > +# (responsible: OvmfPkg/PlatformPei/PlatformPei.inf) > +# > +# Our own entry point is responsible for confirming the SMI feature and > +for # configuring it. > +# > +# Copyright (C) 2013, 2015, Red Hat, Inc. > +# > +# This program and the accompanying materials are licensed and made > +available # under the terms and conditions of the BSD License which > +accompanies this # distribution. The full text of the license may be > +found at # http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +WITHOUT # WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = SmmControl2Dxe > + FILE_GUID = 1206F7CA-A475-4624-A83E-E6FC9BB38E49 > + MODULE_TYPE = DXE_RUNTIME_DRIVER > + VERSION_STRING = 1.0 > + PI_SPECIFICATION_VERSION = 0x00010400 > + ENTRY_POINT = SmmControl2DxeEntryPoint > + > +# > +# The following information is for reference only and not required by the > build tools. > +# > +# VALID_ARCHITECTURES = IA32 X64 > +# > + > +[Sources] > + SmmControl2Dxe.c > + > +[Packages] > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + DebugLib > + IoLib > + PcdLib > + PciLib > + QemuFwCfgLib > + UefiBootServicesTableLib > + UefiDriverEntryPoint > + > +[Protocols] > + gEfiS3SaveStateProtocolGuid # PROTOCOL SOMETIMES_CONSUMED Comment should be: ## SOMETIMES_CONSUMES > + gEfiSmmControl2ProtocolGuid # PROTOCOL ALWAYS_PRODUCED Comment should be: ## PRODUCES > + > +[FeaturePcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > + > +[Depex] > + TRUE > diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > new file mode 100644 > index 0000000..e895fd6 > --- /dev/null > +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c > @@ -0,0 +1,365 @@ > +/** @file > + > + A DXE_RUNTIME_DRIVER providing synchronous SMI activations via the > + EFI_SMM_CONTROL2_PROTOCOL. > + > + We expect the PEI phase to have covered the following: > + - ensure that the underlying QEMU machine type be Q35 > + (responsible: OvmfPkg/SmmAccess/SmmAccessPei.inf) > + - ensure that the ACPI PM IO space be configured > + (responsible: OvmfPkg/PlatformPei/PlatformPei.inf) > + > + Our own entry point is responsible for confirming the SMI feature and > + for configuring it. > + > + Copyright (C) 2013, 2015, Red Hat, Inc.<BR> Copyright (c) 2009 - > + 2010, Intel Corporation. All rights reserved.<BR> > + > + This program and the accompanying materials are licensed and made > + available under the terms and conditions of the BSD License which > + accompanies this distribution. The full text of the license may be > + found at http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > + > +**/ > + > +#include <IndustryStandard/Q35MchIch9.h> #include <Library/DebugLib.h> > +#include <Library/IoLib.h> #include <Library/PcdLib.h> #include > +<Library/PciLib.h> #include <Library/QemuFwCfgLib.h> #include > +<Library/UefiBootServicesTableLib.h> > +#include <Protocol/S3SaveState.h> > +#include <Protocol/SmmControl2.h> > + > +// > +// The absolute IO port address of the SMI Control and Enable Register. > +It is // only used to carry information from the entry point function > +to the // S3SaveState protocol installation callback, strictly before > +the runtime // phase. > +// > +STATIC UINTN mSmiEnable; > + > +// > +// Event signaled when an S3SaveState protocol interface is installed. > +// > +STATIC EFI_EVENT mS3SaveStateInstalled; > + > +/** > + Invokes SMI activation from either the preboot or runtime environment. > + > + This function generates an SMI. > + > + @param[in] This The EFI_SMM_CONTROL2_PROTOCOL instance. > + @param[in,out] CommandPort The value written to the command port. > + @param[in,out] DataPort The value written to the data port. > + @param[in] Periodic Optional mechanism to engender a > periodic > + stream. > + @param[in] ActivationInterval Optional parameter to repeat at this > + period one time or, if the Periodic > + Boolean is set, periodically. > + > + @retval EFI_SUCCESS The SMI/PMI has been engendered. > + @retval EFI_DEVICE_ERROR The timing is unsupported. > + @retval EFI_INVALID_PARAMETER The activation period is unsupported. > + @retval EFI_INVALID_PARAMETER The last periodic activation has not been > + cleared. > + @retval EFI_NOT_STARTED The SMM base service has not been > initialized. > +**/ > +STATIC Recommend removing use of STATIC > +EFI_STATUS > +EFIAPI > +SmmControl2DxeTrigger ( > + IN CONST EFI_SMM_CONTROL2_PROTOCOL *This, > + IN OUT UINT8 *CommandPort OPTIONAL, > + IN OUT UINT8 *DataPort OPTIONAL, > + IN BOOLEAN Periodic OPTIONAL, > + IN UINTN ActivationInterval OPTIONAL > + ) > +{ > + // > + // No support for queued or periodic activation. > + // > + if (Periodic || ActivationInterval > 0) { > + return EFI_DEVICE_ERROR; > + } > + > + // > + // The so-called "Advanced Power Management Status Port Register" is > +in fact > + // a generic data passing register, between the caller and the SMI > + // dispatcher. The ICH9 spec calls it "scratchpad register" -- > +calling it > + // "status" elsewhere seems quite the misnomer. Status registers > +usually > + // 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. > + // > + IoWrite8 (ICH9_APM_STS, DataPort == NULL ? 0 : *DataPort); > + IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort); > + return EFI_SUCCESS; > +} > + > +/** > + Clears any system state that was created in response to the Trigger() call. > + > + This function acknowledges and causes the deassertion of the SMI > + activation source. > + > + @param[in] This The EFI_SMM_CONTROL2_PROTOCOL instance. > + @param[in] Periodic Optional parameter to repeat at this period > + one time > + > + @retval EFI_SUCCESS The SMI/PMI has been engendered. > + @retval EFI_DEVICE_ERROR The source could not be cleared. > + @retval EFI_INVALID_PARAMETER The service did not support the Periodic > input > + argument. > +**/ > +STATIC Recommend removing use of STATIC > +EFI_STATUS > +EFIAPI > +SmmControl2DxeClear ( > + IN CONST EFI_SMM_CONTROL2_PROTOCOL *This, > + IN BOOLEAN Periodic OPTIONAL > + ) > +{ > + if (Periodic) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // The PI spec v1.4 explains that Clear() is only supposed to clear > +software > + // status; it is not in fact responsible for deasserting the SMI. It > +gives > + // two reasons for this: (a) many boards clear the SMI automatically > +when > + // entering SMM, (b) if Clear() actually deasserted the SMI, then it > +could > + // incorrectly suppress an SMI that was asynchronously asserted > +between the > + // last return of the SMI handler and the call made to Clear(). > + // > + // In fact QEMU automatically deasserts CPU_INTERRUPT_SMI in: > + // - x86_cpu_exec_interrupt() [target-i386/seg_helper.c], and > + // - kvm_arch_pre_run() [target-i386/kvm.c]. > + // > + // So, nothing to do here. > + // > + return EFI_SUCCESS; > +} > + > +STATIC EFI_SMM_CONTROL2_PROTOCOL mControl2 = { > + &SmmControl2DxeTrigger, > + &SmmControl2DxeClear, > + MAX_UINTN // MinimumTriggerPeriod -- we don't support periodic SMIs > +}; Recommend removing use of STATIC Global variable should be above function implementations > + > +// > +// Forward declaration. > +// > +STATIC Recommend removing use of STATIC > +VOID > +EFIAPI > +OnS3SaveStateInstalled ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ); Forward declarations should go before function implementations > + > +// > +// Entry point of this driver. > +// > +EFI_STATUS > +EFIAPI > +SmmControl2DxeEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + UINT32 PmBase; > + UINT32 SmiEnableVal; > + EFI_STATUS Status; > + > + // > + // This module should only be included if SMRAM support is required. > + // > + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); > + > + // > + // Calculate the absolute IO port address of the SMI Control and > + Enable // Register. (As noted at the top, the PEI phase has left us > + with a working // ACPI PM IO space.) // PmBase = PciRead32 > + (POWER_MGMT_REGISTER_Q35 (ICH9_PMBASE)) & > + ICH9_PMBASE_MASK; > + mSmiEnable = PmBase + ICH9_PMBASE_OFS_SMI_EN; > + > + // > + // If APMC_EN is pre-set in SMI_EN, that's QEMU's way to tell us that > + SMI // support is not available. (For example due to KVM lacking it.) > + Otherwise, // this bit is clear after each reset. > + // > + SmiEnableVal = IoRead32 (mSmiEnable); if ((SmiEnableVal & > + ICH9_SMI_EN_APMC_EN) != 0) { > + DEBUG ((EFI_D_ERROR, "%a: this Q35 implementation lacks SMI\n", > + __FUNCTION__)); > + goto FatalError; > + } > + > + // > + // Otherwise, configure the board to inject an SMI when ICH9_APM_CNT > + is // written to. (See the Trigger() method above.) // SmiEnableVal > + |= ICH9_SMI_EN_APMC_EN | ICH9_SMI_EN_GBL_SMI_EN; > + IoWrite32 (mSmiEnable, SmiEnableVal); > + > + // > + // Prevent software from undoing the above (until platform reset). > + // > + PciOr16 (POWER_MGMT_REGISTER_Q35 (ICH9_GEN_PMCON_1), > + ICH9_GEN_PMCON_1_SMI_LOCK); > + > + // > + // If we can clear GBL_SMI_EN now, that means QEMU's SMI support is > + not // appropriate. > + // > + IoWrite32 (mSmiEnable, SmiEnableVal & > + ~(UINT32)ICH9_SMI_EN_GBL_SMI_EN); if (IoRead32 (mSmiEnable) != > SmiEnableVal) { > + DEBUG ((EFI_D_ERROR, "%a: failed to lock down GBL_SMI_EN\n", > + __FUNCTION__)); > + goto FatalError; > + } > + > + if (QemuFwCfgS3Enabled ()) { > + VOID *Registration; > + > + // > + // On S3 resume the above register settings have to be repeated. > Register a > + // protocol notify callback that, when boot script saving becomes > + // available, saves operations equivalent to the above to the boot > script. > + // > + Status = gBS->CreateEvent (EVT_NOTIFY_SIGNAL, TPL_CALLBACK, > + OnS3SaveStateInstalled, NULL /* Context */, > + &mS3SaveStateInstalled); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a: CreateEvent: %r\n", __FUNCTION__, Status)); > + goto FatalError; > + } > + > + Status = gBS->RegisterProtocolNotify (&gEfiS3SaveStateProtocolGuid, > + mS3SaveStateInstalled, &Registration); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a: RegisterProtocolNotify: %r\n", __FUNCTION__, > + Status)); > + goto ReleaseEvent; > + } > + > + // > + // Kick the event right now -- maybe the boot script is already saveable. > + // > + Status = gBS->SignalEvent (mS3SaveStateInstalled); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a: SignalEvent: %r\n", __FUNCTION__, Status)); > + goto ReleaseEvent; > + } > + } > + > + // > + // We have no pointers to convert to virtual addresses. The handle > + itself // doesn't matter, as protocol services are not accessible at > runtime. > + // > + Status = gBS->InstallMultipleProtocolInterfaces (&ImageHandle, > + &gEfiSmmControl2ProtocolGuid, &mControl2, > + NULL); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a: InstallMultipleProtocolInterfaces: %r\n", > + __FUNCTION__, Status)); > + goto ReleaseEvent; > + } > + > + return EFI_SUCCESS; > + > +ReleaseEvent: > + if (mS3SaveStateInstalled != NULL) { > + gBS->CloseEvent (mS3SaveStateInstalled); > + } > + > +FatalError: > + // > + // We really don't want to continue in this case. > + // > + // _ASSERT() is never compiled out, and it respects PcdDebugPropertyMask > (ie. > + // prevent further execution with CPU breakpoint vs. dead loop). > + // > + _ASSERT (FALSE); Should not use internal macros. If you need a CPU break point, then use BaseLib CpuBreakpoint () If you need a deadloop, then use BaseLib CpuDeadLoop () > + return EFI_UNSUPPORTED; > +} > + > +/** > + Notification callback for S3SaveState installation. > + > + @param[in] Event Event whose notification function is being invoked. > + > + @param[in] Context The pointer to the notification function's context, > which > + is implementation-dependent. > +**/ > +STATIC Recommend removing use of STATIC > +VOID > +EFIAPI > +OnS3SaveStateInstalled ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ) > +{ > + EFI_STATUS Status; > + EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState; > + UINT32 SmiEnOrMask, SmiEnAndMask; > + UINT16 GenPmCon1OrMask, GenPmCon1AndMask; > + > + ASSERT (Event == mS3SaveStateInstalled); > + > + Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, > + NULL /* Registration */, (VOID **)&S3SaveState); if > + (EFI_ERROR (Status)) { > + return; > + } > + > + // > + // These operations were originally done, verified and explained in > + the entry // point function of the driver. > + // > + SmiEnOrMask = ICH9_SMI_EN_APMC_EN | ICH9_SMI_EN_GBL_SMI_EN; > + SmiEnAndMask = MAX_UINT32; Status = S3SaveState->Write ( > + S3SaveState, > + EFI_BOOT_SCRIPT_IO_READ_WRITE_OPCODE, > + EfiBootScriptWidthUint32, > + (UINT64)mSmiEnable, > + &SmiEnOrMask, > + &SmiEnAndMask > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a: EFI_BOOT_SCRIPT_IO_READ_WRITE_OPCODE: %r\n", > + __FUNCTION__, Status)); > + _ASSERT (FALSE); > + } > + > + GenPmCon1OrMask = ICH9_GEN_PMCON_1_SMI_LOCK; GenPmCon1AndMask = > + MAX_UINT16; Status = S3SaveState->Write ( > + S3SaveState, > + EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE, > + EfiBootScriptWidthUint16, > + (UINT64)POWER_MGMT_REGISTER_Q35 (ICH9_GEN_PMCON_1), > + &GenPmCon1OrMask, > + &GenPmCon1AndMask > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, > + "%a: EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE: %r\n", __FUNCTION__, > + Status)); > + _ASSERT (FALSE); > + } > + > + DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", > +__FUNCTION__)); > + gBS->CloseEvent (Event); > + mS3SaveStateInstalled = NULL; > +} > -- > 1.8.3.1 > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
