Re: [edk2] [RFC v3 16/19] UefiCpuPkg/CpuDxe: introduce CPU bus lock for sync data

Subject: Re: [edk2] [RFC v3 16/19] UefiCpuPkg/CpuDxe: introduce CPU bus lock for sync data

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

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

Date: 2014-09-19 10:40:56

On Thu, 2014-09-18 at 07:30 +0000, Fan, Jeff wrote: 
> Chen,
> 
> It seems that you fixed ASSERT issue by getting rid of AcquireSpinLock (). 
> 
> Could you use the following code to take a try?
>    while (!AcquireSpinLockOrFail (&CpuData->CpuDataLock)) {
>      CpuPause();
>    }
> 
Hi Jeff,

   I had used the above code to rerun the code,  it seems to work fine.

Thanks,
Chen


> I suspect the ASSERT is caused by TimerLib services consumed by AcquireSpinLock() implementation.
> 
> Thanks!
> Jeff
> -----Original Message-----
> From: Chen Fan [mailto:chen.fan.fnst@cn.fujitsu.com] 
> Sent: Thursday, September 18, 2014 11:12 AM
> To: edk2-devel@lists.sourceforge.net
> Cc: Jordan Justen; Fan, Jeff; Andrew Fish
> Subject: [RFC v3 16/19] UefiCpuPkg/CpuDxe: introduce CPU bus lock for sync data
> 
> Because of AcquireSpinLock()/ReleaseSpinLock() was not MP safe, We should introduce CPU bus lock to replace them.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chen Fan 
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.inf      |  1 -
>  UefiCpuPkg/CpuDxe/CpuMp.c         | 33 ++++++++++++++++-----------------
>  UefiCpuPkg/CpuDxe/CpuMp.h         | 24 ++++++++++++++++++++++--
>  UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm | 31 +++++++++++++++++++++++++++++++  UefiCpuPkg/CpuDxe/X64/MpAsm.nasm  | 31 +++++++++++++++++++++++++++++++
>  UefiCpuPkg/UefiCpuPkg.dsc         |  1 -
>  6 files changed, 100 insertions(+), 21 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf index ed90ff2..f73b845 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -41,7 +41,6 @@
>    UefiCpuLib
>    UefiLib
>    CpuExceptionHandlerLib
> -  SynchronizationLib
>  
>  [Sources]
>    ApStartup.c
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c index 2ba1e28..bcf5106 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -78,9 +78,9 @@ GetApState (
>  {
>    CPU_STATE State;
>  
> -  AcquireSpinLock (&CpuData->CpuDataLock);
> +  AsmApAcquireSpinLock ();
>    State = CpuData->State;
> -  ReleaseSpinLock (&CpuData->CpuDataLock);
> +  AsmApReleaseSpinLock ();
>  
>    return State;
>  }
> @@ -98,9 +98,9 @@ SetApState (
>    IN  CPU_STATE        State
>    )
>  {
> -  AcquireSpinLock (&CpuData->CpuDataLock);
> +  AsmApAcquireSpinLock ();
>    CpuData->State = State;
> -  ReleaseSpinLock (&CpuData->CpuDataLock);
> +  AsmApReleaseSpinLock ();
>  }
>  
>  /**
> @@ -119,10 +119,10 @@ SetApProcedure (
>    IN   VOID                  *ProcedureArgument
>    )
>  {
> -  AcquireSpinLock (&CpuData->CpuDataLock);
> +  AsmApAcquireSpinLock ();
>    CpuData->Parameter  = ProcedureArgument;
>    CpuData->Procedure  = Procedure;
> -  ReleaseSpinLock (&CpuData->CpuDataLock);
> +  AsmApReleaseSpinLock ();
>  }
>  
>  /**
> @@ -143,9 +143,9 @@ TestCpuStatusFlag (
>  {
>    UINT32 Ret;
>  
> -  AcquireSpinLock (&CpuData->CpuDataLock);
> +  AsmApAcquireSpinLock ();
>    Ret = CpuData->Info.StatusFlag & Flags;
> -  ReleaseSpinLock (&CpuData->CpuDataLock);
> +  AsmApReleaseSpinLock ();
>  
>    return !!(Ret);
>  }
> @@ -163,9 +163,9 @@ CpuStatusFlagOr (
>    IN  UINT32          Flags
>    )
>  {
> -  AcquireSpinLock (&CpuData->CpuDataLock);
> +  AsmApAcquireSpinLock ();
>    CpuData->Info.StatusFlag |= Flags;
> -  ReleaseSpinLock (&CpuData->CpuDataLock);
> +  AsmApReleaseSpinLock ();
>  }
>  
>  /**
> @@ -181,9 +181,9 @@ CpuStatusFlagAndNot (
>    IN  UINT32          Flags
>    )
>  {
> -  AcquireSpinLock (&CpuData->CpuDataLock);
> +  AsmApAcquireSpinLock ();
>    CpuData->Info.StatusFlag &= ~Flags;
> -  ReleaseSpinLock (&CpuData->CpuDataLock);
> +  AsmApReleaseSpinLock ();
>  }
>  
>  /**
> @@ -671,19 +671,19 @@ ApDoLoop (
>    CpuData = (CPU_DATA_BLOCK *)Context;
>  
>    while (TRUE) {
> -    AcquireSpinLock (&CpuData->CpuDataLock);
> +    AsmApAcquireSpinLock ();
>      ProcedureArgument = CpuData->Parameter;
>      Procedure = CpuData->Procedure;
> -    ReleaseSpinLock (&CpuData->CpuDataLock);
> +    AsmApReleaseSpinLock ();
>  
>      if (Procedure != NULL) {
>        SetApState (CpuData, CPU_STATE_BUSY);
>  
>        Procedure (ProcedureArgument);
>  
> -      AcquireSpinLock (&CpuData->CpuDataLock);
> +      AsmApAcquireSpinLock ();
>        CpuData->Procedure = NULL;
> -      ReleaseSpinLock (&CpuData->CpuDataLock);
> +      AsmApReleaseSpinLock ();
>  
>        SetApState (CpuData, CPU_STATE_FINISHED);
>      }
> @@ -766,7 +766,6 @@ InitMpSystemData (
>        CpuData->Info.StatusFlag |= PROCESSOR_AS_BSP_BIT;
>        CpuData->Info.ProcessorId = GetApicId ();
>      }
> -    InitializeSpinLock (&CpuData->CpuDataLock);
>  
>      Status = gBS->CreateEvent (
>                      EVT_TIMER | EVT_NOTIFY_SIGNAL, diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h index 411fe81..236fd22 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.h
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
> @@ -16,7 +16,6 @@
>  #define _CPU_MP_H_
>  
>  #include 
> -#include 
>  
>  #define AP_STACK_SIZE      SIZE_64KB
>  
> @@ -110,6 +109,28 @@ AsmApReleaseLock (
>    VOID
>    );
>  
> +/**
> +  the function added a CPU bus lock to protect the same data location
> +  in multi processors environment.
> +
> +**/
> +VOID
> +EFIAPI
> +AsmApAcquireSpinLock (
> +  VOID
> +  );
> +
> +/**
> +  release the CPU bus lock.
> +
> +**/
> +VOID
> +EFIAPI
> +AsmApReleaseSpinLock (
> +  VOID
> +  );
> +
> +
>  typedef enum {
>    CPU_STATE_IDLE,
>    CPU_STATE_BLOCKED,
> @@ -124,7 +145,6 @@ typedef enum {
>  **/
>  typedef struct {
>    EFI_PROCESSOR_INFORMATION      Info;
> -  SPIN_LOCK                      CpuDataLock;
>    CPU_STATE volatile             State;
>  
>    EFI_AP_PROCEDURE               Procedure;
> diff --git a/UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm b/UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm
> index 154b3a5..de7d3a4 100644
> --- a/UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm
> +++ b/UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm
> @@ -24,6 +24,9 @@ extern ASM_PFX(mCpuExchangeData)
>  ApStackLock:
>      dd      0
>  
> +ApSpinLock:
> +    dd      0
> +
>  ;------------------------------------------------------------------------------
>  ; VOID
>  ; EFIAPI
> @@ -111,3 +114,31 @@ ASM_PFX(AsmApReleaseLock):
>  lock btc  dword[ApStackLock], 0
>      ret
>  
> +;----------------------------------------------------------------------
> +--------
> +; VOID
> +; EFIAPI
> +; AsmApAcquireSpinLock (
> +;   VOID
> +;   );
> +;----------------------------------------------------------------------
> +--------
> +global ASM_PFX(AsmApAcquireSpinLock)
> +ASM_PFX(AsmApAcquireSpinLock):
> +AsmApSpinLock:
> +lock bts   dword[ApSpinLock], 0
> +    pause
> +    jc     AsmApSpinLock
> +
> +    ret
> +
> +;----------------------------------------------------------------------
> +--------
> +; VOID
> +; EFIAPI
> +; AsmApReleaseSpinLock (
> +;   VOID
> +;   );
> +;----------------------------------------------------------------------
> +--------
> +global ASM_PFX(AsmApReleaseSpinLock)
> +ASM_PFX(AsmApReleaseSpinLock):
> +lock btc  dword[ApSpinLock], 0
> +    ret
> +
> diff --git a/UefiCpuPkg/CpuDxe/X64/MpAsm.nasm b/UefiCpuPkg/CpuDxe/X64/MpAsm.nasm
> index 46ed107..dec9ee7 100644
> --- a/UefiCpuPkg/CpuDxe/X64/MpAsm.nasm
> +++ b/UefiCpuPkg/CpuDxe/X64/MpAsm.nasm
> @@ -24,6 +24,9 @@ extern ASM_PFX(mCpuExchangeData)
>  ApStackLock:
>      dd      0
>  
> +ApSpinLock:
> +    dd      0
> +
>  ;------------------------------------------------------------------------------
>  ; VOID
>  ; EFIAPI
> @@ -109,3 +112,31 @@ ASM_PFX(AsmApReleaseLock):
>  lock btc  dword[ApStackLock], 0
>      ret
>  
> +;----------------------------------------------------------------------
> +--------
> +; VOID
> +; EFIAPI
> +; AsmApAcquireSpinLock (
> +;   VOID
> +;   );
> +;----------------------------------------------------------------------
> +--------
> +global ASM_PFX(AsmApAcquireSpinLock)
> +ASM_PFX(AsmApAcquireSpinLock):
> +AsmApSpinLock:
> +lock bts   dword[ApSpinLock], 0
> +    pause
> +    jc     AsmApSpinLock
> +
> +    ret
> +
> +;----------------------------------------------------------------------
> +--------
> +; VOID
> +; EFIAPI
> +; AsmApReleaseSpinLock (
> +;   VOID
> +;   );
> +;----------------------------------------------------------------------
> +--------
> +global ASM_PFX(AsmApReleaseSpinLock)
> +ASM_PFX(AsmApReleaseSpinLock):
> +lock btc  dword[ApSpinLock], 0
> +    ret
> +
> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc index 9fa9270..70d5bb0 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -52,7 +52,6 @@
>    LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>    ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
>    CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf
> -  SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
>  
>  [LibraryClasses.common.PEIM]
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> --
> 1.9.3
> 

------------------------------------------------------------------------------
Slashdot TV.  Video for Nerds.  Stuff that Matters.
http://pubads.g.doubleclick.net/gampad/clk?id=160591471&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel