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 6332D7803D0 for ; Tue, 12 Dec 2023 20:19:10 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=nccJkwq6rumySKDmlbaEC/bMSw+lSh8BIFM8f0HQEz8=; 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=1702412349; v=1; b=BCf9iL6POALta5ZtSvJ6fKMSMl253WpqnOrDQUYGIdCSfnlxzRH25M41mfssBxzwztKXasEn rQa0FrA8Q5ilgHfA3m19OB7k5mwE0M1O5dNCaq0r7IKXSRZkCgb7vhcUQgtfLAdpU1JbhHHtYb1 mC1EckRS/hLJH9pMONvFDkFg= X-Received: by 127.0.0.2 with SMTP id 7SRdYY7687511xLbKZcM71nf; Tue, 12 Dec 2023 12:19:09 -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.6753.1702412348053751573 for ; Tue, 12 Dec 2023 12:19:08 -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-211-gUmAk6JbNZudRv81D0oAMg-1; Tue, 12 Dec 2023 15:19:03 -0500 X-MC-Unique: gUmAk6JbNZudRv81D0oAMg-1 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (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 693993C2AF60; Tue, 12 Dec 2023 20:19:02 +0000 (UTC) X-Received: from [10.39.192.120] (unknown [10.39.192.120]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F396A51E3; Tue, 12 Dec 2023 20:19:00 +0000 (UTC) Message-ID: <302017ed-f9da-bf2c-3f95-6dd53395c335@redhat.com> Date: Tue, 12 Dec 2023 21:18:59 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/6] 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: <20231206100122.8028-1-jiaxin.wu@intel.com> <20231206100122.8028-3-jiaxin.wu@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231206100122.8028-3-jiaxin.wu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 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: niXdMS0BKyJfFoJEIhp0ogNSx7686176AA= 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=BCf9iL6P; 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 12/6/23 11:01, 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 > For the new SmmCpuSyncLib, it provides 3 sets of APIs: >=20 > 1. ContextInit/ContextDeinit/ContextReset: > ContextInit() is called in driver's entrypoint to allocate and > initialize the SMM CPU Sync context. ContextDeinit() is called in > driver's unload function to deinitialize SMM CPU Sync context. > ContextReset() is called before CPU exist SMI, which allows CPU to > check into the next SMI from this point. >=20 > 2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor: > When SMI happens, all processors including BSP enter to SMM mode by > calling CheckInCpu(). The elected BSP calls LockDoor() so that > CheckInCpu() will return the error code after that. CheckOutCpu() can > be called in error handling flow for the CPU who calls CheckInCpu() > earlier. GetArrivedCpuCount() returns the number of checked-in CPUs. >=20 > 3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp > WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number > of APs and release one specific AP. WaitForBsp() & ReleaseBsp() are > called from APs to wait and release BSP. The 4 APIs are used to > synchronize the running flow among BSP and APs. BSP and AP Sync flow > can be easy understand as below: > BSP: ReleaseOneAp --> AP: WaitForBsp > BSP: WaitForAPs <-- AP: ReleaseBsp >=20 > Cc: Laszlo Ersek > 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 | 275 +++++++++++++++++++++++= ++++++ > UefiCpuPkg/UefiCpuPkg.dec | 3 + > 2 files changed, 278 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..0f9eb3414a > --- /dev/null > +++ b/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h > @@ -0,0 +1,275 @@ > +/** @file > + Library that provides SMM CPU Sync related operations. > + The lib provides 3 sets of APIs: > + 1. ContextInit/ContextDeinit/ContextReset: > + ContextInit() is called in driver's entrypoint to allocate and initial= ize the SMM CPU Sync context. > + ContextDeinit() is called in driver's unload function to deinitialize = the SMM CPU Sync context. > + ContextReset() is called before CPU exist SMI, which allows CPU to che= ck into the next SMI from this point. > + > + 2. GetArrivedCpuCount/CheckInCpu/CheckOutCpu/LockDoor: > + When SMI happens, all processors including BSP enter to SMM mode by ca= lling CheckInCpu(). > + The elected BSP calls LockDoor() so that CheckInCpu() will return the = error code after that. > + CheckOutCpu() can be called in error handling flow for the CPU who cal= ls CheckInCpu() earlier. > + GetArrivedCpuCount() returns the number of checked-in CPUs. > + > + 3. WaitForAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp > + WaitForAPs() & ReleaseOneAp() are called from BSP to wait the number o= f APs and release one specific AP. > + WaitForBsp() & ReleaseBsp() are called from APs to wait and release BS= P. > + The 4 APIs are used to synchronize the running flow among BSP and APs.= BSP and AP Sync flow can be > + easy understand as below: > + BSP: ReleaseOneAp --> AP: WaitForBsp > + BSP: WaitForAPs <-- AP: ReleaseBsp > + > + Copyright (c) 2023, Intel Corporation. All rights reserved.
> + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ Thanks. This documentation (in the commit message and the lib class header file) seems really good (especially with the formatting updates suggested by Ray). (1) I think there is one typo: exist <-> exits. > + > +#ifndef SMM_CPU_SYNC_LIB_H_ > +#define SMM_CPU_SYNC_LIB_H_ > + > +#include > + > +// > +// Opaque structure for SMM CPU Sync context. > +// > +typedef struct SMM_CPU_SYNC_CONTEXT SMM_CPU_SYNC_CONTEXT; > + > +/** > + Create and initialize the SMM CPU Sync context. > + > + SmmCpuSyncContextInit() function is to allocate and initialize the SMM= CPU Sync context. > + > + @param[in] NumberOfCpus The number of Logical Processors in = the system. > + @param[out] SmmCpuSyncCtx Pointer to the new created and initi= alized SMM CPU Sync context object. > + NULL will be returned if any error h= appen during init. > + > + @retval RETURN_SUCCESS The SMM CPU Sync context was success= ful created and initialized. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL. > + @retval RETURN_BUFFER_TOO_SMALL Overflow happen > + @retval RETURN_OUT_OF_RESOURCES There are not enough resources avail= able to create and initialize SMM CPU Sync context. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncContextInit ( > + IN UINTN NumberOfCpus, > + OUT SMM_CPU_SYNC_CONTEXT **SmmCpuSyncCtx > + ); > + > +/** > + Deinit an allocated SMM CPU Sync context. > + > + SmmCpuSyncContextDeinit() function is to deinitialize SMM CPU Sync con= text, the resources allocated in > + SmmCpuSyncContextInit() will be freed. > + > + Note: This function only can be called after SmmCpuSyncContextInit() r= eturn success. > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object to be deinitialized. > + > + @retval RETURN_SUCCESS The SMM CPU Sync context was success= ful deinitialized. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL. > + @retval RETURN_UNSUPPORTED Unsupported operation. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncContextDeinit ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx > + ); > + > +/** > + Reset SMM CPU Sync context. > + > + SmmCpuSyncContextReset() function is to reset SMM CPU Sync context to = the initialized state. > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object to be reset. > + > + @retval RETURN_SUCCESS The SMM CPU Sync context was success= ful reset. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncContextReset ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx > + ); (2) The file-top documentation says about this API: "ContextReset() is called before CPU exist SMI, which allows CPU to check into the next SMI from this point". It is not clear *which* CPU is supposed to call ContextReset (and the function does not take a CPU index). Can you explain this in the documentation? (Assuming my observation makes sense.) > + > +/** > + Get current number of arrived CPU in SMI. > + > + For traditional CPU synchronization method, BSP might need to know the= current number of arrived CPU in > + SMI to make sure all APs in SMI. This API can be for that purpose. > + > + @param[in] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object. > + @param[in,out] CpuCount Current count of arrived CPU in SMI. > + > + @retval RETURN_SUCCESS Get current number of arrived CPU in= SMI successfully. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx or CpuCount is NULL. > + @retval RETURN_UNSUPPORTED Unsupported operation. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncGetArrivedCpuCount ( > + IN SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx, > + IN OUT UINTN *CpuCount > + ); (3) Why is CpuCount IN OUT? I would think just OUT should suffice. > + > +/** > + Performs an atomic operation to check in CPU. > + > + When SMI happens, all processors including BSP enter to SMM mode by ca= lling SmmCpuSyncCheckInCpu(). > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object. > + @param[in] CpuIndex Check in CPU index. > + > + @retval RETURN_SUCCESS Check in CPU (CpuIndex) successfully= . > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL. > + @retval RETURN_ABORTED Check in CPU failed due to SmmCpuSyn= cLockDoor() has been called by one elected CPU. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncCheckInCpu ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx, > + IN UINTN CpuIndex > + ); (4) Do we need an error condition for CpuIndex being out of range? (5) Do we have a special CpuIndex value for the BSP? > + > +/** > + Performs an atomic operation to check out CPU. > + > + CheckOutCpu() can be called in error handling flow for the CPU who cal= ls CheckInCpu() earlier. > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object. > + @param[in] CpuIndex Check out CPU index. > + > + @retval RETURN_SUCCESS Check out CPU (CpuIndex) successfull= y. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL. > + @retval RETURN_NOT_READY The CPU is not checked-in. > + @retval RETURN_UNSUPPORTED Unsupported operation. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncCheckOutCpu ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx, > + IN UINTN CpuIndex > + ); > + (6) Looks good, my only question is again if we need a status code for CpuIndex being out of range. > +/** > + Performs an atomic operation lock door for CPU checkin or checkout. > + > + After this function, CPU can not check in via SmmCpuSyncCheckInCpu(). > + > + The CPU specified by CpuIndex is elected to lock door. > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object. > + @param[in] CpuIndex Indicate which CPU to lock door. > + @param[in,out] CpuCount Number of arrived CPU in SMI after l= ook door. > + > + @retval RETURN_SUCCESS Lock door for CPU successfully. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx or CpuCount is NULL. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncLockDoor ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx, > + IN UINTN CpuIndex, > + IN OUT UINTN *CpuCount > + ); This is where it's getting tricky :) (7) error condition for CpuIndex being out of range? (8) why is CpuCount IN OUT and not just OUT? (Other than that, I can see how outputting CpuCout at once can be useful.) (9) do we need error conditions for: (9.1) CpuIndex being "wrong" (i.e., not the CPU that's "supposed" to lock the door)? (9.2) CpuIndex not having checked in already, before trying to lock the door? Now, I can imagine that problem (9.1) is undetectable, i.e., it causes undefined behavior. That's fine, but then we should mention that. > + > +/** > + Used by the BSP to wait for APs. > + > + The number of APs need to be waited is specified by NumberOfAPs. The B= SP is specified by BspIndex. > + > + Note: This function is blocking mode, and it will return only after th= e number of APs released by > + calling SmmCpuSyncReleaseBsp(): > + BSP: WaitForAPs <-- AP: ReleaseBsp > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object. > + @param[in] NumberOfAPs Number of APs need to be waited by B= SP. > + @param[in] BspIndex The BSP Index to wait for APs. > + > + @retval RETURN_SUCCESS BSP to wait for APs successfully. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or NumberOfAPs= > total number of processors in system. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncWaitForAPs ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx, > + IN UINTN NumberOfAPs, > + IN UINTN BspIndex > + ); The "NumberOfAPs > total number of processors in system" check is nice! (10) Again, do we need a similar error condition for BspIndex being out of range? (11) Do we need to document / enforce explicitly (status code) that the BSP and the APs must have checked in, and/or the door must have been locked? Again -- if we can't detect / enforce these conditions, that's fine, but then we should mention the expected call environment. The file-top description does not seem very explicit about it. > + > +/** > + Used by the BSP to release one AP. > + > + The AP is specified by CpuIndex. The BSP is specified by BspIndex. > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object. > + @param[in] CpuIndex Indicate which AP need to be release= d. > + @param[in] BspIndex The BSP Index to release AP. > + > + @retval RETURN_SUCCESS BSP to release one AP successfully. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is= same as BspIndex. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncReleaseOneAp ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx, > + IN UINTN CpuIndex, > + IN UINTN BspIndex > + ); (12) Same comments as elsewhere: - it's good that we check CpuIndex versus BspIndex, but do we also need to range-check each? - document that both affected CPUs need to have checked in, with the door potentially locked? > + > +/** > + Used by the AP to wait BSP. > + > + The AP is specified by CpuIndex. The BSP is specified by BspIndex. > + > + Note: This function is blocking mode, and it will return only after th= e AP released by > + calling SmmCpuSyncReleaseOneAp(): > + BSP: ReleaseOneAp --> AP: WaitForBsp > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context o= bject. > + @param[in] CpuIndex Indicate which AP wait BSP. > + @param[in] BspIndex The BSP Index to be waited. > + > + @retval RETURN_SUCCESS AP to wait BSP successfully. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is= same as BspIndex. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncWaitForBsp ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx, > + IN UINTN CpuIndex, > + IN UINTN BspIndex > + ); > + (13) Same questions as under (12). > +/** > + Used by the AP to release BSP. > + > + The AP is specified by CpuIndex. The BSP is specified by BspIndex. > + > + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context = object. > + @param[in] CpuIndex Indicate which AP release BSP. > + @param[in] BspIndex The BSP Index to be released. > + > + @retval RETURN_SUCCESS AP to release BSP successfully. > + @retval RETURN_INVALID_PARAMETER SmmCpuSyncCtx is NULL or CpuIndex is= same as BspIndex. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SmmCpuSyncReleaseBsp ( > + IN OUT SMM_CPU_SYNC_CONTEXT *SmmCpuSyncCtx, > + IN UINTN CpuIndex, > + IN UINTN BspIndex > + ); (14) Same questions as under (12). > + > +#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 These interfaces look real nice, my comments/questions are all docs-related= . 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 (#112455): https://edk2.groups.io/g/devel/message/112455 Mute This Topic: https://groups.io/mt/103010164/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-