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

[Laszlo Ersek <lersek@redhat.com>]

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