Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

["Laszlo Ersek" <lersek@redhat.com>]

On 01/13/21 07:12, Ni, Ray wrote:
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> (I guess you don't want to put Redhat copyright in the patch 1&2 so the copyright year is not updated.
> Since it's a simple change that make the logic more neat, I am ok with that.)

I don't care about updating existent (C) notices; the git history is
there for that. (Unless the package maintainer insists, of course.)

At Red Hat, we add copyright notices when we create new files.

> Will you create a pull request to merge all 3 patches together?

Sure, I'll be happy to.

Thanks!
Laszlo

> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, January 13, 2021 3:20 AM
>> To: devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>> register tables
>>
>> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
>> arrays in ACPI_CPU_DATA just so every processor in the system can have its
>> own empty register table, matched by APIC ID. This has never been useful
>> in practice.
>>
>> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
>> SMRAM
>> consumption in CpuS3.c", 2021-01-11), simply leave both
>> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
>> initialized to the zero address. This simplifies the driver, and saves
>> both normal RAM (boot services data type memory) and -- in
>> PiSmmCpuDxeSmm
>> -- SMRAM.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the OVMF
>> IA32
>>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
>>     as long as no CPUs are hot-plugged.
>>
>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
>>  1 file changed, 32 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>> index 2be335d91903..078af36cfb41 100644
>> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
>>    UINTN                      NumberOfCpus;
>>    UINTN                      NumberOfEnabledProcessors;
>>    VOID                       *Stack;
>> -  UINTN                      TableSize;
>> -  CPU_REGISTER_TABLE         *RegisterTable;
>> -  UINTN                      Index;
>> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>>    UINTN                      GdtSize;
>>    UINTN                      IdtSize;
>>    VOID                       *Gdt;
>> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
>>      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
>> -    //
>> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
>> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
>> -    ASSERT (RegisterTable != NULL);
>> -
>> -    for (Index = 0; Index < NumberOfCpus; Index++) {
>> -      Status = MpServices->GetProcessorInfo (
>> -                           MpServices,
>> -                           Index,
>> -                           &ProcessorInfoBuffer
>> -                           );
>> -      ASSERT_EFI_ERROR (Status);
>> -
>> -      RegisterTable[Index].InitialApicId      =
>> (UINT32)ProcessorInfoBuffer.ProcessorId;
>> -      RegisterTable[Index].TableLength        = 0;
>> -      RegisterTable[Index].AllocatedSize      = 0;
>> -      RegisterTable[Index].RegisterTableEntry = 0;
>> -
>> -      RegisterTable[NumberOfCpus + Index].InitialApicId      =
>> (UINT32)ProcessorInfoBuffer.ProcessorId;
>> -      RegisterTable[NumberOfCpus + Index].TableLength        = 0;
>> -      RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;
>> -      RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
>> -    }
>> -    AcpiCpuData->RegisterTable           =
>> (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
>> -    AcpiCpuData->PreSmmInitRegisterTable =
>> (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
>>    }
>>
>>    //
>> --
>> 2.19.1.3.g30247aa5d201
>>
>