Re: [edk2] [PATCH v3 2/3] EmulatorPkg/MpService: StartupAllAPs should verify processor state before setting state

Subject: Re: [edk2] [PATCH v3 2/3] EmulatorPkg/MpService: StartupAllAPs should verify processor state before setting state

From: Jordan Justen <jordan.l.justen@intel.com>

To: Chen Fan <chen.fan.fnst@cn.fujitsu.com>, edk2-devel@lists.sourceforge.net

Date: 2014-11-17 04:02:49

On 2014-11-16 18:51:16, Chen Fan wrote:
> if any enabled APs are not in idle state, StartupAllAPs() should return immediately,
> and must not change the other idled processor state. so we checked the state before
> changed them.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chen Fan 
> ---
>  EmulatorPkg/CpuRuntimeDxe/MpService.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/EmulatorPkg/CpuRuntimeDxe/MpService.c b/EmulatorPkg/CpuRuntimeDxe/MpService.c
> index 8263915..f395e7c 100644
> --- a/EmulatorPkg/CpuRuntimeDxe/MpService.c
> +++ b/EmulatorPkg/CpuRuntimeDxe/MpService.c
> @@ -397,6 +397,24 @@ CpuMpServicesStartupAllAps (
>      return EFI_UNSUPPORTED;
>    }
>  
> +  for (Number = 0; Number < gMPSystem.NumberOfProcessors; Number++) {
> +    ProcessorData = &gMPSystem.ProcessorData[Number];
> +    if ((ProcessorData->Info.StatusFlag & PROCESSOR_AS_BSP_BIT) == PROCESSOR_AS_BSP_BIT) {
> +      // Skip BSP
> +      continue;
> +    }
> +
> +    if ((ProcessorData->Info.StatusFlag & PROCESSOR_ENABLED_BIT) == 0) {
> +      // Skip Disabled processors
> +      continue;
> +    }
> +    gThread->MutexLock(ProcessorData->StateLock);
> +    if (ProcessorData->State != CPU_STATE_IDLE) {
> +      gThread->MutexUnlock (ProcessorData->StateLock);
> +      return EFI_NOT_READY;
> +    }
> +    gThread->MutexUnlock(ProcessorData->StateLock);
> +  }
>  
>    if (FailedCpuList != NULL) {
>      gMPSystem.FailedList = AllocatePool ((gMPSystem.NumberOfProcessors + 1) * sizeof (UINTN));
> @@ -437,18 +455,14 @@ CpuMpServicesStartupAllAps (
>      // state 1 by 1, until the previous 1 finished its task
>      // if not "SingleThread", all APs are put to ready state from the beginning
>      //
> +    ASSERT (ProcessorData->State == CPU_STATE_IDLE);
>      gThread->MutexLock(ProcessorData->StateLock);

I think the ASSERT should move just after the lock.

With that change,
Reviewed-by: Jordan Justen 

-Jordan

> -    if (ProcessorData->State == CPU_STATE_IDLE) {
> -      ProcessorData->State = APInitialState;
> -      gThread->MutexUnlock (ProcessorData->StateLock);
> +    ProcessorData->State = APInitialState;
> +    gThread->MutexUnlock (ProcessorData->StateLock);
>  
> -      gMPSystem.StartCount++;
> -      if (SingleThread) {
> -        APInitialState = CPU_STATE_BLOCKED;
> -      }
> -    } else {
> -      gThread->MutexUnlock (ProcessorData->StateLock);
> -      return EFI_NOT_READY;
> +    gMPSystem.StartCount++;
> +    if (SingleThread) {
> +      APInitialState = CPU_STATE_BLOCKED;
>      }
>    }
>  
> -- 
> 1.9.3
> 

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel