Re: [edk2] [PATCH v3 01/52] UefiCpuPkg: CpuDxe: Fix ASSERT() when only 1 CPU detected

2015-10-21 Thread Laszlo Ersek 
Adding Jordan.

On 10/21/15 03:14, Kinney, Michael D wrote:
> Bruce,
> 
> No.  Different ASSERT().
> 
> That looks like a new bug, and it is in the new code I added to force BSP to 
> wait for all APs to enter sleeping state before StartupAllAPs() is called.
> 
> How did you reproduce this?

Actually, I'm seeing a related / similar assertion failure, from here:

  //
  // Wait for all APs to enter idle loop.
  //
  Timeout = 0;
  do {
if (CheckAllAPsSleeping ()) {
  break;
}
gBS->Stall (gPollInterval);
Timeout += gPollInterval;
  } while (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
  ASSERT (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));

I'm not doing anything special, just booting a preexistent guest on KVM.

The default timeout from UefiCpuPkg is 50msec, which is apparently too
short here. I have not discovered this in practice earlier because:
- I've been focusing on TCG
- my patch

  [PATCH] OvmfPkg: increase MP services startup timeou
  http://thread.gmane.org/gmane.comp.bios.edk2.devel/3260

  has been part of my builds recently.

So, I think we might have to apply said patch to OvmfPkg quickly, but
not strictly in response to the bug report / analysis from Janusz Mocek
& Xiao Guangrong: instead, to fix up this regression.

(See also
<http://thread.gmane.org/gmane.comp.bios.edk2.devel/3198/focus=3223>,
where I pointed out that the UefiCpuPkg change actually halves the
preexistent timeout.)

Then, any smarts discussed under
<http://thread.gmane.org/gmane.comp.bios.edk2.devel/3260/focus=3264>
should be implemented on top.

Jordan, do you agree we can / should preliminarily apply my patch?
As-is, OVMF might not boot on KVM at all at the moment, due to these
timeouts.

Thanks
Laszlo

> 
> Thanks,
> 
> Mike
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Bruce Cran
>> Sent: Tuesday, October 20, 2015 5:21 PM
>> To: Laszlo Ersek; edk2-de...@ml01.01.org
>> Subject: Re: [edk2] [PATCH v3 01/52] UefiCpuPkg: CpuDxe: Fix ASSERT() when
>> only 1 CPU detected
>>
>> On 10/14/15 4:25 PM, Laszlo Ersek wrote:
>>> From: Michael Kinney <michael.d.kin...@intel.com>
>>>
>>> When only 1 CPU is detected and the max CPUs is greater than 1,
>>> an ASSERT() is generated because the pages associated with the
>>> AP stack have already been freed.  Only do this FreePages() call
>>> if there is more than 1 CPU detected.
>>
>> Is the ASSERT() that was being triggered "LockValue == ((UINTN) 2) ||
>> LockValue == ((UINTN) 1)" in:
>>
>>   AcquireSpinLockOrFail
>>   GetMpSpinLock
>>   TestCpuStatusFlag
>>   CheckAllAPsSleeping
>>   InitializeMpSupport
>>   InitializeCpu
>>   ...
>>
>> Or is this a separate bug?
>>
>> --
>> Bruce
>> ___
>> 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

Re: [edk2] [PATCH v3 01/52] UefiCpuPkg: CpuDxe: Fix ASSERT() when only 1 CPU detected

2015-10-20 Thread Bruce Cran 

On 10/14/15 4:25 PM, Laszlo Ersek wrote:

From: Michael Kinney 

When only 1 CPU is detected and the max CPUs is greater than 1,
an ASSERT() is generated because the pages associated with the
AP stack have already been freed.  Only do this FreePages() call
if there is more than 1 CPU detected.


Is the ASSERT() that was being triggered "LockValue == ((UINTN) 2) || 
LockValue == ((UINTN) 1)" in:


  AcquireSpinLockOrFail
  GetMpSpinLock
  TestCpuStatusFlag
  CheckAllAPsSleeping
  InitializeMpSupport
  InitializeCpu
  …

Or is this a separate bug?

--
Bruce
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 01/52] UefiCpuPkg: CpuDxe: Fix ASSERT() when only 1 CPU detected

2015-10-20 Thread Kinney, Michael D 
Bruce,

No.  Different ASSERT().

That looks like a new bug, and it is in the new code I added to force BSP to 
wait for all APs to enter sleeping state before StartupAllAPs() is called.

How did you reproduce this?

Thanks,

Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Bruce Cran
>Sent: Tuesday, October 20, 2015 5:21 PM
>To: Laszlo Ersek; edk2-de...@ml01.01.org
>Subject: Re: [edk2] [PATCH v3 01/52] UefiCpuPkg: CpuDxe: Fix ASSERT() when
>only 1 CPU detected
>
>On 10/14/15 4:25 PM, Laszlo Ersek wrote:
>> From: Michael Kinney <michael.d.kin...@intel.com>
>>
>> When only 1 CPU is detected and the max CPUs is greater than 1,
>> an ASSERT() is generated because the pages associated with the
>> AP stack have already been freed.  Only do this FreePages() call
>> if there is more than 1 CPU detected.
>
>Is the ASSERT() that was being triggered "LockValue == ((UINTN) 2) ||
>LockValue == ((UINTN) 1)" in:
>
>   AcquireSpinLockOrFail
>   GetMpSpinLock
>   TestCpuStatusFlag
>   CheckAllAPsSleeping
>   InitializeMpSupport
>   InitializeCpu
>   ...
>
>Or is this a separate bug?
>
>--
>Bruce
>___
>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

[edk2] [PATCH v3 01/52] UefiCpuPkg: CpuDxe: Fix ASSERT() when only 1 CPU detected

2015-10-14 Thread Laszlo Ersek 
From: Michael Kinney 

When only 1 CPU is detected and the max CPUs is greater than 1,
an ASSERT() is generated because the pages associated with the
AP stack have already been freed.  Only do this FreePages() call
if there is more than 1 CPU detected.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney 
---

Notes:
v2:
- New in v2, but this is just a fixup from Mike on top of his series
  "[edk2] [PATCH 0/7] UefiCpuPkg: Add CPU SMM and SecCore". I'm
  including it here for completeness; the next version of Mike's series
  will contain it squashed-in.

 UefiCpuPkg/CpuDxe/CpuMp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
index 22a0d9d..da3686e 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -1697,7 +1697,7 @@ InitializeMpSupport (
   );
   ASSERT_EFI_ERROR (Status);
 
-  if (mMpSystemData.NumberOfProcessors < gMaxLogicalProcessorNumber) {
+  if (mMpSystemData.NumberOfProcessors > 1 && mMpSystemData.NumberOfProcessors 
< gMaxLogicalProcessorNumber) {
 if (mApStackStart != NULL) {
   FreePages (mApStackStart, EFI_SIZE_TO_PAGES (
   (gMaxLogicalProcessorNumber - 
mMpSystemData.NumberOfProcessors) *
-- 
1.8.3.1


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel