From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 C44BD21F3883A for ; Thu, 12 Oct 2017 02:39:23 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9A1F8806B3; Thu, 12 Oct 2017 09:42:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9A1F8806B3 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-92.rdu2.redhat.com [10.10.120.92]) by smtp.corp.redhat.com (Postfix) with ESMTP id 707F660632; Thu, 12 Oct 2017 09:42:52 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni , Jiewen Yao References: <1507797478-13028-1-git-send-email-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <5842b1ee-80f3-1d66-0b2d-38e392dd5284@redhat.com> Date: Thu, 12 Oct 2017 11:42:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1507797478-13028-1-git-send-email-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 12 Oct 2017 09:42:53 +0000 (UTC) Subject: Re: [Patch] UefiCpuPkg/S3Resume2Pei: Handle Communicate Ppi not exist issue. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Oct 2017 09:39:24 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Eric, On 10/12/17 10:37, Eric Dong wrote: > Current code assume Communicate Ppi always existed, so it adds > ASSERT to confirm it. Ovmf platform happened not has this Ppi, so > the ASSERT been trig. This patch handle Ppi not existed case. > > Cc: Ruiyu Ni > Cc: Jiewen Yao > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > index c2171cb..e0c2d36 100644 > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > @@ -465,7 +465,7 @@ SignalEndOfS3Resume ( > SMM_COMMUNICATE_HEADER_64 Header64; > VOID *CommBuffer; > > - DEBUG ((EFI_D_INFO, "SignalEndOfS3Resume - Enter\n")); > + DEBUG ((DEBUG_INFO, "SignalEndOfS3Resume - Enter\n")); > > // > // This buffer consumed in DXE phase, so base on DXE mode to prepare communicate buffer. > @@ -484,29 +484,27 @@ SignalEndOfS3Resume ( > } > CopyGuid (CommBuffer, &gEdkiiSmmEndOfS3ResumeProtocolGuid); > > - // > - // Get needed resource > - // > Status = PeiServicesLocatePpi ( > &gEfiPeiSmmCommunicationPpiGuid, > 0, > NULL, > (VOID **)&SmmCommunicationPpi > ); > - ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Locate Smm Communicate Ppi failed (%r)!\n", Status)); > + return Status; > + } > > - // > - // Send command > - // > Status = SmmCommunicationPpi->Communicate ( > SmmCommunicationPpi, > (VOID *)CommBuffer, > &CommSize > ); > - ASSERT_EFI_ERROR (Status); > - > - DEBUG ((EFI_D_INFO, "SignalEndOfS3Resume - Exit (%r)\n", Status)); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "SmmCommunicationPpi->Communicate return failure (%r)!\n", Status)); > + } > > + DEBUG ((DEBUG_INFO, "SignalEndOfS3Resume - Exit (%r)\n", Status)); > return Status; > } > > @@ -587,8 +585,7 @@ S3ResumeBootOs ( > // > // Signal EndOfS3Resume event. > // > - Status = SignalEndOfS3Resume (); > - ASSERT_EFI_ERROR (Status); > + SignalEndOfS3Resume (); > > // > // report status code on S3 resume > Thanks for the quick patch! Tested-by: Laszlo Ersek Reviewed-by: Laszlo Ersek I used OVMF IA32 and IA32X64 to test the patch (with SMM). I also checked S3 with the OVMF X64 build (without SMM). It works too. But, interestingly, even in that build, I'm getting a message Locate Smm Communicate Ppi failed (Not Found)! which seems strange for a firmware that does not include any SMM support. This tells me that the original ASSERT (which is removed by this patch) would have triggered in the OVMF X64 build (*without* SMM) -- I didn't try that originally, but in retrospect that's what I believe. So, my question is; is this intentional? If I remember correctly, other parts of S3Resume2Pei distinguish explicitly between "no-SMM" and "SMM". If this new facility is meaningful only for SMM, then should we avoid even the PeiServicesLocatePpi() call in SignalEndOfS3Resume(), in case the platform lacks SMM support? Thanks, Laszlo