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 <star.z...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org Cc: You, Benjamin <benjamin....@intel.com>; Dong, Eric <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 (&gEfiAcpiVariableGuid); > if (GuidHob != NULL) { > ... > } > > With that, Reviewed-by: Star Zeng > <star.z...@intel.com<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<mailto:edk2-devel@lists.01.org> > Cc: Zeng, Star <star.z...@intel.com<mailto:star.z...@intel.com>>; You, > Benjamin <benjamin....@intel.com<mailto:benjamin....@intel.com>>; Dong, Eric > <eric.d...@intel.com<mailto:eric.d...@intel.com>>; Laszlo Ersek > <ler...@redhat.com<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 <star.z...@intel.com<mailto:star.z...@intel.com>> > Cc: Benjamin You <benjamin....@intel.com<mailto:benjamin....@intel.com>> > Cc: Eric Dong <eric.d...@intel.com<mailto:eric.d...@intel.com>> > Cc: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > <jian.j.w...@intel.com<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
