From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 613242194D387 for ; Wed, 12 Sep 2018 03:03:55 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7E58387A7D; Wed, 12 Sep 2018 10:03:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-31.rdu2.redhat.com [10.10.120.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7C7C710EE6D7; Wed, 12 Sep 2018 10:03:53 +0000 (UTC) To: "Wang, Jian J" , "Zeng, Star" , "edk2-devel@lists.01.org" Cc: "You, Benjamin" , "Dong, Eric" References: <20180910032225.10044-1-jian.j.wang@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BBB719C@shsmsx102.ccr.corp.intel.com> <7a176246-7816-042a-d7a8-ba5b36dd47da@redhat.com> From: Laszlo Ersek Message-ID: <0c374a61-8acc-ebea-6fcd-b7873f21b5f2@redhat.com> Date: Wed, 12 Sep 2018 12:03:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 12 Sep 2018 10:03:54 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 12 Sep 2018 10:03:54 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: add message for S3 config error X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Sep 2018 10:03:55 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 ; 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 (&gEfiAcpiVariableGuid); >> 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 >>