Re: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

Got it. Go ahead.

Thanks,
Eric

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Wednesday, March 4, 2020 8:23 PM
To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Igor Mammedov <imammedo@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Ni, Ray <ray.ni@intel.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>
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@intel.com>

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