Thanks for the explanation. Regards, Jian
From: Laszlo Ersek [mailto:[email protected]] Sent: Wednesday, September 12, 2018 6:04 PM To: Wang, Jian J <[email protected]>; Zeng, Star <[email protected]>; [email protected] Cc: You, Benjamin <[email protected]>; Dong, Eric <[email protected]> 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:[email protected]] > Sent: Tuesday, September 11, 2018 9:52 PM > To: Zeng, Star <[email protected]<mailto:[email protected]>>; Wang, Jian > J <[email protected]<mailto:[email protected]>>; > [email protected]<mailto:[email protected]> > Cc: You, Benjamin <[email protected]<mailto:[email protected]>>; > Dong, Eric <[email protected]<mailto:[email protected]>> > 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 >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>> > > 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: >> [email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>> >> Cc: Zeng, Star >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>>; >> You, Benjamin >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>>; >> Dong, Eric >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>>; >> Laszlo Ersek >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>> >> 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 >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>> >> Cc: Benjamin You >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>> >> Cc: Eric Dong >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>> >> Cc: Laszlo Ersek >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Jian J Wang >> <[email protected]<mailto:[email protected]<mailto:[email protected]%3cmailto:[email protected]>>> >> --- >> 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
