From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0AEC8210C6684 for ; Wed, 8 Aug 2018 07:45:28 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4B62040241C3; Wed, 8 Aug 2018 14:45:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-208.rdu2.redhat.com [10.10.120.208]) by smtp.corp.redhat.com (Postfix) with ESMTP id 48EDD1C735; Wed, 8 Aug 2018 14:45:26 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20180808074006.21360-1-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <667e1392-c895-2063-a10a-7e6d19455010@redhat.com> Date: Wed, 8 Aug 2018 16:45:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180808074006.21360-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 08 Aug 2018 14:45:27 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 08 Aug 2018 14:45:27 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Aug 2018 14:45:29 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 > Suggested-by: Fan Jeff > Cc: Marvin Häuser > Cc: Fan Jeff > Cc: Laszlo Ersek > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > .../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