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

["Zeng, Star" <star.zeng@intel.com>]

Same suggestion with Laszlo.

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
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>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <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>

Thanks
Laszlo