Laszlo,
I other modules where we want to halt no matter what, the following 2
statements are used together. ASSERT() could be removed by PCD settings, so
dead loop catches that case. If ASSERT() is enabled, then the ASSERT()
behavior of BP or dead loop is controlled by PCD.
ASSERT (FALSE);
CpuDeadLoop ();
Thanks,
Mike
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Monday, November 23, 2015 4:56 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-de...@ml01.01.org
> Subject: Re: [edk2] [PATCH v4 09/41] OvmfPkg: implement
> EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER
>
> On 11/21/15 07:17, Kinney, Michael D wrote:
> > Laszlo,
> >
> > Minor comments included below.
> >
> > Reviewed-by: Michael Kinney <michael.d.kin...@intel.com>
>
> Thank you. I'll pick up this tag. Answers and further questions below:
>
> >
> > 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
>
> Will fix.
>
> >
> >> + gEfiSmmControl2ProtocolGuid # PROTOCOL ALWAYS_PRODUCED
> >
> > Comment should be: ## PRODUCES
>
> Ditto, thanks.
>
> >
> >> +
> >> +[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
>
> I'd like to stick with it in OvmfPkg.
>
> >
> >> +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
>
> I can move it to the top and assign the struct in code, if you'd like me to
> do that.
>
> >
> >> +
> >> +//
> >> +// 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
>
> I wanted to keep it close to the reference to it, just below in
> SmmControl2DxeEntryPoint(), but I will now move it near the top.
> Thanks.
>
> >
> >> +
> >> +//
> >> +// 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 ()
>
> That's the thing -- I don't know (and don't want to know) what I need;
> PcdDebugPropertyMask decides about that. Whichever is
> chosen however, that should never be compiled out of the code. So _ASSERT()
> does exactly what I'd like to have.
>
> Do you recommend to open-code the following instead?
>
> if ((PcdGet8 (PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> CpuBreakpoint ();
> }
> if ((PcdGet8(PcdDebugPropertyMask) &
> DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
> CpuDeadLoop ();
> }
>
> Thanks!
> Laszlo
>
> >
> >> + 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
