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:[email protected]]
> Sent: Thursday, September 18, 2014 11:12 AM
> To: [email protected]
> 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 <[email protected]>
> ---
> 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 <Protocol/MpService.h>
> -#include <Library/SynchronizationLib.h>
>
> #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
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
