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 B2B76D8107F for ; Tue, 7 Nov 2023 10:26:19 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=Nn3qg3LMeYW9vgF2/FBzOryBtfZJAdo7dJKvJPKb9Hg=; 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=1699352778; v=1; b=xYc1OV6zzVwk5ALMzPS02nJWKtat0CNF9qFgk7991BosxxKNbIPUwnATEfdZvmIfJaiM0LUS Tpw/dYqYF++6vNMnYlbGciaVEW+v6WmF3yF4Vw1DMjE0p+G2gIKbrPHZqWosEsPogP8fWx7aw7y jP0Nf9j+BsejbIuuQ+thTQ9A= X-Received: by 127.0.0.2 with SMTP id Pk9mYY7687511xWUmd0vZUdl; Tue, 07 Nov 2023 02:26:18 -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.7493.1699352777455655968 for ; Tue, 07 Nov 2023 02:26:17 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-257-4MlTweMZNRSaeUS6X2sgcA-1; Tue, 07 Nov 2023 05:26:11 -0500 X-MC-Unique: 4MlTweMZNRSaeUS6X2sgcA-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (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 A568F2806057; Tue, 7 Nov 2023 10:26:10 +0000 (UTC) X-Received: from [10.39.193.64] (unknown [10.39.193.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 61DA225C0; Tue, 7 Nov 2023 10:26:09 +0000 (UTC) Message-ID: Date: Tue, 7 Nov 2023 11:26:08 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class To: devel@edk2.groups.io, jiaxin.wu@intel.com Cc: Eric Dong , Ray Ni , Zeng Star , Gerd Hoffmann , Rahul Kumar References: <20231103153012.3704-1-jiaxin.wu@intel.com> <20231103153012.3704-4-jiaxin.wu@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231103153012.3704-4-jiaxin.wu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 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: kFvLLt34MR2cOaszVRqqq2Jgx7686176AA= 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=xYc1OV6z; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 11/3/23 16:30, Wu, Jiaxin wrote: > Intel is planning to provide different SMM CPU Sync implementation > along with some specific registers to improve the SMI performance, > hence need SmmCpuSyncLib Library for Intel. >=20 > This patch is to: > 1.Adds SmmCpuSyncLib Library class in UefiCpuPkg.dec. > 2.Adds SmmCpuSyncLib.h function declaration header file. >=20 > Change-Id: Ib7f5e317526e8b9e29b65e072bdb485dbd678817 > Cc: Eric Dong > Cc: Ray Ni > Cc: Zeng Star > Cc: Gerd Hoffmann > Cc: Rahul Kumar > Signed-off-by: Jiaxin Wu > --- > UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 191 +++++++++++++++++++++++= ++++++ > UefiCpuPkg/UefiCpuPkg.dec | 3 + > 2 files changed, 194 insertions(+) > create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h >=20 > diff --git a/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h b/UefiCpuPkg/Incl= ude/Library/SmmCpuSyncLib.h > new file mode 100644 > index 0000000000..b9b190c516 > --- /dev/null > +++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h > @@ -0,0 +1,191 @@ > +/** @file > +Library that provides SMM CPU Sync related operations. > + > +Copyright (c) 2023, Intel Corporation. All rights reserved.
> +SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#ifndef SMM_CPU_SYNC_LIB_H_ > +#define SMM_CPU_SYNC_LIB_H_ > + > +#include > + > +/** > + Creates and Init a new Smm Cpu Sync context. > + > + @param[in] NumberOfCpus The number of processors in the syste= m. > + > + @return Pointer to an allocated Smm Cpu Sync context object. > + If the creation failed, returns NULL. > + > +**/ > +VOID * > +EFIAPI > +SmmCpuSyncContextInit ( > + IN UINTN NumberOfCpus > + ); (1) I agree that the internals of the context should be hidden, but (VOID*) is not the right way. Instead, please use an incomplete structure type. That is, in the library header class file, do the following: typedef struct SMM_CPU_SYNC_CTX SMM_CPU_SYNC_CTX; and then make all these functions return and take SMM_CPU_SYNC_CTX * The library users will still not know what is inside SMM_CPU_SYNC_CTX, just like with (VOID*), but the compiler will be able to perform much stronger type checking than with (VOID*). And then in the library *instance(s)*, you can complete the incomplete type= : struct SMM_CPU_SYNC_CTX { ... }; > + > +/** > + Deinit an allocated Smm Cpu Sync context object. > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t to be released. > + > +**/ > +VOID > +EFIAPI > +SmmCpuSyncContextDeinit ( > + IN VOID *SmmCpuSyncCtx > + ); > + > +/** > + Reset Smm Cpu Sync context object. > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t to be released. > + > +**/ > +VOID > +EFIAPI > +SmmCpuSyncContextReset ( > + IN VOID *SmmCpuSyncCtx > + ); > + (2) The documentation on these two functions (deinit and reset) is contradictory. Both say "object to be released". That cannot be right. I'm sure one of these functions just internally resets the object, but doesn't actually *free* the SMRAM; while the other function releases the SMRAM too. So please clean up the comments. (3) By definition, in a function that resets the internals of an object, you cannot specify "IN" for that function. It must be OUT. > +/** > + Get current arrived CPU count. > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t to be released. (4) Bogus comment. > + > + @return Current number of arrived CPU count. > + -1: indicate the door has been locked. (5) The return type is UINT32, so -1 is somewhat confusing. Either write MAX_UINT32 or (UINT32)-1, for clarity, please. (6) More importantly -- what is "arrived CPU count", and what is "door has been locked"? Please add a large comment to the top of the library class header that explains the operation / concepts of the context. What operations and behaviors are defined for this data type? > + > +**/ > +UINT32 > +EFIAPI > +SmmCpuSyncGetArrivedCpuCount ( > + IN VOID *SmmCpuSyncCtx > + ); > + > +/** > + Performs an atomic operation to check in CPU. > + Check in CPU successfully if the returned arrival CPU count value is > + positive, otherwise indicate the door has been locked, the CPU can > + not checkin. I hope this will be understandable after you explain the data type comprehensively. > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm CPU Sync context objec= t to be released. (7) bogus comment > + @param[in] CpuIndex Pointer to the CPU Index to checkin. (8) This parameter is not a pointer. :/ > + > + @return Positive value (>0): CPU arrival count number after ch= eck in CPU successfully. > + Nonpositive value (<=3D0): check in CPU failure. (9) To be honest, I don't like how inconsistent these return types and values are. - in SmmCpuSyncContextInit(), you return NULL on failure. - in SmmCpuSyncGetArrivedCpuCount(), you return MAX_UINT32 on failure; and on success, you can use return values larger than MAX_INT32. - in SmmCpuSyncCheckInCpu() below, you return -1 on failure; and on success, you *cannot* use return values larger than MAX_INT32, for representing the arrived CPU count. Please clean up this mess. All functions should have RETURN_STATUS for return type, and all results should be produced via OUT paramaters. (For example, the Init() function can fail for two reasons, minimally: (a) the allocated memory almost certainly depends on the NumberOfCpus parameter, so you'll have a multiplication there, and all multiplications must be checked against integer overflow, (b) if the multiplication succeeds, you can still run out of SMRAM. It's nice to return different error codes for (a) and (b)) Furthermore, counts of objects should be generally represented as UINTN values, unless you have a good (explained!) reason for using a different type. > + > +**/ > +INT32 > +EFIAPI > +SmmCpuSyncCheckInCpu ( > + IN VOID *SmmCpuSyncCtx, (10) "IN" is bogus here; when this function succeeds, it obviously changes the internals of context. Make it "IN OUT" perhaps. (Don't forget to update the corresponding @param documentation either!) > + IN UINTN CpuIndex > + ); > + > +/** > + Performs an atomic operation to check out CPU. > + Check out CPU successfully if the returned arrival CPU count value is > + nonnegative, otherwise indicate the door has been locked, the CPU can > + not checkout. > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t to be released. > + @param[in] CpuIndex Pointer to the Cpu Index to checkout. > + > + @return Nonnegative value (>=3D0): CPU arrival count number after = check out CPU successfully. > + Negative value (<0): Check out CPU failure. > + > + > +**/ > +INT32 > +EFIAPI > +SmmCpuSyncCheckOutCpu ( > + IN VOID *SmmCpuSyncCtx, > + IN UINTN CpuIndex > + ); (11) Remarks (7) through (10) apply here as well. > + > +/** > + Performs an atomic operation lock door for CPU checkin or checkout. > + With this function, CPU can not check in via SmmCpuSyncCheckInCpu () o= r > + check out via SmmCpuSyncCheckOutCpu (). > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t to be released. (12) bogus comment > + @param[in] CpuIndex Pointer to the Cpu Index to lock door. (13) impossible to make any sense of, without the comprehensive data type description > + > + @return CPU arrival count number. (14) why is it necessary for outputting the arrived number when locking? > + > +**/ > +UINT32 > +EFIAPI > +SmmCpuSyncLockDoor ( > + IN VOID *SmmCpuSyncCtx, (15) should be IN OUT (in the @param desc too) > + IN UINTN CpuIndex > + ); > + > +/** > + Used for BSP to wait all APs. (16) This is a new library class, let's try to make it grammatically correct and understandable. "Used by the BSP to wait for all the APs". > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t. > + @param[in] NumberOfAPs Number of APs need to wait. (17) "Number of APs to wait for". The current explanation suggests we are making some APs wait. > + @param[in] BspIndex Pointer to the BSP Index. (18) Not a pointer. > + > +**/ > +VOID > +EFIAPI > +SmmCpuSyncWaitForAllAPs ( > + IN VOID *SmmCpuSyncCtx, (19) Almost certainly IN OUT (-> @param too) > + IN UINTN NumberOfAPs, > + IN UINTN BspIndex > + ); > + > +/** > + Used for BSP to release one AP. (20) "used by" > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t. > + @param[in] CpuIndex Pointer to the Cpu Index, indicate which = AP need to be released. > + @param[in] BspIndex Pointer to the BSP Index. (21) neither index is a pointer :/ > + > +**/ > +VOID > +EFIAPI > +SmmCpuSyncReleaseOneAp ( > + IN VOID *SmmCpuSyncCtx, (22) IN OUT etc sorry I'm exhausted; this is shoddy code. Please take much more care and invest more time in the details next time. Laszlo > + IN UINTN CpuIndex, > + IN UINTN BspIndex > + ); > + > +/** > + Used for AP to wait BSP. > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t. > + @param[in] CpuIndex Pointer to the Cpu Index, indicate which = AP wait BSP. > + @param[in] BspIndex Pointer to the BSP Index. > + > +**/ > +VOID > +EFIAPI > +SmmCpuSyncWaitForBsp ( > + IN VOID *SmmCpuSyncCtx, > + IN UINTN CpuIndex, > + IN UINTN BspIndex > + ); > + > +/** > + Used for AP to release BSP. > + > + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context objec= t. > + @param[in] CpuIndex Pointer to the Cpu Index, indicate which = AP release BSP. > + @param[in] BspIndex Pointer to the BSP Index. > + > +**/ > +VOID > +EFIAPI > +SmmCpuSyncReleaseBsp ( > + IN VOID *SmmCpuSyncCtx, > + IN UINTN CpuIndex, > + IN UINTN BspIndex > + ); > + > +#endif > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec > index 0b5431dbf7..20ab079219 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -62,10 +62,13 @@ > CpuPageTableLib|Include/Library/CpuPageTableLib.h > =20 > ## @libraryclass Provides functions for manipulating smram savestate= registers. > MmSaveStateLib|Include/Library/MmSaveStateLib.h > =20 > + ## @libraryclass Provides functions for SMM CPU Sync Operation. > + SmmCpuSyncLib|Include/Library/SmmCpuSyncLib.h > + > [LibraryClasses.RISCV64] > ## @libraryclass Provides functions to manage MMU features on RISCV6= 4 CPUs. > ## > RiscVMmuLib|Include/Library/BaseRiscVMmuLib.h > =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 (#110834): https://edk2.groups.io/g/devel/message/110834 Mute This Topic: https://groups.io/mt/102366300/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-