Re: [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

["Wang, Jian J" <jian.j.wang@intel.com>]

Laszlo,

Thanks. I’ll update it before check in.

Regards,
Jian

From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Saturday, September 15, 2018 4:09 AM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

On 09/14/18 09:11, Jian J Wang wrote:
>> v2
>>   a. Refine the error message
>>   b. Use CpuDeadLoop to replace ASSERT(FALSE) for release build
>
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165
>
> HOB gEfiAcpiVariableGuid is a must have data for S3 resume if
> PcdAcpiS3Enable is set to TRUE. Current code in CpuS3.c doesn't
> embody this strong binding between them. An error message and
> CpuDeadLoop are added in this patch to warn platform developer
> about it.
>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Cc: Benjamin You <benjamin.you@intel.com<mailto:benjamin.you@intel.com>>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index abd8a5a07b..0f6b6ef587 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -714,7 +714,13 @@ InitSmmS3ResumeState (
>    }
>
>    GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid);
> -  if (GuidHob != NULL) {
> +  if (GuidHob == NULL) {
> +    DEBUG ((DEBUG_ERROR,
> +            "ERROR:%a(): HOB(gEfiAcpiVariableGuid=%g) needed by S3 resume doesn't exist!\n",
> +            __FUNCTION__,
> +            &gEfiAcpiVariableGuid));
> +    CpuDeadLoop ();
> +  } else {
>      SmramDescriptor = (EFI_SMRAM_DESCRIPTOR *) GET_GUID_HOB_DATA (GuidHob);
>
>      DEBUG ((EFI_D_INFO, "SMM S3 SMRAM Structure = %x\n", SmramDescriptor));
>

The indentation of the DEBUG macro invocation is not idiomatic. It should be:

    DEBUG ((
      DEBUG_ERROR,
      "ERROR:%a(): HOB(gEfiAcpiVariableGuid=%g) needed by S3 resume doesn't exist!\n",
      __FUNCTION__,
      &gEfiAcpiVariableGuid
      ));

It's OK with me if you fix that up before you push the series. (Please wait for Eric's and Star's reviews as well.)

With that update:

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>

Thanks
Laszlo