[Patch v2 0/2] UefiCpuPkg: Remove debug message.

[Patch v2 0/2] UefiCpuPkg: Remove debug message.

[Dong, Eric]

This debug message may be called by BSP and APs. It may
caused ASSERT when APs call this debug code.

In order to avoid system boot assert, Remove this debug
message.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>

Eric Dong (2):
  UefiCpuPkg/RegisterCpuFeaturesLib: Remove debug message.
  UefiCpuPkg/PiSmmCpuDxeSmm: Remove debug message.

 .../CpuFeaturesInitialize.c                   | 22 -------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 22 +------------------
 2 files changed, 1 insertion(+), 43 deletions(-)

-- 
2.21.0.windows.1

[Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Remove debug message.

[Dong, Eric]

This debug message may be called by BSP and APs. It may
caused ASSERT when APs call this debug code.

In order to avoid system boot assert, Remove this debug
message.

Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 .../CpuFeaturesInitialize.c                   | 22 -------------------
 1 file changed, 22 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index 4e97e863c7..fb0535edd6 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -9,7 +9,6 @@
 #include "RegisterCpuFeatures.h"
 
 CHAR16 *mDependTypeStr[]   = {L"None", L"Thread", L"Core", L"Package", L"Invalid" };
-CHAR16 *mRegisterTypeStr[] = {L"MSR", L"CR", L"MMIO", L"CACHE", L"SEMAP", L"INVALID" };
 
 /**
   Worker function to save PcdCpuFeaturesCapability.
@@ -772,7 +771,6 @@ ProgramProcessorRegister (
   UINT32                    PackageThreadsCount;
   UINT32                    CurrentThread;
   UINTN                     ProcessorIndex;
-  UINTN                     ThreadIndex;
   UINTN                     ValidThreadCount;
   UINT32                    *ValidCoreCountPerPackage;
 
@@ -785,26 +783,6 @@ ProgramProcessorRegister (
 
     RegisterTableEntry = &RegisterTableEntryHead[Index];
 
-    DEBUG_CODE_BEGIN ();
-      //
-      // Wait for the AP to release the MSR spin lock.
-      //
-      while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) {
-        CpuPause ();
-      }
-      ThreadIndex = ApLocation->Package * CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount +
-              ApLocation->Core * CpuStatus->MaxThreadCount +
-              ApLocation->Thread;
-      DEBUG ((
-        DEBUG_INFO,
-        "Processor = %08lu, Index %08lu, Type = %s!\n",
-        (UINT64)ThreadIndex,
-        (UINT64)Index,
-        mRegisterTypeStr[MIN ((REGISTER_TYPE)RegisterTableEntry->RegisterType, InvalidReg)]
-        ));
-      ReleaseSpinLock (&CpuFlags->ConsoleLogLock);
-    DEBUG_CODE_END ();
-
     //
     // Check the type of specified register
     //
-- 
2.21.0.windows.1

Re: [edk2-devel] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Remove debug message.

[rebecca]

On 2019-08-05 00:43, Dong, Eric wrote:
> This debug message may be called by BSP and APs. It may
> caused ASSERT when APs call this debug code.
>
> In order to avoid system boot assert, Remove this debug
> message.
>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>

I can't find it just now, but I seem to recall there's an associated BZ
ticket that we triaged a couple of weeks ago.


-- 
Rebecca Cran

Re: [edk2-devel] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Remove debug message.

[Laszlo Ersek]

On 08/07/19 19:17, Rebecca Cran wrote:
> On 2019-08-05 00:43, Dong, Eric wrote:
>> This debug message may be called by BSP and APs. It may
>> caused ASSERT when APs call this debug code.
>>
>> In order to avoid system boot assert, Remove this debug
>> message.
>>
>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> I can't find it just now, but I seem to recall there's an associated BZ
> ticket that we triaged a couple of weeks ago.

Possibly <https://bugzilla.tianocore.org/show_bug.cgi?id=1984>?

Thanks
Laszlo

[Patch v2 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove debug message.

[Dong, Eric]

This debug message may be called by BSP and APs. It may
caused ASSERT when APs call this debug code.

In order to avoid system boot assert, Remove this debug
message.

Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 2cfc61b2c6..d20bc4aae6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -1,7 +1,7 @@
 /** @file
 Code for Processor S3 restoration
 
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -90,8 +90,6 @@ UINT8                        mApHltLoopCodeTemplate[] = {
                                0xEB, 0xFC               // jmp $-2
                                };
 
-CHAR16 *mRegisterTypeStr[] = {L"MSR", L"CR", L"MMIO", L"CACHE", L"SEMAP", L"INVALID" };
-
 /**
   Sync up the MTRR values for all processors.
 
@@ -189,7 +187,6 @@ ProgramProcessorRegister (
   UINT32                    PackageThreadsCount;
   UINT32                    CurrentThread;
   UINTN                     ProcessorIndex;
-  UINTN                     ThreadIndex;
   UINTN                     ValidThreadCount;
   UINT32                    *ValidCoreCountPerPackage;
 
@@ -202,23 +199,6 @@ ProgramProcessorRegister (
 
     RegisterTableEntry = &RegisterTableEntryHead[Index];
 
-    DEBUG_CODE_BEGIN ();
-      if (ApLocation != NULL) {
-        AcquireSpinLock (&CpuFlags->ConsoleLogLock);
-        ThreadIndex = ApLocation->Package * CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount +
-              ApLocation->Core * CpuStatus->MaxThreadCount +
-              ApLocation->Thread;
-        DEBUG ((
-          DEBUG_INFO,
-          "Processor = %lu, Entry Index %lu, Type = %s!\n",
-          (UINT64)ThreadIndex,
-          (UINT64)Index,
-          mRegisterTypeStr[MIN ((REGISTER_TYPE)RegisterTableEntry->RegisterType, InvalidReg)]
-          ));
-        ReleaseSpinLock (&CpuFlags->ConsoleLogLock);
-      }
-    DEBUG_CODE_END ();
-
     //
     // Check the type of specified register
     //
-- 
2.21.0.windows.1

Re: [Patch v2 0/2] UefiCpuPkg: Remove debug message.

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Dong, Eric
> Sent: Monday, August 5, 2019 2:44 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [Patch v2 0/2] UefiCpuPkg: Remove debug message.
> 
> This debug message may be called by BSP and APs. It may caused ASSERT
> when APs call this debug code.
> 
> In order to avoid system boot assert, Remove this debug message.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Eric Dong (2):
>   UefiCpuPkg/RegisterCpuFeaturesLib: Remove debug message.
>   UefiCpuPkg/PiSmmCpuDxeSmm: Remove debug message.
> 
>  .../CpuFeaturesInitialize.c                   | 22 -------------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 22 +------------------
>  2 files changed, 1 insertion(+), 43 deletions(-)
> 
> --
> 2.21.0.windows.1

Re: [Patch v2 0/2] UefiCpuPkg: Remove debug message.

[Laszlo Ersek]

On 08/05/19 08:43, Eric Dong wrote:
> This debug message may be called by BSP and APs. It may
> caused ASSERT when APs call this debug code.
> 
> In order to avoid system boot assert, Remove this debug
> message.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Eric Dong (2):
>   UefiCpuPkg/RegisterCpuFeaturesLib: Remove debug message.
>   UefiCpuPkg/PiSmmCpuDxeSmm: Remove debug message.
> 
>  .../CpuFeaturesInitialize.c                   | 22 -------------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 22 +------------------
>  2 files changed, 1 insertion(+), 43 deletions(-)
> 

It seems to me that, after these patches are applied, no uses of
"ConsoleLogLock" remain, in either module (RegisterCpuFeaturesLib and
PiSmmCpuDxeSmm).

Can we eliminate the field from both modules? Otherwise we'll be left
with a useless, initialized spinlock, in each of these modules.

Thanks
Laszlo

Re: [Patch v2 0/2] UefiCpuPkg: Remove debug message.

[Laszlo Ersek]

On 08/07/19 19:11, Laszlo Ersek wrote:
> On 08/05/19 08:43, Eric Dong wrote:
>> This debug message may be called by BSP and APs. It may
>> caused ASSERT when APs call this debug code.
>>
>> In order to avoid system boot assert, Remove this debug
>> message.
>>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>>
>> Eric Dong (2):
>>   UefiCpuPkg/RegisterCpuFeaturesLib: Remove debug message.
>>   UefiCpuPkg/PiSmmCpuDxeSmm: Remove debug message.
>>
>>  .../CpuFeaturesInitialize.c                   | 22 -------------------
>>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 22 +------------------
>>  2 files changed, 1 insertion(+), 43 deletions(-)
>>
> 
> It seems to me that, after these patches are applied, no uses of
> "ConsoleLogLock" remain, in either module (RegisterCpuFeaturesLib and
> PiSmmCpuDxeSmm).
> 
> Can we eliminate the field from both modules? Otherwise we'll be left
> with a useless, initialized spinlock, in each of these modules.

I can see that this series has been pushed already, so I've now filed a
reminder:

https://bugzilla.tianocore.org/show_bug.cgi?id=2060

Thanks
Laszlo

Re: [Patch v2 0/2] UefiCpuPkg: Remove debug message.

[Dong, Eric]

Hi Laszlo,

Good catch, I will follow up to clean it. Thanks.

Thanks,
Eric

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, August 8, 2019 2:14 AM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [Patch v2 0/2] UefiCpuPkg: Remove debug message.
> 
> On 08/07/19 19:11, Laszlo Ersek wrote:
> > On 08/05/19 08:43, Eric Dong wrote:
> >> This debug message may be called by BSP and APs. It may caused ASSERT
> >> when APs call this debug code.
> >>
> >> In order to avoid system boot assert, Remove this debug message.
> >>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >>
> >> Eric Dong (2):
> >>   UefiCpuPkg/RegisterCpuFeaturesLib: Remove debug message.
> >>   UefiCpuPkg/PiSmmCpuDxeSmm: Remove debug message.
> >>
> >>  .../CpuFeaturesInitialize.c                   | 22 -------------------
> >>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c             | 22 +------------------
> >>  2 files changed, 1 insertion(+), 43 deletions(-)
> >>
> >
> > It seems to me that, after these patches are applied, no uses of
> > "ConsoleLogLock" remain, in either module (RegisterCpuFeaturesLib and
> > PiSmmCpuDxeSmm).
> >
> > Can we eliminate the field from both modules? Otherwise we'll be left
> > with a useless, initialized spinlock, in each of these modules.
> 
> I can see that this series has been pushed already, so I've now filed a
> reminder:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2060
> 
> Thanks
> Laszlo