From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 C44FC2116DA13 for ; Mon, 15 Oct 2018 09:02:48 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3B3B33086267; Mon, 15 Oct 2018 16:02:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-109.rdu2.redhat.com [10.10.121.109]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4C27B61548; Mon, 15 Oct 2018 16:02:47 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20181015024948.228-1-eric.dong@intel.com> <20181015024948.228-2-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <7d4916dc-7d3f-3609-44de-1b9472ad237e@redhat.com> Date: Mon, 15 Oct 2018 18:02:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181015024948.228-2-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Mon, 15 Oct 2018 16:02:48 +0000 (UTC) Subject: Re: [Patch 1/4] UefiCpuPkg/Include/AcpiCpuData.h: Add Semaphore related Information. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Oct 2018 16:02:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/15/18 04:49, Eric Dong wrote: > In order to support semaphore related logic, add new definition for it. > > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > UefiCpuPkg/Include/AcpiCpuData.h | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) (1) If it's possible, I suggest moving the (very nice) description from the 0/4 cover letter to this patch. The cover letter is not captured in the git commit history. I don't insist, but it would be a nice touch, IMO. > > diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h > index 9e51145c08..b3cf2f664a 100644 > --- a/UefiCpuPkg/Include/AcpiCpuData.h > +++ b/UefiCpuPkg/Include/AcpiCpuData.h > @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > #ifndef _ACPI_CPU_DATA_H_ > #define _ACPI_CPU_DATA_H_ > > +#include > + > // > // Register types in register table > // > @@ -22,9 +24,20 @@ typedef enum { > Msr, > ControlRegister, > MemoryMapped, > - CacheControl > + CacheControl, > + Semaphore > } REGISTER_TYPE; > > +// > +// CPU information. > +// > +typedef struct { > + UINT32 PackageCount; // Packages in this CPU. (2) Is it possible to have multiple packages in a single CPU? If not, then please clean up the comment. Did you perhaps mean "number of sockets in the system"? > + UINT32 CoreCount; // Max Core count in the packages. > + UINT32 ThreadCount; // MAx thread count in the cores. (3) The word "MAx" should be "Max", I think. > + UINT32 *ValidCoresInPackages; // Valid cores in each package. (4) Is it possible to document the structure of this array (?) in some detail? Other parts of "UefiCpuPkg/Include/AcpiCpuData.h" are very well documented. > +} CPU_STATUS_INFORMATION; > + > // > // Element of register table entry > // > @@ -147,6 +160,14 @@ typedef struct { > // provided. > // > UINT32 ApMachineCheckHandlerSize; > + // > + // CPU information which is required when set the register table. > + // > + CPU_STATUS_INFORMATION CpuStatus; > + // > + // Location info for each ap. (5) This header file spells "AP" in upper case elsewhere. > + // > + EFI_CPU_PHYSICAL_LOCATION *ApLocation; (6) Is this supposed to be an array? If so, what is the structure of the array? What is the size? (7) This is the first field in ACPI_CPU_DATA that has pointer type. Other pointers are represented as EFI_PHYSICAL_ADDRESS. What justifies this difference? > } ACPI_CPU_DATA; > > #endif > (8) "UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c" will zero-fill the new fields. Is that safe? Thanks Laszlo