Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek <lersek@redhat.com>]

Hi Eric,

On 08/08/18 09:40, Eric Dong wrote:
> V1 changes:
>> Current code logic can't confirm CpuS3DataDxe driver start before
>> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
>> Add implementation for AllocateAcpiCpuData function to remove this
>> assumption.
>
> V2 changes:
>> Because CpuS3Data memory will be copy to smram at SmmReadToLock point,
>> so the memory type no need to be ACPI NVS type, also the address not
>> limit to below 4G.
>> This change remove the limit of ACPI NVS memory type and below 4G.
>
> Pass OS boot and resume from S3 test.
>
> Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959
>
> Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Cc: Fan Jeff <vanjeff_919@hotmail.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
>  .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
>  .../RegisterCpuFeaturesLib.c                       |  90 ++++++++++++++
>  3 files changed, 90 insertions(+), 198 deletions(-)

In order to review this patch, I created the following text files:

- allocate-pei.txt
- allocate-dxe.txt
- allocate-common.txt

- enlarge-pei.txt
- enlarge-dxe.txt
- enlarge-common.txt

This way I can compare the PEI<->DXE variants before the patch, and I
can also compare each of PEI/DXE to the combined version.

For AllocateAcpiCpuData(), I have the following comments:

(1) The common implementation still says "Allocates ACPI NVS memory to
save ACPI_CPU_DATA" in the leading comment block. Please update that to
"boot services data".

(2) The AllocateAcpiCpuData() function declaration should be removed
from the "RegisterCpuFeatures.h" file -- this function is now internal.
(Preferably, the function should be made STATIC as well.)

(3) When diffing "allocate-pei.txt" against "allocate-common.txt", I'm
noticing whitespace changes that don't look justified:

> -  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> +  AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus;

and

> -  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
> -  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
> +  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN) RegisterTable;
> +  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN) (RegisterTable + NumberOfCpus);

Inserting a space character between "(type)" and "expression" in a cast
expression is extremely bad practice in edk2, IMO. As I mentioned
earlier, the binding of this operator is one of the strongest bindings
in the C language, and inserting a space character before the expression
makes a misleading and counter-intuitive impression about that.

If you really want to insert that space, I guess I'll have to live with
that, but I'd like to highlight that it's bad practice, and (to my
knowledge) it isn't required by the edk2 coding style.


About the EnlargeRegisterTable() function:

(4) The function declaration should be removed from
"RegisterCpuFeatures.h" -- the function is now internal. (Preferably, it
should be made STATIC too.)

(5) Some more whitespace in cast expressions that I find questionable,
near AllocatePages() and FreePages() :)


What do you think? Thanks!
Laszlo