From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id DBB0C941A62 for ; Fri, 5 Jan 2024 13:59:33 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=1MOMLykSfv4+Lmo1oy/FUAnTJ9QFTk+81cRzFlPJqjs=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1704463172; v=1; b=uUEYhlOFgCAIDJvA4pxpQ3A4b407qoCyj6wM7xAuPl2utYI6v2j9nMkiXlIPU0a2Ha/T9c/b 5VNfXl5sRb9wgdY+XGi1E/rDtkeVVm9L2D69ybXrrz2I28uViosLS7FH/n3/ye/BCfZCcQ24d2Y cN0gN02GdD7hKJKWkpUxa3X4= X-Received: by 127.0.0.2 with SMTP id liiYYY7687511x0GxA102sMK; Fri, 05 Jan 2024 05:59:32 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.23885.1704463171978909991 for ; Fri, 05 Jan 2024 05:59:32 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-193-5SDBZ7wtM62ydveOLasgJA-1; Fri, 05 Jan 2024 08:59:28 -0500 X-MC-Unique: 5SDBZ7wtM62ydveOLasgJA-1 X-Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8F8398371C1; Fri, 5 Jan 2024 13:59:27 +0000 (UTC) X-Received: from [10.39.193.6] (unknown [10.39.193.6]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 86844492BE6; Fri, 5 Jan 2024 13:59:26 +0000 (UTC) Message-ID: Date: Fri, 5 Jan 2024 14:59:25 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: Retrive EXTENDED_PROCESSOR_INFORMATION To: "Ni, Ray" , "Tan, Dun" , "devel@edk2.groups.io" Cc: "Kumar, Rahul R" , Gerd Hoffmann , "Xu, Min M" References: <20240104073216.1327-1-dun.tan@intel.com> <20240104073216.1327-2-dun.tan@intel.com> <9816c694-e5b1-1753-078e-66dfdd5f15bb@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: pLhVFpZaTpzJhd93TM4It57bx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=uUEYhlOF; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 1/5/24 13:56, Ni, Ray wrote: > Laszlo, > Good suggestion. >=20 > Your solution will not work if in future some extra fields might require = to be set to non-zero. > But future is not coming yet. I agree with your approach. Well, if we need to set some fields to nonzero, manual assignments will become necessary either way, with or without the ZeroMem(). With the ZeroMem(), we just overwrite the zero values later. I certainly agree that there is a tipping point. Like, if we have 5 fields, and we need to set 4 of them to nonzero values, then an initial ZeroMem is wasteful. Right now the ZeroMem() looks much more frugal, and a bit more future-proof too. Thanks! Laszlo >=20 > Thanks, > Ray >> -----Original Message----- >> From: Tan, Dun >> Sent: Friday, January 5, 2024 5:25 PM >> To: Laszlo Ersek ; devel@edk2.groups.io >> Cc: Ni, Ray ; Kumar, Rahul R = ; >> Gerd Hoffmann ; Xu, Min M >> Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg: Retrive >> EXTENDED_PROCESSOR_INFORMATION >> >> Hi Laszlo, >> >> Thanks for your comments. I agree with your solution. It seems simpler a= nd >> clearer. Will change the code and keep the additional function comments = in >> next version patch set. >> >> Thanks, >> Dun >> >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Thursday, January 4, 2024 10:53 PM >> To: devel@edk2.groups.io; Tan, Dun >> Cc: Ni, Ray ; Kumar, Rahul R = ; >> Gerd Hoffmann ; Xu, Min M >> Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg: Retrive >> EXTENDED_PROCESSOR_INFORMATION >> >> On 1/4/24 08:32, duntan wrote: >>> Retrive EXTENDED_PROCESSOR_INFORMATION in the API >>> MpInitLibGetProcessorInfo() of MpInitLibUp instance when the BIT24 of >>> input ProcessorNumber is set. >>> It's to align with the behavior in PEI/DXE MpInitLib >>> >>> Signed-off-by: Dun Tan >>> Cc: Ray Ni >>> Cc: Laszlo Ersek >>> Cc: Rahul Kumar >>> Cc: Gerd Hoffmann >>> Cc: Min Xu >>> --- >>> UefiCpuPkg/Include/Library/MpInitLib.h | 2 ++ >>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 ++ >>> UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c | 12 ++++++++++++ >>> 3 files changed, 16 insertions(+) >>> >>> diff --git a/UefiCpuPkg/Include/Library/MpInitLib.h >>> b/UefiCpuPkg/Include/Library/MpInitLib.h >>> index 1853c46415..842c6f7ff9 100644 >>> --- a/UefiCpuPkg/Include/Library/MpInitLib.h >>> +++ b/UefiCpuPkg/Include/Library/MpInitLib.h >>> @@ -63,6 +63,8 @@ MpInitLibGetNumberOfProcessors ( >>> instant this call is made. This service may only be called from the = BSP. >>> >>> @param[in] ProcessorNumber The handle number of processor. >>> + Lower 24 bits contains the actual = processor number. >>> + BIT24 indicates if the >> EXTENDED_PROCESSOR_INFORMATION will be retrived. >>> @param[out] ProcessorInfoBuffer A pointer to the buffer where >> information for >>> the requested processor is deposit= ed. >>> @param[out] HealthData Return processor health data. >>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> index a359906923..cdfb570e61 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> @@ -2333,6 +2333,8 @@ MpInitLibInitialize ( >>> instant this call is made. This service may only be called from the = BSP. >>> >>> @param[in] ProcessorNumber The handle number of processor. >>> + Lower 24 bits contains the actual = processor number. >>> + BIT24 indicates if the >> EXTENDED_PROCESSOR_INFORMATION will be retrived. >>> @param[out] ProcessorInfoBuffer A pointer to the buffer where >> information for >>> the requested processor is deposit= ed. >>> @param[out] HealthData Return processor health data. >>> diff --git a/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c >>> b/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c >>> index 86f9fbf903..3af4911d4b 100644 >>> --- a/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c >>> +++ b/UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.c >>> @@ -77,6 +77,8 @@ MpInitLibGetNumberOfProcessors ( >>> instant this call is made. This service may only be called from the = BSP. >>> >>> @param[in] ProcessorNumber The handle number of processor. >>> + Lower 24 bits contains the actual = processor number. >>> + BIT24 indicates if the >> EXTENDED_PROCESSOR_INFORMATION will be retrived. >>> @param[out] ProcessorInfoBuffer A pointer to the buffer where >> information for >>> the requested processor is deposit= ed. >>> @param[out] HealthData Return processor health data. >>> @@ -115,6 +117,16 @@ MpInitLibGetProcessorInfo ( >>> ProcessorInfoBuffer->Location.Package =3D 0; >>> ProcessorInfoBuffer->Location.Core =3D 0; >>> ProcessorInfoBuffer->Location.Thread =3D 0; >>> + >>> + if ((ProcessorNumber & CPU_V2_EXTENDED_TOPOLOGY) !=3D 0) { >>> + ProcessorInfoBuffer->ExtendedInformation.Location2.Package =3D 0; >>> + ProcessorInfoBuffer->ExtendedInformation.Location2.Die =3D 0; >>> + ProcessorInfoBuffer->ExtendedInformation.Location2.Tile =3D 0; >>> + ProcessorInfoBuffer->ExtendedInformation.Location2.Module =3D 0; >>> + ProcessorInfoBuffer->ExtendedInformation.Location2.Core =3D 0; >>> + ProcessorInfoBuffer->ExtendedInformation.Location2.Thread =3D 0; >>> + } >>> + >>> if (HealthData !=3D NULL) { >>> GuidHob =3D GetFirstGuidHob (&gEfiSecPlatformInformationPpiGuid); >>> if (GuidHob !=3D NULL) { >> >> (1) For the UP implementation of MpInitLibGetProcessorInfo(): >> >> How about, for a *complete* solution (covering both pre-patch and post- >> patch functionality): >> >> ZeroMem (ProcessorInfoBuffer, sizeof *ProcessorInfoBuffer); >> ProcessorInfoBuffer->StatusFlag =3D PROCESSOR_AS_BSP_BIT | >> PROCESSOR_ENABLED_BIT | >> PROCESSOR_HEALTH_STATUS_BIT; >> >> This approach is not slow (most of the time I expect the platform will h= ave an >> optimized ZeroMem() implementation), it is frugal with code (replaces a >> bunch of manual zeroing of fields), and it is relatively future-proof (t= he next >> time EFI_PROCESSOR_INFORMATION is extended, you likely won't have to >> touch up the code again, because the ZeroMem() will cover the new fields >> automatically). >> >> Also, this approach will zero out >> ProcessorInfoBuffer->ExtendedInformation *regardless* of BIT24 in the >> input, which I kind of consider an advantage! (No garbage in the output >> structure.) Again, I don't think the zeroing is wasteful, regarding CPU = cycles. >> >> I do agree that the leading function comments should mention BIT24 >> >> Thanks >> Laszlo >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113288): https://edk2.groups.io/g/devel/message/113288 Mute This Topic: https://groups.io/mt/103518742/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-