Re: [edk2] [PATCH 2/3] MdeModulePkg/PeiMain: Support EFI_PEI_CORE_FV_LOCATION_PPI

2019-02-14 Thread Chiu, Chasel 



> -Original Message-
> From: Ni, Ray
> Sent: Thursday, February 14, 2019 4:20 PM
> To: Chiu, Chasel ; edk2-devel@lists.01.org
> Cc: Wang, Jian J ; Wu, Hao A ;
> Zeng, Star ; Gao, Liming 
> Subject: RE: [PATCH 2/3] MdeModulePkg/PeiMain: Support
> EFI_PEI_CORE_FV_LOCATION_PPI
> 
> Sorry, please ignore my other review mail and just check this mail.
> 3 minor comments below.
> 
> > -Original Message-
> > From: Chiu, Chasel 
> > Sent: Tuesday, February 12, 2019 9:20 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wang, Jian J ; Wu, Hao A
> > ; Ni, Ray ; Zeng, Star
> > ; Gao, Liming ; Chiu,
> > Chasel 
> > Subject: [PATCH 2/3] MdeModulePkg/PeiMain: Support
> > EFI_PEI_CORE_FV_LOCATION_PPI
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1524
> >
> > When shadowing PeiCore the EFI_PEI_CORE_FV_LOCATION_PPI should be
> > checked to see if PeiCore not in BFV, otherwise just shadowing PeiCore
> > from BFV.
> >
> > Test: Verified on internal platform and booting successfully.
> >
> > Cc: Jian J Wang 
> > Cc: Hao Wu 
> > Cc: Ray Ni 
> > Cc: Star Zeng 
> > Cc: Liming Gao 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chasel Chiu 
> > ---
> >  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 58
> > +-
> >  MdeModulePkg/Core/Pei/PeiMain.h |  3 ++-
> >  MdeModulePkg/Core/Pei/PeiMain.inf   |  3 ++-
> >  3 files changed, 49 insertions(+), 15 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > index 4da80a8222..408f24c216 100644
> > --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >Pei Core Main Entry Point
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.
> >  This program and the accompanying materials  are licensed and made
> > available under the terms and conditions of the BSD License  which
> > accompanies this distribution.  The full text of the license may be
> > found at @@ -80,23 +80,55 @@ ShadowPeiCore (
> >IN PEI_CORE_INSTANCE  *PrivateData
> >)
> >  {
> > -  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> > -  EFI_PHYSICAL_ADDRESS EntryPoint;
> > -  EFI_STATUS   Status;
> > -  UINT32   AuthenticationState;
> > +  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> > +  EFI_PHYSICAL_ADDRESS EntryPoint;
> > +  EFI_STATUS   Status;
> > +  UINT32   AuthenticationState;
> > +  UINTNIndex;
> > +  EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> >
> >PeiCoreFileHandle = NULL;
> >
> >//
> > -  // Find the PEI Core in the BFV
> > +  // Find the PEI Core either from EFI_PEI_CORE_FV_LOCATION_PPI
> > + indicated FV or BFV
> >//
> > -  Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> > -   PrivateData->Fv[0].FvPpi,
> > -   EFI_FV_FILETYPE_PEI_CORE,
> > -   PrivateData->Fv[0].FvHandle,
> > -   
> > -   );
> > -  ASSERT_EFI_ERROR (Status);
> > +  Status = PeiServicesLocatePpi (
> > + ,
> > + 0,
> > + NULL,
> > + (VOID **) 
> > + );
> > +  if (!EFI_ERROR (Status) && (PeiCoreFvLocationPpi->PeiCoreFvLocation
> > + !=
> > NULL)) {
> > +//
> > +// If PeiCoreFvLocation present, the PEI Core should be found
> > + from
> > indicated FV.
> > +//
> > +for (Index = 0; Index < PrivateData->FvCount; Index ++) {
> > +  if ((UINT32) PrivateData->Fv[Index].FvHandle != (UINT32)
> > PeiCoreFvLocationPpi->PeiCoreFvLocation) {
> > +continue;
> > +  }
> > +  Status = PrivateData->Fv[Index].FvPpi->FindFileByType (
> > +   
> > PrivateData->Fv[Index].FvPpi,
> > +   EFI_FV_FILETYPE_PEI_CORE,
> > +   
> > PrivateData->Fv[Index].FvHandle,
> > +   
> > +   );
> > +  if (!EFI_ERROR (Status)) {
> > +break;
> 1. Is it valid that PEI_CORE cannot be found in the PeiCoreFvLocation?
> If no, why not assert here?
Agree. I will update the code.

> 
> > +  }
> > +}
> > +ASSERT (Index < PrivateData->FvCount);  } else {
> > +//
> > +// Find PEI Core from BFV
> > +//
> > +Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> > + PrivateData->Fv[0].FvPpi,
> > + EFI_FV_FILETYPE_PEI_CORE,
> > + PrivateData->Fv[0].FvHandle,
> > +

Re: [edk2] [PATCH 2/3] MdeModulePkg/PeiMain: Support EFI_PEI_CORE_FV_LOCATION_PPI

2019-02-14 Thread Ni, Ray 
Sorry, please ignore my other review mail and just check this mail.
3 minor comments below.

> -Original Message-
> From: Chiu, Chasel 
> Sent: Tuesday, February 12, 2019 9:20 PM
> To: edk2-devel@lists.01.org
> Cc: Wang, Jian J ; Wu, Hao A ;
> Ni, Ray ; Zeng, Star ; Gao, Liming
> ; Chiu, Chasel 
> Subject: [PATCH 2/3] MdeModulePkg/PeiMain: Support
> EFI_PEI_CORE_FV_LOCATION_PPI
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1524
> 
> When shadowing PeiCore the EFI_PEI_CORE_FV_LOCATION_PPI should be
> checked to see if PeiCore not in BFV, otherwise just shadowing PeiCore from
> BFV.
> 
> Test: Verified on internal platform and booting successfully.
> 
> Cc: Jian J Wang 
> Cc: Hao Wu 
> Cc: Ray Ni 
> Cc: Star Zeng 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu 
> ---
>  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 58
> +-
>  MdeModulePkg/Core/Pei/PeiMain.h |  3 ++-
>  MdeModulePkg/Core/Pei/PeiMain.inf   |  3 ++-
>  3 files changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> index 4da80a8222..408f24c216 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> @@ -1,7 +1,7 @@
>  /** @file
>Pei Core Main Entry Point
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
>  This program and the accompanying materials  are licensed and made
> available under the terms and conditions of the BSD License  which
> accompanies this distribution.  The full text of the license may be found at
> @@ -80,23 +80,55 @@ ShadowPeiCore (
>IN PEI_CORE_INSTANCE  *PrivateData
>)
>  {
> -  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> -  EFI_PHYSICAL_ADDRESS EntryPoint;
> -  EFI_STATUS   Status;
> -  UINT32   AuthenticationState;
> +  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> +  EFI_PHYSICAL_ADDRESS EntryPoint;
> +  EFI_STATUS   Status;
> +  UINT32   AuthenticationState;
> +  UINTNIndex;
> +  EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> 
>PeiCoreFileHandle = NULL;
> 
>//
> -  // Find the PEI Core in the BFV
> +  // Find the PEI Core either from EFI_PEI_CORE_FV_LOCATION_PPI
> + indicated FV or BFV
>//
> -  Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> -   PrivateData->Fv[0].FvPpi,
> -   EFI_FV_FILETYPE_PEI_CORE,
> -   PrivateData->Fv[0].FvHandle,
> -   
> -   );
> -  ASSERT_EFI_ERROR (Status);
> +  Status = PeiServicesLocatePpi (
> + ,
> + 0,
> + NULL,
> + (VOID **) 
> + );
> +  if (!EFI_ERROR (Status) && (PeiCoreFvLocationPpi->PeiCoreFvLocation !=
> NULL)) {
> +//
> +// If PeiCoreFvLocation present, the PEI Core should be found from
> indicated FV.
> +//
> +for (Index = 0; Index < PrivateData->FvCount; Index ++) {
> +  if ((UINT32) PrivateData->Fv[Index].FvHandle != (UINT32)
> PeiCoreFvLocationPpi->PeiCoreFvLocation) {
> +continue;
> +  }
> +  Status = PrivateData->Fv[Index].FvPpi->FindFileByType (
> +   PrivateData->Fv[Index].FvPpi,
> +   EFI_FV_FILETYPE_PEI_CORE,
> +   
> PrivateData->Fv[Index].FvHandle,
> +   
> +   );
> +  if (!EFI_ERROR (Status)) {
> +break;
1. Is it valid that PEI_CORE cannot be found in the PeiCoreFvLocation?
If no, why not assert here?

> +  }
> +}
> +ASSERT (Index < PrivateData->FvCount);  } else {
> +//
> +// Find PEI Core from BFV
> +//
> +Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> + PrivateData->Fv[0].FvPpi,
> + EFI_FV_FILETYPE_PEI_CORE,
> + PrivateData->Fv[0].FvHandle,
> + 
> + );
> +ASSERT_EFI_ERROR (Status);

2. Seems to have code duplication here. Can we have a local PeiCoreFvIndex to 
be 0 as default?
And when the gEfiPeiCoreFvLocationPpiGuid PPI exists, PeiCoreFvIndex is updated 
to the correct value.
So that FindFileByType() is only used once in source code with first parameter
PrivateData->Fv[PeiCoreFvIndex].FvPpi.

> +  }
> 
>//
>// Shadow PEI Core into memory so it will run faster diff --git
> a/MdeModulePkg/Core/Pei/PeiMain.h
>

Re: [edk2] [PATCH 2/3] MdeModulePkg/PeiMain: Support EFI_PEI_CORE_FV_LOCATION_PPI

2019-02-14 Thread Ni, Ray 
1. please move the #include adjacent to other #include
2. Can you explain why 

> -Original Message-
> From: Chiu, Chasel 
> Sent: Tuesday, February 12, 2019 9:20 PM
> To: edk2-devel@lists.01.org
> Cc: Wang, Jian J ; Wu, Hao A ;
> Ni, Ray ; Zeng, Star ; Gao, Liming
> ; Chiu, Chasel 
> Subject: [PATCH 2/3] MdeModulePkg/PeiMain: Support
> EFI_PEI_CORE_FV_LOCATION_PPI
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1524
> 
> When shadowing PeiCore the EFI_PEI_CORE_FV_LOCATION_PPI should be
> checked to see if PeiCore not in BFV, otherwise just shadowing PeiCore from
> BFV.
> 
> Test: Verified on internal platform and booting successfully.
> 
> Cc: Jian J Wang 
> Cc: Hao Wu 
> Cc: Ray Ni 
> Cc: Star Zeng 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu 
> ---
>  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 58
> +-
>  MdeModulePkg/Core/Pei/PeiMain.h |  3 ++-
>  MdeModulePkg/Core/Pei/PeiMain.inf   |  3 ++-
>  3 files changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> index 4da80a8222..408f24c216 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> @@ -1,7 +1,7 @@
>  /** @file
>Pei Core Main Entry Point
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
>  This program and the accompanying materials  are licensed and made
> available under the terms and conditions of the BSD License  which
> accompanies this distribution.  The full text of the license may be found at
> @@ -80,23 +80,55 @@ ShadowPeiCore (
>IN PEI_CORE_INSTANCE  *PrivateData
>)
>  {
> -  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> -  EFI_PHYSICAL_ADDRESS EntryPoint;
> -  EFI_STATUS   Status;
> -  UINT32   AuthenticationState;
> +  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> +  EFI_PHYSICAL_ADDRESS EntryPoint;
> +  EFI_STATUS   Status;
> +  UINT32   AuthenticationState;
> +  UINTNIndex;
> +  EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> 
>PeiCoreFileHandle = NULL;
> 
>//
> -  // Find the PEI Core in the BFV
> +  // Find the PEI Core either from EFI_PEI_CORE_FV_LOCATION_PPI
> + indicated FV or BFV
>//
> -  Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> -   PrivateData->Fv[0].FvPpi,
> -   EFI_FV_FILETYPE_PEI_CORE,
> -   PrivateData->Fv[0].FvHandle,
> -   
> -   );
> -  ASSERT_EFI_ERROR (Status);
> +  Status = PeiServicesLocatePpi (
> + ,
> + 0,
> + NULL,
> + (VOID **) 
> + );
> +  if (!EFI_ERROR (Status) && (PeiCoreFvLocationPpi->PeiCoreFvLocation !=
> NULL)) {
> +//
> +// If PeiCoreFvLocation present, the PEI Core should be found from
> indicated FV.
> +//
> +for (Index = 0; Index < PrivateData->FvCount; Index ++) {
> +  if ((UINT32) PrivateData->Fv[Index].FvHandle != (UINT32)
> PeiCoreFvLocationPpi->PeiCoreFvLocation) {
> +continue;
> +  }
> +  Status = PrivateData->Fv[Index].FvPpi->FindFileByType (
> +   PrivateData->Fv[Index].FvPpi,
> +   EFI_FV_FILETYPE_PEI_CORE,
> +   
> PrivateData->Fv[Index].FvHandle,
> +   
> +   );
> +  if (!EFI_ERROR (Status)) {
> +break;

1. Is it valid that PEI_CORE cannot be found in the PeiCoreFvLocation?
If no, why not assert here?

> +  }
> +}
> +ASSERT (Index < PrivateData->FvCount);  } else {
> +//
> +// Find PEI Core from BFV
> +//
> +Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> + PrivateData->Fv[0].FvPpi,
> + EFI_FV_FILETYPE_PEI_CORE,
> + PrivateData->Fv[0].FvHandle,
> + 
> + );
> +ASSERT_EFI_ERROR (Status);
> +  }
> 
>//
>// Shadow PEI Core into memory so it will run faster diff --git
> a/MdeModulePkg/Core/Pei/PeiMain.h
> b/MdeModulePkg/Core/Pei/PeiMain.h index 322e7cd845..38542ab072
> 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.h
> +++ b/MdeModulePkg/Core/Pei/PeiMain.h
> @@ -1,7 +1,7 @@
>  /** @file
>Definition of Pei Core Structures and Services
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel

Re: [edk2] [PATCH 2/3] MdeModulePkg/PeiMain: Support EFI_PEI_CORE_FV_LOCATION_PPI

2019-02-13 Thread Zeng, Star 
With this addressed, Reviewed-by: Star Zeng .

-Original Message-
From: Chiu, Chasel 
Sent: Wednesday, February 13, 2019 11:59 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Wu, Hao A ; Ni, Ray ; Zeng, Star 
; Gao, Liming 
Subject: RE: [PATCH 2/3] MdeModulePkg/PeiMain: Support 
EFI_PEI_CORE_FV_LOCATION_PPI


No issue, I will remove UINT32 casting. Thanks!

> -Original Message-
> From: Wang, Jian J
> Sent: Wednesday, February 13, 2019 9:14 AM
> To: Chiu, Chasel ; edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Ni, Ray ; Zeng, 
> Star ; Gao, Liming 
> Subject: RE: [PATCH 2/3] MdeModulePkg/PeiMain: Support 
> EFI_PEI_CORE_FV_LOCATION_PPI
> 
> Chasel,
> 
> 
> > -Original Message-
> > From: Chiu, Chasel
> > Sent: Tuesday, February 12, 2019 9:20 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wang, Jian J ; Wu, Hao A 
> > ; Ni, Ray ; Zeng, Star 
> > ; Gao, Liming ; Chiu, 
> > Chasel 
> > Subject: [PATCH 2/3] MdeModulePkg/PeiMain: Support 
> > EFI_PEI_CORE_FV_LOCATION_PPI
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1524
> >
> > When shadowing PeiCore the EFI_PEI_CORE_FV_LOCATION_PPI should be 
> > checked to see if PeiCore not in BFV, otherwise just shadowing 
> > PeiCore from BFV.
> >
> > Test: Verified on internal platform and booting successfully.
> >
> > Cc: Jian J Wang 
> > Cc: Hao Wu 
> > Cc: Ray Ni 
> > Cc: Star Zeng 
> > Cc: Liming Gao 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chasel Chiu 
> > ---
> >  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 58
> > +-
> >  MdeModulePkg/Core/Pei/PeiMain.h |  3 ++-
> >  MdeModulePkg/Core/Pei/PeiMain.inf   |  3 ++-
> >  3 files changed, 49 insertions(+), 15 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > index 4da80a8222..408f24c216 100644
> > --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >Pei Core Main Entry Point
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights 
> > reserved.
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights 
> > +reserved.
> >  This program and the accompanying materials  are licensed and made 
> > available under the terms and conditions of the BSD License  which 
> > accompanies this distribution.  The full text of the license may be 
> > found at @@ -80,23 +80,55 @@ ShadowPeiCore (
> >IN PEI_CORE_INSTANCE  *PrivateData
> >)
> >  {
> > -  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> > -  EFI_PHYSICAL_ADDRESS EntryPoint;
> > -  EFI_STATUS   Status;
> > -  UINT32   AuthenticationState;
> > +  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> > +  EFI_PHYSICAL_ADDRESS EntryPoint;
> > +  EFI_STATUS   Status;
> > +  UINT32   AuthenticationState;
> > +  UINTNIndex;
> > +  EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> >
> >PeiCoreFileHandle = NULL;
> >
> >//
> > -  // Find the PEI Core in the BFV
> > +  // Find the PEI Core either from EFI_PEI_CORE_FV_LOCATION_PPI 
> > + indicated
> > FV or BFV
> >//
> > -  Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> > -   PrivateData->Fv[0].FvPpi,
> > -   EFI_FV_FILETYPE_PEI_CORE,
> > -   PrivateData->Fv[0].FvHandle,
> > -   
> > -   );
> > -  ASSERT_EFI_ERROR (Status);
> > +  Status = PeiServicesLocatePpi (
> > + ,
> > + 0,
> > + NULL,
> > + (VOID **) 
> > + );
> > +  if (!EFI_ERROR (Status) && 
> > + (PeiCoreFvLocationPpi->PeiCoreFvLocation
> > + !=
> > NULL)) {
> > +//
> > +// If PeiCoreFvLocation present, the PEI Core should be found 
> > + from indicated
> > FV.
> > +//
> > +for (Index = 0; Index < PrivateData->FvCount; Index ++) {
> > +  if ((UINT32) PrivateData->Fv[Index].FvHandle != (UINT32)
> > PeiCoreFvLocationPpi->PeiCoreFvLocation) {
> 
> I think the UINT32 type cast is not necessary. FvHandle and 
> PeiCoreFvLocation are actually type of VOID*. Do you encounter any compiler 
> error here?
> 
> Regards,
> Jian
> 
> > +continue;
> > +  }
> > +  Status = PrivateData->Fv[Index].FvPpi->FindFileByType (
> > +   
> > PrivateData->Fv[Index].FvPpi,
> > +   EFI_FV_FILETYPE_PEI_CORE,
> > +   
> > PrivateData->Fv[Index].FvHandle,
> > +   
> > +   );
> > +  if (!EFI_ERROR (Status)) {
> > +break;
> > +  }
> > +}
> > +ASSERT (Index < PrivateData->FvCount);  } else {

Re: [edk2] [PATCH 2/3] MdeModulePkg/PeiMain: Support EFI_PEI_CORE_FV_LOCATION_PPI

2019-02-12 Thread Chiu, Chasel 


No issue, I will remove UINT32 casting. Thanks!

> -Original Message-
> From: Wang, Jian J
> Sent: Wednesday, February 13, 2019 9:14 AM
> To: Chiu, Chasel ; edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Ni, Ray ; Zeng, Star
> ; Gao, Liming 
> Subject: RE: [PATCH 2/3] MdeModulePkg/PeiMain: Support
> EFI_PEI_CORE_FV_LOCATION_PPI
> 
> Chasel,
> 
> 
> > -Original Message-
> > From: Chiu, Chasel
> > Sent: Tuesday, February 12, 2019 9:20 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wang, Jian J ; Wu, Hao A
> > ; Ni, Ray ; Zeng, Star
> > ; Gao, Liming ; Chiu,
> > Chasel 
> > Subject: [PATCH 2/3] MdeModulePkg/PeiMain: Support
> > EFI_PEI_CORE_FV_LOCATION_PPI
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1524
> >
> > When shadowing PeiCore the EFI_PEI_CORE_FV_LOCATION_PPI should be
> > checked to see if PeiCore not in BFV, otherwise just shadowing PeiCore
> > from BFV.
> >
> > Test: Verified on internal platform and booting successfully.
> >
> > Cc: Jian J Wang 
> > Cc: Hao Wu 
> > Cc: Ray Ni 
> > Cc: Star Zeng 
> > Cc: Liming Gao 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chasel Chiu 
> > ---
> >  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 58
> > +-
> >  MdeModulePkg/Core/Pei/PeiMain.h |  3 ++-
> >  MdeModulePkg/Core/Pei/PeiMain.inf   |  3 ++-
> >  3 files changed, 49 insertions(+), 15 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > index 4da80a8222..408f24c216 100644
> > --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >Pei Core Main Entry Point
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.
> >  This program and the accompanying materials  are licensed and made
> > available under the terms and conditions of the BSD License  which
> > accompanies this distribution.  The full text of the license may be
> > found at @@ -80,23 +80,55 @@ ShadowPeiCore (
> >IN PEI_CORE_INSTANCE  *PrivateData
> >)
> >  {
> > -  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> > -  EFI_PHYSICAL_ADDRESS EntryPoint;
> > -  EFI_STATUS   Status;
> > -  UINT32   AuthenticationState;
> > +  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> > +  EFI_PHYSICAL_ADDRESS EntryPoint;
> > +  EFI_STATUS   Status;
> > +  UINT32   AuthenticationState;
> > +  UINTNIndex;
> > +  EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> >
> >PeiCoreFileHandle = NULL;
> >
> >//
> > -  // Find the PEI Core in the BFV
> > +  // Find the PEI Core either from EFI_PEI_CORE_FV_LOCATION_PPI
> > + indicated
> > FV or BFV
> >//
> > -  Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> > -   PrivateData->Fv[0].FvPpi,
> > -   EFI_FV_FILETYPE_PEI_CORE,
> > -   PrivateData->Fv[0].FvHandle,
> > -   
> > -   );
> > -  ASSERT_EFI_ERROR (Status);
> > +  Status = PeiServicesLocatePpi (
> > + ,
> > + 0,
> > + NULL,
> > + (VOID **) 
> > + );
> > +  if (!EFI_ERROR (Status) && (PeiCoreFvLocationPpi->PeiCoreFvLocation
> > + !=
> > NULL)) {
> > +//
> > +// If PeiCoreFvLocation present, the PEI Core should be found
> > + from indicated
> > FV.
> > +//
> > +for (Index = 0; Index < PrivateData->FvCount; Index ++) {
> > +  if ((UINT32) PrivateData->Fv[Index].FvHandle != (UINT32)
> > PeiCoreFvLocationPpi->PeiCoreFvLocation) {
> 
> I think the UINT32 type cast is not necessary. FvHandle and PeiCoreFvLocation
> are actually type of VOID*. Do you encounter any compiler error here?
> 
> Regards,
> Jian
> 
> > +continue;
> > +  }
> > +  Status = PrivateData->Fv[Index].FvPpi->FindFileByType (
> > +   
> > PrivateData->Fv[Index].FvPpi,
> > +   EFI_FV_FILETYPE_PEI_CORE,
> > +   
> > PrivateData->Fv[Index].FvHandle,
> > +   
> > +   );
> > +  if (!EFI_ERROR (Status)) {
> > +break;
> > +  }
> > +}
> > +ASSERT (Index < PrivateData->FvCount);  } else {
> > +//
> > +// Find PEI Core from BFV
> > +//
> > +Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> > + PrivateData->Fv[0].FvPpi,
> > + EFI_FV_FILETYPE_PEI_CORE,
> > + PrivateData->Fv[0].FvHandle,
> > +

Re: [edk2] [PATCH 2/3] MdeModulePkg/PeiMain: Support EFI_PEI_CORE_FV_LOCATION_PPI

2019-02-12 Thread Wang, Jian J 
Chasel,


> -Original Message-
> From: Chiu, Chasel
> Sent: Tuesday, February 12, 2019 9:20 PM
> To: edk2-devel@lists.01.org
> Cc: Wang, Jian J ; Wu, Hao A ;
> Ni, Ray ; Zeng, Star ; Gao, Liming
> ; Chiu, Chasel 
> Subject: [PATCH 2/3] MdeModulePkg/PeiMain: Support
> EFI_PEI_CORE_FV_LOCATION_PPI
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1524
> 
> When shadowing PeiCore the EFI_PEI_CORE_FV_LOCATION_PPI
> should be checked to see if PeiCore not in BFV, otherwise
> just shadowing PeiCore from BFV.
> 
> Test: Verified on internal platform and booting successfully.
> 
> Cc: Jian J Wang 
> Cc: Hao Wu 
> Cc: Ray Ni 
> Cc: Star Zeng 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu 
> ---
>  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 58
> +-
>  MdeModulePkg/Core/Pei/PeiMain.h |  3 ++-
>  MdeModulePkg/Core/Pei/PeiMain.inf   |  3 ++-
>  3 files changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> index 4da80a8222..408f24c216 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
> @@ -1,7 +1,7 @@
>  /** @file
>Pei Core Main Entry Point
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be 
> found
> at
> @@ -80,23 +80,55 @@ ShadowPeiCore (
>IN PEI_CORE_INSTANCE  *PrivateData
>)
>  {
> -  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> -  EFI_PHYSICAL_ADDRESS EntryPoint;
> -  EFI_STATUS   Status;
> -  UINT32   AuthenticationState;
> +  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
> +  EFI_PHYSICAL_ADDRESS EntryPoint;
> +  EFI_STATUS   Status;
> +  UINT32   AuthenticationState;
> +  UINTNIndex;
> +  EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
> 
>PeiCoreFileHandle = NULL;
> 
>//
> -  // Find the PEI Core in the BFV
> +  // Find the PEI Core either from EFI_PEI_CORE_FV_LOCATION_PPI indicated
> FV or BFV
>//
> -  Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> -   PrivateData->Fv[0].FvPpi,
> -   EFI_FV_FILETYPE_PEI_CORE,
> -   PrivateData->Fv[0].FvHandle,
> -   
> -   );
> -  ASSERT_EFI_ERROR (Status);
> +  Status = PeiServicesLocatePpi (
> + ,
> + 0,
> + NULL,
> + (VOID **) 
> + );
> +  if (!EFI_ERROR (Status) && (PeiCoreFvLocationPpi->PeiCoreFvLocation !=
> NULL)) {
> +//
> +// If PeiCoreFvLocation present, the PEI Core should be found from 
> indicated
> FV.
> +//
> +for (Index = 0; Index < PrivateData->FvCount; Index ++) {
> +  if ((UINT32) PrivateData->Fv[Index].FvHandle != (UINT32)
> PeiCoreFvLocationPpi->PeiCoreFvLocation) {

I think the UINT32 type cast is not necessary. FvHandle and PeiCoreFvLocation 
are
actually type of VOID*. Do you encounter any compiler error here?

Regards,
Jian

> +continue;
> +  }
> +  Status = PrivateData->Fv[Index].FvPpi->FindFileByType (
> +   PrivateData->Fv[Index].FvPpi,
> +   EFI_FV_FILETYPE_PEI_CORE,
> +   
> PrivateData->Fv[Index].FvHandle,
> +   
> +   );
> +  if (!EFI_ERROR (Status)) {
> +break;
> +  }
> +}
> +ASSERT (Index < PrivateData->FvCount);
> +  } else {
> +//
> +// Find PEI Core from BFV
> +//
> +Status = PrivateData->Fv[0].FvPpi->FindFileByType (
> + PrivateData->Fv[0].FvPpi,
> + EFI_FV_FILETYPE_PEI_CORE,
> + PrivateData->Fv[0].FvHandle,
> + 
> + );
> +ASSERT_EFI_ERROR (Status);
> +  }
> 
>//
>// Shadow PEI Core into memory so it will run faster
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.h
> b/MdeModulePkg/Core/Pei/PeiMain.h
> index 322e7cd845..38542ab072 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.h
> +++ b/MdeModulePkg/Core/Pei/PeiMain.h
> @@ -1,7 +1,7 @@
>  /** @file
>Definition of Pei Core Structures and Services
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel

[edk2] [PATCH 2/3] MdeModulePkg/PeiMain: Support EFI_PEI_CORE_FV_LOCATION_PPI

2019-02-12 Thread Chasel, Chiu 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1524

When shadowing PeiCore the EFI_PEI_CORE_FV_LOCATION_PPI
should be checked to see if PeiCore not in BFV, otherwise
just shadowing PeiCore from BFV.

Test: Verified on internal platform and booting successfully.

Cc: Jian J Wang 
Cc: Hao Wu 
Cc: Ray Ni 
Cc: Star Zeng 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 58 
+-
 MdeModulePkg/Core/Pei/PeiMain.h |  3 ++-
 MdeModulePkg/Core/Pei/PeiMain.inf   |  3 ++-
 3 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c 
b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
index 4da80a8222..408f24c216 100644
--- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
+++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c
@@ -1,7 +1,7 @@
 /** @file
   Pei Core Main Entry Point
 
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -80,23 +80,55 @@ ShadowPeiCore (
   IN PEI_CORE_INSTANCE  *PrivateData
   )
 {
-  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
-  EFI_PHYSICAL_ADDRESS EntryPoint;
-  EFI_STATUS   Status;
-  UINT32   AuthenticationState;
+  EFI_PEI_FILE_HANDLE  PeiCoreFileHandle;
+  EFI_PHYSICAL_ADDRESS EntryPoint;
+  EFI_STATUS   Status;
+  UINT32   AuthenticationState;
+  UINTNIndex;
+  EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
 
   PeiCoreFileHandle = NULL;
 
   //
-  // Find the PEI Core in the BFV
+  // Find the PEI Core either from EFI_PEI_CORE_FV_LOCATION_PPI indicated FV 
or BFV
   //
-  Status = PrivateData->Fv[0].FvPpi->FindFileByType (
-   PrivateData->Fv[0].FvPpi,
-   EFI_FV_FILETYPE_PEI_CORE,
-   PrivateData->Fv[0].FvHandle,
-   
-   );
-  ASSERT_EFI_ERROR (Status);
+  Status = PeiServicesLocatePpi (
+ ,
+ 0,
+ NULL,
+ (VOID **) 
+ );
+  if (!EFI_ERROR (Status) && (PeiCoreFvLocationPpi->PeiCoreFvLocation != 
NULL)) {
+//
+// If PeiCoreFvLocation present, the PEI Core should be found from 
indicated FV.
+//
+for (Index = 0; Index < PrivateData->FvCount; Index ++) {
+  if ((UINT32) PrivateData->Fv[Index].FvHandle != (UINT32) 
PeiCoreFvLocationPpi->PeiCoreFvLocation) {
+continue;
+  }
+  Status = PrivateData->Fv[Index].FvPpi->FindFileByType (
+   PrivateData->Fv[Index].FvPpi,
+   EFI_FV_FILETYPE_PEI_CORE,
+   PrivateData->Fv[Index].FvHandle,
+   
+   );
+  if (!EFI_ERROR (Status)) {
+break;
+  }
+}
+ASSERT (Index < PrivateData->FvCount);
+  } else {
+//
+// Find PEI Core from BFV
+//
+Status = PrivateData->Fv[0].FvPpi->FindFileByType (
+ PrivateData->Fv[0].FvPpi,
+ EFI_FV_FILETYPE_PEI_CORE,
+ PrivateData->Fv[0].FvHandle,
+ 
+ );
+ASSERT_EFI_ERROR (Status);
+  }
 
   //
   // Shadow PEI Core into memory so it will run faster
diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h
index 322e7cd845..38542ab072 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.h
+++ b/MdeModulePkg/Core/Pei/PeiMain.h
@@ -1,7 +1,7 @@
 /** @file
   Definition of Pei Core Structures and Services
 
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -49,6 +49,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 
 ///
 /// It is an FFS type extension used for PeiFindFileEx. It indicates current
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf 
b/MdeModulePkg/Core/Pei/PeiMain.inf
index 140cb1..5bab2aab8c 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -6,7 +6,7 @@
 # 2) Dispatch PEIM from