Re: [Patch v3 5/6] UefiCpuPkg/CpuS3DataDxe: Keep old data if value already existed.

[Laszlo Ersek <lersek@redhat.com>]

Hi Eric,

On 10/18/18 09:34, Eric Dong wrote:
> AcpiCpuData add new fields, keep these fields if old data already existed.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> ---
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index ef98239844..1b847e453a 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -259,6 +259,8 @@ CpuS3DataInitialize (
>    if (OldAcpiCpuData != NULL) {
>      AcpiCpuData->RegisterTable           = OldAcpiCpuData->RegisterTable;
>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
> +    AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
> +    CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
>    } else {
>      //
>      // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> 

sorry my earlier point about CpuS3DataDxe may not have been clear.

OVMF does not include any PEIM that consumes RegisterCpuFeaturesLib, so
it does not set PcdCpuS3DataAddress, in the PEI phase (or in the DXE
phase elsewhere, for that matter). That is, in this code, OVMF takes the
other branch: "Allocate buffer for empty RegisterTable and
PreSmmInitRegisterTable for all CPUs".

This series does not extend that branch, and as a result, the new fields
are all zero-filled. (From the AllocateZeroPages() function call near
the top of the CpuS3DataInitialize() function.)

But, in patch #4, in PiSmmCpuDxeSmm, the GetAcpiCpuData() function calls
AllocateCopyPool() -- copying data from normal RAM into SMRAM -- on the
following pointers:
- (UINT32 *)(UINTN)AcpiCpuData->CpuStatus.ValidCoreCountPerPackage
- (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)AcpiCpuData->ApLocation

These pointers will be NULL at that time. That's a problem in particular
for "ApLocation", because the byte count of the copy is

  mAcpiCpuData.NumberOfCpus * sizeof (EFI_CPU_PHYSICAL_LOCATION)

which is nonzero (even on OVMF).


Now, what I'm suggesting is *not* that we extend this "other" branch in
CpuS3DataInitialize(), so that we fill in the new fields sensibly. That
can't be done in a platform-independent manner. For example, getting the
CPU topology could be platform dependent.

However, I am requesting that platforms be allowed to continue working
with CpuS3DataDxe and PiSmmCpuDxeSmm even if they do not need the new
feature (i.e. if they do not set any MSRs at S3 resume). That means that
GetAcpiCpuData() in PiSmmCpuDxeSmm should work if the new fields are all
zero.

For explaining myself better, please consider a new Feature PCD that
declares whether a platform sets MSRs at S3 resume. If it doesn't, then
it doesn't need the new semaphore feature either. Then, if the
FeaturePCD is FALSE, and the register stable *still* contains a
semaphore / dependency operation, we can trigger an assert.

Now, I know that new FeaturePCDs are not welcome. And, I don't really
want to introduce one, I just used it as an example, for illustration.
Because we can determine the same condition *dynamically* in
PiSmmCpuDxeSmm. (And, perhaps we should document it in patch #1,
"AcpiCpuData.h", as well.) For example:

    If the platform does not support MSR setting at S3 resume, and
    therefore it doesn't need the dependency semaphores, it should set
    both the CpuStatus.ValidCoreCountPerPackage field, and the
    ApLocation field, to 0.

Then CpuS3DataDxe will need no further change beyond the present patch;
however PiSmmCpuDxeSmm should check for those zero values, and disable
the new feature dynamically.

Thanks
Laszlo