Re: [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI.

[Laszlo Ersek <lersek@redhat.com>]

Hi Eric,

On 10/25/17 09:58, Eric Dong wrote:
> In current implementation, CPU initialized can be done in PEI
> or DXE phase. PEI uses CpuFeaturesPie and Dxe uses CpuFeaturesDxe.

(1) "CpuFeaturesPie" has a typo

> If CPU initialized in PEI phase, CpuFeaturesDxe driver will
> not be used. This driver will install gEdkiiCpuFeaturesInitDoneGuid
> protocol after it initializes the CPU.
> 
> Some drivers depend on this protocol to dispatch themselves. If
> CpuFeaturesDxe not been used, these drivers will not be dispatched.
> 
> This patch fix the above issue. If Cpu initialized in PEI
> phase, it also report a guid HOB for CpuFeaturesDxe.
> CpuFeaturesDxe will check this HOB first. If it found this
> HOB, it just install gEdkiiCpuFeaturesInitDoneGuid protocol,
> else it will also do the CPU initialization.
> 
> Cc: Ruiyu Ni <ruiyu.ni@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/CpuFeatures/CpuFeaturesDxe.c   | 19 +++++++++++++++++++
>  UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf |  1 +
>  UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c   |  6 ++++++
>  UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf |  1 +
>  4 files changed, 27 insertions(+)
> 
> diff --git a/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c b/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c
> index b0b186d..6c0ae9d 100644
> --- a/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c
> +++ b/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c
> @@ -19,6 +19,7 @@
>  #include <Library/UefiLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/RegisterCpuFeaturesLib.h>
> +#include <Library/HobLib.h>
>  
>  #include <Protocol/SmmConfiguration.h>
>  #include <Guid/CpuFeaturesInitDone.h>
> @@ -101,6 +102,24 @@ CpuFeaturesDxeInitialize (
>    )
>  {
>    VOID        *Registration;
> +  EFI_STATUS  Status;
> +  EFI_HANDLE  Handle;
> +
> +  if (GetFirstGuidHob (&gEdkiiCpuFeaturesInitDoneGuid) != NULL) {
> +    //
> +    // Try to find HOB first. This HOB exist means CPU features have 
> +    // been initialized by CpuFeaturesPei driver, just install gEdkiiCpuFeaturesInitDoneGuid.

(2) I think this line is too long; I suggest to wrap it to 80 characters.

> +    //
> +    Handle = NULL;
> +    Status = gBS->InstallProtocolInterface (
> +                    &Handle,
> +                    &gEdkiiCpuFeaturesInitDoneGuid,
> +                    EFI_NATIVE_INTERFACE,
> +                    NULL
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +    return EFI_SUCCESS;
> +  }

(3) Can we remove the ASSERT_EFI_ERROR() and just return Status here?

No resources would be leaked, and the driver exit status would reflect
the failure.

(4) Just a question (not even a suggestion): can we perhaps install the
protocol on ImageHandle as well? This way we could avoid allocating a
new handle.

>  
>    if (PcdGetBool (PcdCpuFeaturesInitAfterSmmRelocation)) {
>      //
> diff --git a/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf b/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf
> index 175e8a9..b1733be 100644
> --- a/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf
> +++ b/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf
> @@ -33,6 +33,7 @@
>    UefiDriverEntryPoint
>    UefiBootServicesTableLib
>    RegisterCpuFeaturesLib
> +  HobLib
>  
>  [Sources]
>    CpuFeaturesDxe.c
> diff --git a/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c b/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c
> index b052d55..72ee19b 100644
> --- a/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c
> +++ b/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c
> @@ -18,6 +18,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/PeiServicesLib.h>
>  #include <Library/RegisterCpuFeaturesLib.h>
> +#include <Library/HobLib.h>
>  
>  #include <Guid/CpuFeaturesInitDone.h>
>  
> @@ -70,6 +71,11 @@ CpuFeaturesPeimInitialize (
>    Status = PeiServicesInstallPpi(&mPeiCpuFeaturesInitDonePpiDesc);
>    ASSERT_EFI_ERROR (Status);
>  
> +  //
> +  // Build HOB to let CpuFeatureDxe driver skip the initialization process.
> +  //
> +  BuildGuidHob (&gEdkiiCpuFeaturesInitDoneGuid, 0);
> +
>    return EFI_SUCCESS;
>  }
>  
> diff --git a/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf b/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf
> index dd4b388..e617c5b 100644
> --- a/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf
> +++ b/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf
> @@ -32,6 +32,7 @@
>    PeimEntryPoint
>    PeiServicesLib
>    RegisterCpuFeaturesLib
> +  HobLib
>  
>  [Sources]
>    CpuFeaturesPei.c
> 

I think (1) and (2) should really be fixed -- but you can fix those when
you push the patch.

I don't insist on (3) and (4) -- if you don't want to change those
parts, I'm fine with that too.

With (1) and (2) updated:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo