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

2018-09-13 Thread Wang, Jian J 
Thanks for the explanation.

Regards,
Jian

From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Wednesday, September 12, 2018 6:04 PM
To: Wang, Jian J ; Zeng, Star ; 
edk2-devel@lists.01.org
Cc: You, Benjamin ; Dong, Eric 
Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

On 09/12/18 02:32, Wang, Jian J wrote:
> 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?

Separate patch in the same series should be fine.

IMO the second patch can refer to the same BZ, or not even refer to any
BZ at all.

Thanks,
Laszlo

>
> Regards,
> Jian
>
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, September 11, 2018 9:52 PM
> To: Zeng, Star mailto:star.z...@intel.com>>; Wang, Jian 
> J mailto:jian.j.w...@intel.com>>; 
> edk2-devel@lists.01.org
> Cc: You, Benjamin mailto:benjamin@intel.com>>; 
> Dong, Eric mailto:eric.d...@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 ();
>>   if (GuidHob != NULL) {
>> ...
>>   }
>>
>> With that, Reviewed-by: Star Zeng 
>> mailto:star.z...@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>
>> Cc: Zeng, Star 
>> mailto:star.z...@intel.com>>;
>>  You, Benjamin 
>> mailto:benjamin@intel.com>>;
>>  Dong, Eric 
>> mailto:eric.d...@intel.com>>;
>>  Laszlo Ersek 
>> mailto:ler...@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 
>> 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 | 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
>>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-09-12 Thread Laszlo Ersek 
On 09/12/18 02:49, Wang, Jian J wrote:
> Laszlo,
> 
> Thanks for the comments.
> 
> 
> (1)Agree. It’ll be updated in v2.
> 
> (2)DEBUG_ERROR level won’t print word “ERROR” on console. I think an 
> “ERROR”
> 
> word in log should get the attention more easily.
> 
> (3)How about log both of them? GUID value may be more friendly to log 
> parser but
> a GUID name is more friendly to person.
> 
> (4)Good idea. I’ll add it in v2.

Those work for me as well.

Thanks
Laszlo

> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, September 11, 2018 10:01 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; You, Benjamin ; 
> Dong, Eric 
> Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config 
> error
> 
> On 09/10/18 05:22, Jian J Wang wrote:
>> 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 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 | 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);
>>}
>>
>>//
>>
> 
> I have some superficial comments for this patch.
> 
> (1) in case the "if" has an "else" branch, I think it's better style to
> use "==" in the condition than "!=". To me,
> 
>   if (GuidHob != NULL) {
> //
> // do a bunch of stuff
> //
>   } else {
> //
> // error
> //
>   }
> 
> is more difficult to read than:
> 
>   if (GuidHob == NULL) {
> //
> // error
> //
>   } else {
> //
> // do a bunch of stuff
> //
>   }
> 
> in particular if the "bunch of stuff" includes nested "if" statements.
> 
> 
> (2) I think the error message could be improved. I'd suggest removing
> the word "ERROR", since DEBUG_ERROR already sets the error level / mask.
> 
> (3) Furthermore, I suggest not logging the name "gEfiAcpiVariableGuid"
> textually, as a string -- in edk2 we generally log GUIDs by value, and
> log parsers / translators usually look for GUID values. Thus I suggest
> using %g with 
> 
> (4) Please consider logging the module name and/or the function name
> too, with "%a", and gEfiCallerBaseName and/or __FUNCTION__.
> "gEfiCallerBaseName" is usually only helpful with libraries (because
> they can be linked into multiple drivers), but __FUNCTION__ is more
> frequently helpful.
> 
> Thanks
> Laszlo
> 

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

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

2018-09-12 Thread Laszlo Ersek 
On 09/12/18 02:32, Wang, Jian J wrote:
> 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?

Separate patch in the same series should be fine.

IMO the second patch can refer to the same BZ, or not even refer to any
BZ at all.

Thanks,
Laszlo

> 
> Regards,
> Jian
> 
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, September 11, 2018 9:52 PM
> To: Zeng, Star ; Wang, Jian J ; 
> edk2-devel@lists.01.org
> Cc: You, Benjamin ; Dong, Eric 
> 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 ();
>>   if (GuidHob != NULL) {
>> ...
>>   }
>>
>> With that, Reviewed-by: Star Zeng 
>> mailto:star.z...@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
>> Cc: Zeng, Star mailto:star.z...@intel.com>>; You, 
>> Benjamin mailto:benjamin@intel.com>>; Dong, Eric 
>> mailto:eric.d...@intel.com>>; Laszlo Ersek 
>> mailto:ler...@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 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 | 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
>>

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

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

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

Thanks for the comments.


(1)Agree. It’ll be updated in v2.

(2)DEBUG_ERROR level won’t print word “ERROR” on console. I think an “ERROR”

word in log should get the attention more easily.

(3)How about log both of them? GUID value may be more friendly to log 
parser but
a GUID name is more friendly to person.

(4)Good idea. I’ll add it in v2.

Regards,
Jian

From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Tuesday, September 11, 2018 10:01 PM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Zeng, Star ; You, Benjamin ; 
Dong, Eric 
Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error

On 09/10/18 05:22, Jian J Wang wrote:
> 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 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 | 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);
>}
>
>//
>

I have some superficial comments for this patch.

(1) in case the "if" has an "else" branch, I think it's better style to
use "==" in the condition than "!=". To me,

  if (GuidHob != NULL) {
//
// do a bunch of stuff
//
  } else {
//
// error
//
  }

is more difficult to read than:

  if (GuidHob == NULL) {
//
// error
//
  } else {
//
// do a bunch of stuff
//
  }

in particular if the "bunch of stuff" includes nested "if" statements.


(2) I think the error message could be improved. I'd suggest removing
the word "ERROR", since DEBUG_ERROR already sets the error level / mask.

(3) Furthermore, I suggest not logging the name "gEfiAcpiVariableGuid"
textually, as a string -- in edk2 we generally log GUIDs by value, and
log parsers / translators usually look for GUID values. Thus I suggest
using %g with 

(4) Please consider logging the module name and/or the function name
too, with "%a", and gEfiCallerBaseName and/or __FUNCTION__.
"gEfiCallerBaseName" is usually only helpful with libraries (because
they can be linked into multiple drivers), but __FUNCTION__ is more
frequently helpful.

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

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

2018-09-11 Thread Wang, Jian J 
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:ler...@redhat.com]
Sent: Tuesday, September 11, 2018 9:52 PM
To: Zeng, Star ; Wang, Jian J ; 
edk2-devel@lists.01.org
Cc: You, Benjamin ; Dong, Eric 
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 ();
>   if (GuidHob != NULL) {
> ...
>   }
>
> With that, Reviewed-by: Star Zeng 
> mailto:star.z...@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
> Cc: Zeng, Star mailto:star.z...@intel.com>>; You, 
> Benjamin mailto:benjamin@intel.com>>; Dong, Eric 
> mailto:eric.d...@intel.com>>; Laszlo Ersek 
> mailto:ler...@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 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 | 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
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2018-09-11 Thread Laszlo Ersek 
On 09/10/18 05:22, Jian J Wang wrote:
> 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 
> 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 | 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);
>}
>  
>//
> 

I have some superficial comments for this patch.

(1) in case the "if" has an "else" branch, I think it's better style to
use "==" in the condition than "!=". To me,

  if (GuidHob != NULL) {
//
// do a bunch of stuff
//
  } else {
//
// error
//
  }

is more difficult to read than:

  if (GuidHob == NULL) {
//
// error
//
  } else {
//
// do a bunch of stuff
//
  }

in particular if the "bunch of stuff" includes nested "if" statements.


(2) I think the error message could be improved. I'd suggest removing
the word "ERROR", since DEBUG_ERROR already sets the error level / mask.

(3) Furthermore, I suggest not logging the name "gEfiAcpiVariableGuid"
textually, as a string -- in edk2 we generally log GUIDs by value, and
log parsers / translators usually look for GUID values. Thus I suggest
using %g with 

(4) Please consider logging the module name and/or the function name
too, with "%a", and gEfiCallerBaseName and/or __FUNCTION__.
"gEfiCallerBaseName" is usually only helpful with libraries (because
they can be linked into multiple drivers), but __FUNCTION__ is more
frequently helpful.

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

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

2018-09-11 Thread Laszlo Ersek 
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 ();
>   if (GuidHob != NULL) {
> ...
>   }
> 
> With that, Reviewed-by: Star Zeng 

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
> Cc: Zeng, Star ; You, Benjamin ; 
> Dong, Eric ; Laszlo Ersek 
> 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 
> 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 | 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
> 

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

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

2018-09-09 Thread Wang, Jian J 
Make sense to me.

Regards,
Jian


> -Original Message-
> From: Zeng, Star
> Sent: Monday, September 10, 2018 1:08 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: You, Benjamin ; Dong, Eric
> ; Laszlo Ersek ; Zeng, Star
> 
> Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3
> config error
> 
> I agree to add the ASSERT, but even with the ASSERT, I still suggest moving
> //
> // Patch SmmS3ResumeState->SmmS3Cr3
> //
> InitSmmS3Cr3 ();
> 
> into
>   GuidHob = GetFirstGuidHob ();
>   if (GuidHob != NULL) {
> ...
>   }
> 
> With that, Reviewed-by: Star Zeng 
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Wang, Jian J
> Sent: Monday, September 10, 2018 11:22 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; You, Benjamin
> ; Dong, Eric ; Laszlo Ersek
> 
> 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 
> 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 | 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

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

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

2018-09-09 Thread Zeng, Star 
I agree to add the ASSERT, but even with the ASSERT, I still suggest moving 
//
// Patch SmmS3ResumeState->SmmS3Cr3
//
InitSmmS3Cr3 ();

into
  GuidHob = GetFirstGuidHob ();
  if (GuidHob != NULL) {
...
  }

With that, Reviewed-by: Star Zeng 


Thanks,
Star
-Original Message-
From: Wang, Jian J 
Sent: Monday, September 10, 2018 11:22 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; You, Benjamin ; 
Dong, Eric ; Laszlo Ersek 
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 
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 | 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

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

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

2018-09-09 Thread Jian J Wang 
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 
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 | 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

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