-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Wednesday, March 4, 2020 8:23 PM
To: devel@edk2.groups.io; Dong, Eric <eric.dong@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>; Igor Mammedov <imammedo@...>; Yao, Jiewen <jiewen.yao@...>; Justen, Jordan L <jordan.l.justen@...>; Kinney, Michael D <michael.d.kinney@...>; Philippe Mathieu-Daudé <philmd@...>; Ni, Ray <ray.ni@...>; Boris Ostrovsky <boris.ostrovsky@...>
Subject: Re: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug

Hi Eric,

On 02/28/20 04:05, Dong, Eric wrote:

Hi Laszlo,

Thanks for your patch. The change make sense base on the comments in
the data structure header file.

I also checked all the code related to this data structure. The inputs
for this data structure are CpuS3DataDxe and RegisterCpuFeaturesLib.
Both these two drivers not support CPU hot plug feature, so the real
inputs for mAcpiCpuData.NumberOfCpus is the enabled CPU number in this
system. So before and after your code change, the CPU values are same.
But the data structure comments said it can support CPU hot plug, so I
agree your code change.

Reviewed-by: Eric Dong <eric.dong@...>

while merging this patch, the edk2 CI system reported the following build warning (in the "Windows VS2019 PR" check):

https://github.com/tianocore/edk2/pull/416/checks?check_run_id=484688972
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=4568&view=logs&j=898a5c7a-7a49-5be1-c417-92a6761a8039&t=7c50f5c2-8a2c-50f9-5007-e26f12377af8&l=64

2020-03-04T11:06:29.7838532Z ERROR - Compiler #2220 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(624): the following warning is treated as an error
2020-03-04T11:06:29.7839570Z WARNING - Compiler #4244 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(624): '=': conversion from 'UINTN' to 'volatile UINT32', possible loss of data
2020-03-04T11:06:29.7841177Z WARNING - Compiler #4244 from d:\a\1\s\UefiCpuPkg\PiSmmCpuDxeSmm\CpuS3.c(659): '=': conversion from 'UINTN' to 'volatile UINT32', possible loss of data

I've suppressed that by squashing the following hunks into the patch:

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 1e0840119724..29e9ba92b453 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -621,7 +621,7 @@ InitializeCpuBeforeRebase (
} else {
ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
}
- mNumberToFinish = mNumberOfCpus - 1;
+ mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
mExchangeInfo->ApFunction = (VOID *) (UINTN) InitializeAp;

//
@@ -656,7 +656,7 @@ InitializeCpuAfterRebase (
} else {
ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
}
- mNumberToFinish = mNumberOfCpus - 1;
+ mNumberToFinish = (UINT32)(mNumberOfCpus - 1);

//
// Signal that SMM base relocation is complete and to continue initialization for all APs.

Thanks,
Laszlo