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 <[email protected]> > Suggested-by: Fan Jeff <[email protected]> > Cc: Marvin Häuser <[email protected]> > Cc: Fan Jeff <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Cc: Ruiyu Ni <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <[email protected]> > --- > .../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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
