Re: [edk2] [RFC PATCH v6 09/27] UefiCpuPkg/CpuDxe: introduce MP_SYSTEM_DATA for Mp Service Protocol

Subject: Re: [edk2] [RFC PATCH v6 09/27] UefiCpuPkg/CpuDxe: introduce MP_SYSTEM_DATA for Mp Service Protocol

From: "Chen, Fan" <chen.fan.fnst@cn.fujitsu.com>

To: "Fan, Jeff" <jeff.fan@intel.com>

Date: 2014-11-03 17:09:34

On Mon, 2014-11-03 at 08:37 +0000, Fan, Jeff wrote: 
> Chen,
> 
> 1.FillInProcessorInformation() function head's parameters block does not match the function declaration.
> +/**
> +  This function is called by all processors (both BSP and AP) once and 
> +collects MP related data
> +
> +  @param MPSystemData    Pointer to the data structure containing MP related data
> +  @param BSP             TRUE if the CPU is BSP
> +
> +  @retval EFI_SUCCESS    Data for the processor collected and filled in
> +
> +**/
> +EFI_STATUS
> +FillInProcessorInformation (
> +  IN     BOOLEAN              BSP,
> +  IN     UINTN                ProcessorNumber
> +  )
> 
> 2. Missing point at end of function description of FillInProcessorInformation(). The similar issue exists in the following functions.
>     GetMpSpinLock (),ReleaseMpSpinLock (),CheckAndUpdateAllAPsToIdleState ().

Got it.
Thanks,
Chen

> 
> Thanks!
> Jeff
> 
> -----Original Message-----
> From: Chen Fan [mailto:chen.fan.fnst@cn.fujitsu.com] 
> Sent: Monday, October 27, 2014 5:30 PM
> To: edk2-devel@lists.sourceforge.net
> Cc: Fan, Jeff; Jordan Justen
> Subject: [RFC PATCH v6 09/27] UefiCpuPkg/CpuDxe: introduce MP_SYSTEM_DATA for Mp Service Protocol
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chen Fan 
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.inf |  1 +
>  UefiCpuPkg/CpuDxe/CpuMp.c    | 89 +++++++++++++++++++++++++++++++++++++++-----
>  UefiCpuPkg/CpuDxe/CpuMp.h    | 47 +++++++++++++++++++++++
>  UefiCpuPkg/UefiCpuPkg.dsc    |  1 +
>  4 files changed, 128 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf index 4f8ccac..6761e91 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -42,6 +42,7 @@
>    UefiLib
>    CpuExceptionHandlerLib
>    TimerLib
> +  SynchronizationLib
>  
>  [Sources]
>    ApStartup.c
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c index 61c3a23..20433b9 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -18,12 +18,12 @@
>  UINTN gMaxLogicalProcessorNumber;
>  UINTN gApStackSize;
>  
> +MP_SYSTEM_DATA mMpSystemData;
> +
>  VOID *mCommonStack = 0;
>  VOID *mTopOfApCommonStack = 0;
>  VOID *mApStackStart = 0;
>  
> -volatile UINTN mNumberOfProcessors;
> -
>  EFI_MP_SERVICES_PROTOCOL  mMpServicesTemplate = {
>    NULL, // GetNumberOfProcessors,
>    NULL, // GetProcessorInfo,
> @@ -63,14 +63,81 @@ ApEntryPointInC (
>    VOID
>    )
>  {
> -  mNumberOfProcessors++;
> -  mApStackStart = mApStackStart + gApStackSize;
> +  VOID* TopOfApStack;
> +
> +  FillInProcessorInformation (FALSE, mMpSystemData.NumberOfProcessors);
> +  TopOfApStack  = mApStackStart + gApStackSize;  mApStackStart = 
> + TopOfApStack;
> +
> +  mMpSystemData.NumberOfProcessors++;
>  
>    SwitchStack (
>      (SWITCH_STACK_ENTRY_POINT)(UINTN)ProcessorToIdleState,
>      NULL,
>      NULL,
> -    mApStackStart);
> +    TopOfApStack);
> +}
> +
> +/**
> +  This function is called by all processors (both BSP and AP) once and 
> +collects MP related data
> +
> +  @param MPSystemData    Pointer to the data structure containing MP related data
> +  @param BSP             TRUE if the CPU is BSP
> +
> +  @retval EFI_SUCCESS    Data for the processor collected and filled in
> +
> +**/
> +EFI_STATUS
> +FillInProcessorInformation (
> +  IN     BOOLEAN              BSP,
> +  IN     UINTN                ProcessorNumber
> +  )
> +{
> +  CPU_DATA_BLOCK  *CpuData;
> +
> +  CpuData = &mMpSystemData.CpuDatas[ProcessorNumber];
> +  CpuData->Info.ProcessorId  = GetApicId ();
> +  CpuData->Info.StatusFlag   = PROCESSOR_ENABLED_BIT | PROCESSOR_HEALTH_STATUS_BIT;
> +  if (BSP) {
> +    CpuData->Info.StatusFlag |= PROCESSOR_AS_BSP_BIT;  }  
> + CpuData->Info.Location.Package = (UINT32) ProcessorNumber;
> +  CpuData->Info.Location.Core    = 0;
> +  CpuData->Info.Location.Thread  = 0;
> +  CpuData->State = BSP ? CpuStateBuzy : CpuStateIdle;
> +
> +  CpuData->Procedure        = NULL;
> +  CpuData->Parameter        = NULL;
> +  InitializeSpinLock (&CpuData->CpuDataLock);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Prepare the System Data.
> +
> +  @retval EFI_SUCCESS     the System Data finished initilization.
> +
> +**/
> +EFI_STATUS
> +InitMpSystemData (
> +  VOID
> +  )
> +{
> +  ZeroMem (&mMpSystemData, sizeof (MP_SYSTEM_DATA));
> +
> +  mMpSystemData.NumberOfProcessors = 1;  
> + mMpSystemData.NumberOfEnabledProcessors = 1;
> +
> +  mMpSystemData.CpuDatas = AllocateZeroPool (sizeof (CPU_DATA_BLOCK) * 
> + gMaxLogicalProcessorNumber);  ASSERT(mMpSystemData.CpuDatas != NULL);
> +
> +  //
> +  // BSP
> +  //
> +  FillInProcessorInformation (TRUE, 0);
> +
> +  return EFI_SUCCESS;
>  }
>  
> 
> @@ -106,15 +173,17 @@ InitializeMpSupport (
>    mTopOfApCommonStack = mApStackStart + gApStackSize;
>    mApStackStart = mTopOfApCommonStack;
>  
> -  mNumberOfProcessors = 1;
> +  InitMpSystemData ();
>  
> -  if (mNumberOfProcessors == 1) {
> +  if (mMpSystemData.NumberOfProcessors == 1) {
>      FreePages (mCommonStack, EFI_SIZE_TO_PAGES (gMaxLogicalProcessorNumber * gApStackSize));
>      return;
>    }
>  
> -  if (mNumberOfProcessors < gMaxLogicalProcessorNumber) {
> -    FreePages (mApStackStart, EFI_SIZE_TO_PAGES ((gMaxLogicalProcessorNumber - mNumberOfProcessors) *
> -                                                 gApStackSize));
> +
> +  if (mMpSystemData.NumberOfProcessors < gMaxLogicalProcessorNumber) {
> +    FreePages (mApStackStart, EFI_SIZE_TO_PAGES (
> +                                (gMaxLogicalProcessorNumber - mMpSystemData.NumberOfProcessors) *
> +                                gApStackSize));
>    }
>  }
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h index efdd948..b8771f7 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.h
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
> @@ -16,6 +16,7 @@
>  #define _CPU_MP_H_
>  
>  #include 
> +#include 
>  
>  /**
>    Initialize Multi-processor support.
> @@ -76,5 +77,51 @@ AsmApDoneWithCommonStack (
>    VOID
>    );
>  
> +typedef enum {
> +  CpuStateIdle,
> +  CpuStateBlocked,
> +  CpuStateReady,
> +  CpuStateBuzy,
> +  CpuStateFinished
> +} CPU_STATE;
> +
> +/**
> +  Define Individual Processor Data block.
> +
> +**/
> +typedef struct {
> +  EFI_PROCESSOR_INFORMATION      Info;
> +  SPIN_LOCK                      CpuDataLock;
> +  volatile CPU_STATE             State;
> +
> +  EFI_AP_PROCEDURE               Procedure;
> +  VOID                           *Parameter;
> +} CPU_DATA_BLOCK;
> +
> +/**
> +  Define MP data block which consumes individual processor block.
> +
> +**/
> +typedef struct {
> +  CPU_DATA_BLOCK              *CpuDatas;
> +  UINTN                       NumberOfProcessors;
> +  UINTN                       NumberOfEnabledProcessors;
> +} MP_SYSTEM_DATA;
> +
> +/**
> +  This function is called by all processors (both BSP and AP) once and 
> +collects MP related data
> +
> +  @param MPSystemData    Pointer to the data structure containing MP related data
> +  @param BSP             TRUE if the CPU is BSP
> +
> +  @retval EFI_SUCCESS    Data for the processor collected and filled in
> +
> +**/
> +EFI_STATUS
> +FillInProcessorInformation (
> +  IN     BOOLEAN              BSP,
> +  IN     UINTN                ProcessorNumber
> +  );
> +
>  #endif // _CPU_MP_H_
>  
> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc index 70d5bb0..9fa9270 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -52,6 +52,7 @@
>    LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>    ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
>    CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf  
> +  
> + SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchroni
> + zationLib.inf
>  
>  [LibraryClasses.common.PEIM]
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> --
> 1.9.3
> 

------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel