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

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

Laszlo,

Thanks for the comment. I think it’ll ok to add it in a separate patch.

Just a little confuse about “a separate patch”. Does it mean a separate patch file
in the same patch series or a separate patch which needs a separate BZ tracker?

Regards,
Jian

From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, September 11, 2018 9:52 PM
To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: You, Benjamin <benjamin.you@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

On 09/10/18 07:07, Zeng, Star wrote:
> I agree to add the ASSERT, but even with the ASSERT, I still suggest moving
>     //
>     // Patch SmmS3ResumeState->SmmS3Cr3
>     //
>     InitSmmS3Cr3 ();
>
> into
>   GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid);
>   if (GuidHob != NULL) {
>     ...
>   }
>
> With that, Reviewed-by: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>

I think that's a valid idea, but shouldn't it be done in a separate
patch? One patch for the assert, and another moving InitSmmS3Cr3() under
the right condition. Does that sound OK?

Thanks
Laszlo


>
>
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Monday, September 10, 2018 11:22 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Subject: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
>
> 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 ASSERT is added by 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 | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index abd8a5a07b..f371667c44 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -744,6 +744,9 @@ InitSmmS3ResumeState (
>      if (sizeof (UINTN) == sizeof (UINT32)) {
>        SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
>      }
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "ERROR: HOB(gEfiAcpiVariableGuid) needed by S3 resume doesn't exist!\n"));
> +    ASSERT (FALSE);
>    }
>
>    //
> --
> 2.16.2.windows.1
>