On 11/24/15 08:06, Kinney, Michael D wrote:
> 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 ();

Perfect. I'll replace all uses of _ASSERT() in the series, with the
above pattern.

Thank you!
Laszlo

> 
> 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

Reply via email to