* [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
@ 2018-09-10 3:22 Jian J Wang
2018-09-10 5:07 ` Zeng, Star
2018-09-11 14:00 ` Laszlo Ersek
0 siblings, 2 replies; 10+ messages in thread
From: Jian J Wang @ 2018-09-10 3:22 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Benjamin You, Eric Dong, Laszlo Ersek
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>
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 | 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-10 3:22 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error Jian J Wang
@ 2018-09-10 5:07 ` Zeng, Star
2018-09-10 5:35 ` Wang, Jian J
2018-09-11 13:52 ` Laszlo Ersek
2018-09-11 14:00 ` Laszlo Ersek
1 sibling, 2 replies; 10+ messages in thread
From: Zeng, Star @ 2018-09-10 5:07 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: You, Benjamin, Dong, Eric, Laszlo Ersek, Zeng, Star
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>
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 <star.zeng@intel.com>; You, Benjamin <benjamin.you@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <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>
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 | 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-10 5:07 ` Zeng, Star
@ 2018-09-10 5:35 ` Wang, Jian J
2018-09-11 13:52 ` Laszlo Ersek
1 sibling, 0 replies; 10+ messages in thread
From: Wang, Jian J @ 2018-09-10 5:35 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org
Cc: You, Benjamin, Dong, Eric, Laszlo Ersek
Make sense to me.
Regards,
Jian
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, September 10, 2018 1:08 PM
> To: 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>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> <star.zeng@intel.com>
> 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 (&gEfiAcpiVariableGuid);
> if (GuidHob != NULL) {
> ...
> }
>
> With that, Reviewed-by: Star Zeng <star.zeng@intel.com>
>
>
> 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 <star.zeng@intel.com>; You, Benjamin
> <benjamin.you@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> <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>
> 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 | 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-10 5:07 ` Zeng, Star
2018-09-10 5:35 ` Wang, Jian J
@ 2018-09-11 13:52 ` Laszlo Ersek
2018-09-12 0:32 ` Wang, Jian J
1 sibling, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2018-09-11 13:52 UTC (permalink / raw)
To: Zeng, Star, Wang, Jian J, edk2-devel@lists.01.org
Cc: You, Benjamin, Dong, Eric
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>
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 <star.zeng@intel.com>; You, Benjamin <benjamin.you@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <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>
> 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 | 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-10 3:22 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error Jian J Wang
2018-09-10 5:07 ` Zeng, Star
@ 2018-09-11 14:00 ` Laszlo Ersek
2018-09-12 0:49 ` Wang, Jian J
1 sibling, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2018-09-11 14:00 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Star Zeng, Benjamin You, Eric Dong
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 <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 | 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 &gEfiAcpiVariableGuid.
(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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-11 13:52 ` Laszlo Ersek
@ 2018-09-12 0:32 ` Wang, Jian J
2018-09-12 10:03 ` Laszlo Ersek
0 siblings, 1 reply; 10+ messages in thread
From: Wang, Jian J @ 2018-09-12 0:32 UTC (permalink / raw)
To: Laszlo Ersek, Zeng, Star, edk2-devel@lists.01.org
Cc: You, Benjamin, Dong, Eric
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-11 14:00 ` Laszlo Ersek
@ 2018-09-12 0:49 ` Wang, Jian J
2018-09-12 10:04 ` Laszlo Ersek
0 siblings, 1 reply; 10+ messages in thread
From: Wang, Jian J @ 2018-09-12 0:49 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Zeng, Star, You, Benjamin, Dong, Eric
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:lersek@redhat.com]
Sent: Tuesday, September 11, 2018 10:01 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; 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 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 <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);
> }
>
> //
>
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 &gEfiAcpiVariableGuid.
(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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-12 0:32 ` Wang, Jian J
@ 2018-09-12 10:03 ` Laszlo Ersek
2018-09-13 11:03 ` Wang, Jian J
0 siblings, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2018-09-12 10:03 UTC (permalink / raw)
To: Wang, Jian J, Zeng, Star, edk2-devel@lists.01.org
Cc: You, Benjamin, Dong, Eric
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: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
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-12 0:49 ` Wang, Jian J
@ 2018-09-12 10:04 ` Laszlo Ersek
0 siblings, 0 replies; 10+ messages in thread
From: Laszlo Ersek @ 2018-09-12 10:04 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Zeng, Star, You, Benjamin, Dong, Eric
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:lersek@redhat.com]
> Sent: Tuesday, September 11, 2018 10:01 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; 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 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 <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);
>> }
>>
>> //
>>
>
> 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 &gEfiAcpiVariableGuid.
>
> (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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error
2018-09-12 10:03 ` Laszlo Ersek
@ 2018-09-13 11:03 ` Wang, Jian J
0 siblings, 0 replies; 10+ messages in thread
From: Wang, Jian J @ 2018-09-13 11:03 UTC (permalink / raw)
To: Laszlo Ersek, Zeng, Star, edk2-devel@lists.01.org
Cc: You, Benjamin, Dong, Eric
Thanks for the explanation.
Regards,
Jian
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, September 12, 2018 6:04 PM
To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star <star.zeng@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/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:lersek@redhat.com]
> Sent: Tuesday, September 11, 2018 9:52 PM
> To: Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto: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<mailto:star.zeng@intel.com%3cmailto: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<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>> Cc: Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com<mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com>>>; You, Benjamin <benjamin.you@intel.com<mailto:benjamin.you@intel.com<mailto:benjamin.you@intel.com%3cmailto:benjamin.you@intel.com>>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com<mailto:eric.dong@intel.com%3cmailto:eric.dong@intel.com>>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto: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<mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com>>>
>> Cc: Benjamin You <benjamin.you@intel.com<mailto:benjamin.you@intel.com<mailto:benjamin.you@intel.com%3cmailto:benjamin.you@intel.com>>>
>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com<mailto:eric.dong@intel.com%3cmailto:eric.dong@intel.com>>>
>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto: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<mailto:jian.j.wang@intel.com%3cmailto: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
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-13 11:03 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-10 3:22 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error Jian J Wang
2018-09-10 5:07 ` Zeng, Star
2018-09-10 5:35 ` Wang, Jian J
2018-09-11 13:52 ` Laszlo Ersek
2018-09-12 0:32 ` Wang, Jian J
2018-09-12 10:03 ` Laszlo Ersek
2018-09-13 11:03 ` Wang, Jian J
2018-09-11 14:00 ` Laszlo Ersek
2018-09-12 0:49 ` Wang, Jian J
2018-09-12 10:04 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox