[PATCH V2 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

[PATCH V2 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

[Jian J Wang]

> v2
>   a. refine the error message to be more useful
>   b. improve the code logic

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. This patch series try
to fix this problem by a useful message.

Jian J Wang (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
  UefiCpuPkg/PiSmmCpuDxeSmm: move InitSmmS3Cr3() into else block

 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
2.16.2.windows.1

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

[Jian J Wang]

> 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));
-- 
2.16.2.windows.1

[PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: move InitSmmS3Cr3() into else block

[Jian J Wang]

> v2:
>    Move InitSmmS3Cr3() into 'else' block because it needs to access
>    SmmS3ResumeState

BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165

InitSmmS3Cr3 () will update SmmS3ResumeState so moving the calling of
it into else block to keep the logic consistency.

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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 0f6b6ef587..9198fa4e4c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -750,12 +750,12 @@ InitSmmS3ResumeState (
     if (sizeof (UINTN) == sizeof (UINT32)) {
       SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
     }
-  }
 
-  //
-  // Patch SmmS3ResumeState->SmmS3Cr3
-  //
-  InitSmmS3Cr3 ();
+    //
+    // Patch SmmS3ResumeState->SmmS3Cr3
+    //
+    InitSmmS3Cr3 ();
+  }
 
   //
   // Allocate safe memory in ACPI NVS for AP to execute hlt loop in
-- 
2.16.2.windows.1

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

[Laszlo Ersek]

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

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

[Dong, Eric]

Hi Jian,

After update with Laszlo's comments, This serial Reviewed-by: Eric Dong <eric.dong@intel.com>

Thanks,
Eric

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> Sent: Friday, September 14, 2018 3:12 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH V2 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: add
> message for S3 config error
> 
> > v2
> >   a. refine the error message to be more useful
> >   b. improve the code logic
> 
> 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. This patch series try to fix this problem by a
> useful message.
> 
> Jian J Wang (2):
>   UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
>   UefiCpuPkg/PiSmmCpuDxeSmm: move InitSmmS3Cr3() into else block
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> --
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Wang, Jian J]

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

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

[Zeng, Star]

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

Re: [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: move InitSmmS3Cr3() into else block

[Zeng, Star]

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

-----Original Message-----
From: Wang, Jian J 
Sent: Friday, September 14, 2018 3:12 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; You, Benjamin <benjamin.you@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: move InitSmmS3Cr3() into else block

> v2:
>    Move InitSmmS3Cr3() into 'else' block because it needs to access
>    SmmS3ResumeState

BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1165

InitSmmS3Cr3 () will update SmmS3ResumeState so moving the calling of it into else block to keep the logic consistency.

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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 0f6b6ef587..9198fa4e4c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -750,12 +750,12 @@ InitSmmS3ResumeState (
     if (sizeof (UINTN) == sizeof (UINT32)) {
       SmmS3ResumeState->Signature = SMM_S3_RESUME_SMM_32;
     }
-  }
 
-  //
-  // Patch SmmS3ResumeState->SmmS3Cr3
-  //
-  InitSmmS3Cr3 ();
+    //
+    // Patch SmmS3ResumeState->SmmS3Cr3
+    //
+    InitSmmS3Cr3 ();
+  }
 
   //
   // Allocate safe memory in ACPI NVS for AP to execute hlt loop in
--
2.16.2.windows.1