Hi Star,

There are three fields in FSPM_ARCH_UPD that are not in FSPM_ARCH_CONFIG_PPI:

1. StackBase
2. StackSize
3. BootMode

For #1 and #2, there is no need for a second stack and second HOB list when FSP 
is running in dispatch mode, so these entries are not needed.

For #3, there is already a PI specification defined method of setting BootMode, 
specifically (*PeiServices)->SetBootMode().

The purpose of dispatch mode is to align FSP with the existing PI specification 
as much as possible, which is why we explicitly removed these entries from 
FSPM_ARCH_CONFIG_PPI.

Reviewed-by: Nate DeSimone <nathaniel.l.desim...@intel.com>

Thanks,
Nate

-----Original Message-----
From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Zeng, Star
Sent: Monday, December 10, 2018 12:01 AM
To: Chiu, Chasel <chasel.c...@intel.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.z...@intel.com>
Subject: Re: [edk2] [PATCH] IntelFsp2Pkg: Add FspmArchConfigPpi to support 
Dispatch mode

Hi Chasel,

Two minor comments are added inline.

On 2018/12/7 21:30, Chasel, Chiu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1381
> 
> In Dispatch mode FSP may consume PPI directly so creating 
> FSPM_ARCH_CONFIG_PPI to align with FSPM_ARCH_UPD.
> 
> Test: Verified on internal platform to boot with this PPI installed 
> successfully.
> 
> Cc: Nate DeSimone <nathaniel.l.desim...@intel.com>
> Cc: Star Zeng <star.z...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu <chasel.c...@intel.com>
> ---
>   IntelFsp2Pkg/Include/Ppi/FspmArchConfigPpi.h | 41 
> +++++++++++++++++++++++++++++++++++++++++
>   IntelFsp2Pkg/IntelFsp2Pkg.dec                |  3 +++
>   2 files changed, 44 insertions(+)
> 
> diff --git a/IntelFsp2Pkg/Include/Ppi/FspmArchConfigPpi.h 
> b/IntelFsp2Pkg/Include/Ppi/FspmArchConfigPpi.h
> new file mode 100644
> index 0000000000..5b3dac5b04
> --- /dev/null
> +++ b/IntelFsp2Pkg/Include/Ppi/FspmArchConfigPpi.h
> @@ -0,0 +1,41 @@
> +/** @file
> +  Header file for FSP-M Arch Config PPI
> +
> + @copyright
> +  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> +
> +  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  
> + http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" 
> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS 
> OR IMPLIED.
> +
> +**/
> +
> +#ifndef _FSPM_ARCH_CONFIG_PPI_H_
> +#define _FSPM_ARCH_CONFIG_PPI_H_
> +
> +///
> +/// Global ID for the FSPM_ARCH_CONFIG_PPI.
> +///
> +#define FSPM_ARCH_CONFIG_GUID \
> +  { \
> +    0x824d5a3a, 0xaf92, 0x4c0c, { 0x9f, 0x19, 0x19, 0x52, 0x6d, 0xca, 
> +0x4a, 0xbb } \
> +  }
> +
> +///
> +/// This PPI provides FSP-M Arch Config PPI.

How about adding more description about this PPI is used for dispatch mode?

> +///
> +typedef struct {
> +  UINT8         Revision;
> +  UINT8         Reserved[3];
> +  VOID          *NvsBufferPtr;
> +  UINT32        BootLoaderTolumSize;
> +  UINT8         Reserved1[8];
> +} FSPM_ARCH_CONFIG_PPI;

Could the definition be aligned with FSPM_ARCH_UPD in 
IntelFsp2Pkg\Include\FspEas\FspApi.h? Then we can use typedef to define it like 
below.

typedef FSPM_ARCH_UPD FSPM_ARCH_CONFIG_PPI;

Otherwise, please add description for the fields like FSPM_ARCH_UPD did.


Thanks,
Star

> +
> +extern EFI_GUID gFspmArchConfigPpiGuid;
> +
> +#endif // _FSPM_ARCH_CONFIG_PPI_H_
> diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec 
> b/IntelFsp2Pkg/IntelFsp2Pkg.dec index 50496241da..de1bece562 100644
> --- a/IntelFsp2Pkg/IntelFsp2Pkg.dec
> +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec
> @@ -70,6 +70,9 @@
>     gFspPerformanceDataGuid               = { 0x56ed21b6, 0xba23, 0x429e, { 
> 0x89, 0x32, 0x37, 0x6d, 0x8e, 0x18, 0x2e, 0xe3 } }
>     gFspEventEndOfFirmwareGuid            = { 0xbd44f629, 0xeae7, 0x4198, { 
> 0x87, 0xf1, 0x39, 0xfa, 0xb0, 0xfd, 0x71, 0x7e } }
>   
> +[Ppis]
> +  gFspmArchConfigPpiGuid                = { 0x824d5a3a, 0xaf92, 0x4c0c, { 
> 0x9f, 0x19, 0x19, 0x52, 0x6d, 0xca, 0x4a, 0xbb } }
> +
>   [PcdsFixedAtBuild]
>     gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress 
> |0xFED00108|UINT32|0x00000001
>     gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase         
> |0xFEF00000|UINT32|0x10001001
> 

_______________________________________________
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

Reply via email to