[v1] UefiCpuPkg/S3Resume2Pei: Handle Communicate Ppi not exist issue.

[edk2] [Patch] UefiCpuPkg/S3Resume2Pei: Handle Communicate Ppi not exist issue.

Posted by Eric Dong 7 years, 2 months ago

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 <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 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
-- 
2.7.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: Handle Communicate Ppi not exist issue.

Posted by Laszlo Ersek 7 years, 2 months ago

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 <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  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 <lersek@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: Handle Communicate Ppi not exist issue.

Posted by Dong, Eric 7 years, 2 months ago

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, October 12, 2017 5:43 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: Handle
> Communicate Ppi not exist issue.
> 
> 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 <ruiyu.ni@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  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 <lersek@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> 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".

This is an expect behavior. This code is executive right after EndOfPei point, not in SMM environment.
It just tries to use Communicate Ppi to send EndOfS3Resume signal to SmmCore.

> 
> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: Handle Communicate Ppi not exist issue.

Posted by Zeng, Star 7 years, 2 months ago

  //
  // Attempt to use content from SMRAM first
  //
  GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid);
  if (GuidHob != NULL) {

The code may could use this condition above to skip SignalEndOfS3Resume().
We can see two places already in S3Resume.c.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
Sent: Thursday, October 12, 2017 5:55 PM
To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: Handle Communicate Ppi not exist issue.

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, October 12, 2017 5:43 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: Handle 
> Communicate Ppi not exist issue.
> 
> 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 <ruiyu.ni@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  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 <lersek@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> 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".

This is an expect behavior. This code is executive right after EndOfPei point, not in SMM environment.
It just tries to use Communicate Ppi to send EndOfS3Resume signal to SmmCore.

> 
> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel