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 CF7E394160C for ; Wed, 22 Nov 2023 16:41:24 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=5G8MVGM64npRhC2HD7ude4WLelVPWiEuObY/ANLvwPc=; 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=1700671283; v=1; b=CdBbdBJLQmFpcbOwURHn5/pTmIhgFzZAfdf62zfJzwYqL87EfLoY9nFbWeLYw1RLpOxJHB0p j1cdIbC+6n3B23IuY3lRZhu9+OjTjzte0K2cwRFFu8/6M4T4HddlkYegwgjnSQNDqQQY601ZqSo D1Ly46/ZBgABdMa1gCy5RE50= X-Received: by 127.0.0.2 with SMTP id 4qPjYY7687511xXOV2KPYfdu; Wed, 22 Nov 2023 08:41:23 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.24321.1700671282892901459 for ; Wed, 22 Nov 2023 08:41:23 -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-584-CIyqWKMDP8WgCbpDreoENw-1; Wed, 22 Nov 2023 11:41:19 -0500 X-MC-Unique: CIyqWKMDP8WgCbpDreoENw-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (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 2B6688007B3; Wed, 22 Nov 2023 16:41:18 +0000 (UTC) X-Received: from [10.39.193.187] (unknown [10.39.193.187]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0244B1C060AE; Wed, 22 Nov 2023 16:41:16 +0000 (UTC) Message-ID: <296e2bfa-fb27-01c7-17ed-44ed5808f92f@redhat.com> Date: Wed, 22 Nov 2023 17:41:15 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [Patch V7 2/2] UefiCpuPkg/MpInitLib: Update the comments of _CPU_MP_DATA. To: devel@edk2.groups.io, yuanhao.xie@intel.com Cc: Eric Dong , Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20231121023546.2405-1-yuanhao.xie@intel.com> <20231121023546.2405-3-yuanhao.xie@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231121023546.2405-3-yuanhao.xie@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 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: K6iH7LMRl2RLmQbUylGuz4uTx7686176AA= 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=CdBbdBJL; 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 11/21/23 03:35, Yuanhao Xie wrote: > No functional changes in this patch. >=20 > Updates the comments of _CPU_MP_DATA to delcared that duplications in > CpuMpData are present to avoid to be direct accessed and comprehended > in assembly code. CpuMpData: Intended for use in C code while > ExchangeInfo are used in assembly code in this module. (1) The space characters at the front of these last two lines seem superfluous. >=20 > This patch deletes the unnecessary comments in CpuMpData, since > CpuMpData is no longer responsible for passing information from PEI to > DXE. >=20 > Signed-off-by: Yuanhao Xie > Cc: Laszlo Ersek lersek@redhat.com > Cc: Eric Dong > Cc: Ray Ni > Cc: Rahul Kumar > Cc: Gerd Hoffmann > --- > UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 2 ++ > UefiCpuPkg/Library/MpInitLib/MpLib.h | 13 +++++++------ > 2 files changed, 9 insertions(+), 6 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc b/UefiCpuPkg/Library/= MpInitLib/MpEqu.inc > index 72af196513..317e627b58 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc > +++ b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc > @@ -67,6 +67,8 @@ endstruc > =20 > ; > ; Equivalent NASM structure of MP_CPU_EXCHANGE_INFO > +; Assembly routines should refrain from directly interacting with > +; the internal details of CPU_MP_DATA. > ; > struc MP_CPU_EXCHANGE_INFO > .StackStart: CTYPE_UINTN 1 OK! > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/Mp= InitLib/MpLib.h > index af296f6ac0..a96a6389c1 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -203,6 +203,8 @@ typedef struct _CPU_MP_DATA CPU_MP_DATA; > // MP CPU exchange information for AP reset code > // This structure is required to be packed because fixed field offsets > // into this structure are used in assembly code in this module > +// Assembly routines should refrain from directly interacting with > +// the internal details of CPU_MP_DATA. > // > typedef struct { > UINTN StackStart; OK! > @@ -239,17 +241,16 @@ typedef struct { > #pragma pack() > =20 > // > -// CPU MP Data save in memory > +// CPU MP Data save in memory, and intended for use in C code. > +// There are some duplicated fields, such as XD status, between > +// CpuMpData and ExchangeInfo. These duplications in CpuMpData > +// are present to avoid to be direct accessed and comprehended > +// in assembly code. > // (2) So, my following comment applies to both the commit message and the comment here. I think we should not attempt to explain the *direction* of the duplications. I think there have been examples for either direction -- at some point, some information was initially only used by the assembly code, and then needed duplication for the C code's sake; at another time, duplication in the the opposite direction was necessary (because stuff used by the C code was needed by the assembly code too). In particular, the statement "duplicate a field from ExchangeInfo to CpuMpData so that assembly code need not understand it" seems wrong. For easing the job of the assembly code, fields would be duplicated in the *opposite* direction (from CpuMpData to ExchangeInfo). Note that in patch#1 in this series, we indeed duplicate a field from ExchangeInfo to CpuMpData -- but there the reason is different: we want to make the C code's job easier! The "run loop" wait mode does not involve *rebooting* APs, so there is no assembly code for AP startup, and no ExchangeInfo either. Therefore, we're making the C code's job easier, so that it can function with just CpuMpData. Thus, I would rather avoid any language on the *direction* of duplication. I'd just say: MP_CPU_EXCHANGE_INFO is to be consumed by assembly code only, and CPU_MP_DATA is to be consumed by C code only. If both kinds of code need to consume a piece of information, then that information must be duplicated between both structures. ... I'm sorry that I'm splitting hairs for so long on this issue, but I find this principle behind the duplications *really* non-obvious. So I would like us to get the documentation right. (Ray's original explanation was here: .) > struct _CPU_MP_DATA { > UINT64 CpuInfoInHob; > UINT32 CpuCount; > UINT32 BspNumber; > - // > - // The above fields data will be passed from PEI to DXE > - // Please make sure the fields offset same in the different > - // architecture. > - // > SPIN_LOCK MpLock; > UINTN Buffer; > UINTN CpuApStackSize; This is OK. Thanks! Laszlo -=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 (#111615): https://edk2.groups.io/g/devel/message/111615 Mute This Topic: https://groups.io/mt/102721681/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-