public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI.
@ 2017-10-25  7:58 Eric Dong
  2017-10-25 14:11 ` Ni, Ruiyu
  2017-10-25 14:43 ` Laszlo Ersek
  0 siblings, 2 replies; 4+ messages in thread
From: Eric Dong @ 2017-10-25  7:58 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek

In current implementation, CPU initialized can be done in PEI
or DXE phase. PEI uses CpuFeaturesPie and Dxe uses CpuFeaturesDxe.
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.
+    //
+    Handle = NULL;
+    Status = gBS->InstallProtocolInterface (
+                    &Handle,
+                    &gEdkiiCpuFeaturesInitDoneGuid,
+                    EFI_NATIVE_INTERFACE,
+                    NULL
+                    );
+    ASSERT_EFI_ERROR (Status);
+    return EFI_SUCCESS;
+  }
 
   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
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI.
  2017-10-25  7:58 [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI Eric Dong
@ 2017-10-25 14:11 ` Ni, Ruiyu
  2017-10-25 14:43 ` Laszlo Ersek
  1 sibling, 0 replies; 4+ messages in thread
From: Ni, Ruiyu @ 2017-10-25 14:11 UTC (permalink / raw)
  To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-----Original Message-----
From: Dong, Eric 
Sent: Wednesday, October 25, 2017 3:59 PM
To: edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI.

In current implementation, CPU initialized can be done in PEI or DXE phase. PEI uses CpuFeaturesPie and Dxe uses CpuFeaturesDxe.
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.
+    //
+    Handle = NULL;
+    Status = gBS->InstallProtocolInterface (
+                    &Handle,
+                    &gEdkiiCpuFeaturesInitDoneGuid,
+                    EFI_NATIVE_INTERFACE,
+                    NULL
+                    );
+    ASSERT_EFI_ERROR (Status);
+    return EFI_SUCCESS;
+  }
 
   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
--
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI.
  2017-10-25  7:58 [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI Eric Dong
  2017-10-25 14:11 ` Ni, Ruiyu
@ 2017-10-25 14:43 ` Laszlo Ersek
  2017-10-26  2:25   ` Dong, Eric
  1 sibling, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2017-10-25 14:43 UTC (permalink / raw)
  To: Eric Dong, edk2-devel; +Cc: Ruiyu Ni

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI.
  2017-10-25 14:43 ` Laszlo Ersek
@ 2017-10-26  2:25   ` Dong, Eric
  0 siblings, 0 replies; 4+ messages in thread
From: Dong, Eric @ 2017-10-26  2:25 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu

Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 25, 2017 10:43 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in
> PEI.
> 
> 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?

I think we can return Status instead of EFI_SUCCESS here. I will update it when
I push the code.

We assume this installation will always return success, So we add assert code 
Here. This assert code can help to catch the unexpected behavior as early as 
possible.  You can find many similar case in edkii code base.

Thanks,
Eric

> 
> 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.

This code is also been used in CpuFeaturesInitializeWorker.  I prefer to follow 
Same rule to install the protocol.

Thanks,
Eric 

> 
> >
> >    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.
> 

Will do change for 1 and 2 when push the code.

Thanks,
Eric

> 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-10-26  2:21 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-25  7:58 [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI Eric Dong
2017-10-25 14:11 ` Ni, Ruiyu
2017-10-25 14:43 ` Laszlo Ersek
2017-10-26  2:25   ` Dong, Eric

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox