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

2018-09-16 Thread Zeng, Star 
Same suggestion with Laszlo.

Reviewed-by: Star Zeng 

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Saturday, September 15, 2018 4:09 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Dong, Eric ; Zeng, Star 
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 
> Cc: Benjamin You 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  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 

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-09-16 Thread Wang, Jian J 
Laszlo,

Thanks. I’ll update it before check in.

Regards,
Jian

From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Saturday, September 15, 2018 4:09 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Dong, Eric ; Zeng, Star 
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 mailto:star.z...@intel.com>>
> Cc: Benjamin You mailto:benjamin@intel.com>>
> Cc: Eric Dong mailto:eric.d...@intel.com>>
> Cc: Laszlo Ersek mailto:ler...@redhat.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> mailto:jian.j.w...@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 mailto:ler...@redhat.com>>

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-09-14 Thread 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 
> Cc: Benjamin You 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  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 

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-09-14 Thread 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 
Cc: Benjamin You 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 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

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel