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 10F88740057 for ; Wed, 13 Dec 2023 15:02:36 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=phOsdZTxLk0I6HXgmFdskPT2zjcDbFVV5t/f2V/U/o8=; 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=1702479755; v=1; b=OSSOUFUCbFCAJR1NFVZ4+0V6g3xy8n9b4QSIbbBjGtRnjkB7zWmtpwpcPEeCFkZ6yQ4/CBO9 WB4swBtZ72UwRhE/uo25lXp+PDdkNO7ioEYee5tsFERnjpr+U8KaT7jFDqdnz3JR9z73hp9RXR4 BZkJy2LdVWtzUs2GZRLBoIjQ= X-Received: by 127.0.0.2 with SMTP id RDjZYY7687511xtVXoH4rK9S; Wed, 13 Dec 2023 07:02:35 -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.web10.38306.1702479754803565776 for ; Wed, 13 Dec 2023 07:02:35 -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-690-DZe0xaKUMs2VIjPzvKUYuA-1; Wed, 13 Dec 2023 10:02:31 -0500 X-MC-Unique: DZe0xaKUMs2VIjPzvKUYuA-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 C43A03C0C4B2; Wed, 13 Dec 2023 15:02:19 +0000 (UTC) X-Received: from [10.39.194.93] (unknown [10.39.194.93]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 548603C25; Wed, 13 Dec 2023 15:02:18 +0000 (UTC) Message-ID: <6584922b-197c-b351-c87b-b936e0eacd12@redhat.com> Date: Wed, 13 Dec 2023 16:02:17 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/6] UefiCpuPkg: Adds SmmCpuSyncLib library class To: "Wu, Jiaxin" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Ni, Ray" , "Zeng, Star" , Gerd Hoffmann , "Kumar, Rahul R" References: <20231206100122.8028-1-jiaxin.wu@intel.com> <20231206100122.8028-3-jiaxin.wu@intel.com> <302017ed-f9da-bf2c-3f95-6dd53395c335@redhat.com> From: "Laszlo Ersek" In-Reply-To: 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: H6ddtVGiQyEO5hI3jhL4kZ2Yx7686176AA= 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=OSSOUFUC; 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/13/23 05:23, Wu, Jiaxin wrote: >> >> 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. >> >=20 > agree, I will fix this. >=20 >>> +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.) >> >=20 > For SMM CPU driver, it shall the BSP to call the function since BSP will = gather all APs to exit SMM synchronously, it's the role to control the over= all SMI execution flow. >=20 > For the API itself, I don't restrict which CPU can do that. It depends on= the consumer. So, it's not the mandatory, that's the reason I don't mentio= n that. >=20 > But you are right, here, it has a requirement: the elected CPU calling th= is function need make sure all CPUs are ready to exist SMI. I can clear doc= ument this requirement as below: >=20 > "This function is called by one of CPUs after all CPUs are ready to exist= SMI, which allows CPU to check into the next SMI from this point." >=20 > Besides, one comment from Ray: we can ASSERT the SmmCpuSyncCtx is not NUL= L, don't need return status to handle all such case. if so, the RETURN_STAT= US is not required. >=20 > So, based on all above, I will update the API as below: >=20 > /** > Reset SMM CPU Sync context. SMM CPU Sync context will be reset to the i= nitialized state. >=20 > This function is called by one of CPUs after all CPUs are ready to exis= t SMI, which allows CPU to > check into the next SMI from this point. >=20 > If Context is NULL, then ASSERT(). >=20 > @param[in,out] Context Pointer to the SMM CPU Sync context object = to be reset. >=20 > **/ > VOID > EFIAPI > SmmCpuSyncContextReset ( > IN OUT SMM_CPU_SYNC_CONTEXT *Context > ); Looks good, thanks -- except, there is again the same typo: "ready to exist SMI". Should be "ready to exit". I also agree that asserting (Context !=3D NULL) is valid, as long as we document that passing in a non-NULL context is the caller's responsibility. (And we do that above, so fine.) >=20 >=20 >>> +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. >> >> >=20 > Agree. I will correct all similar case. Besides, I also received the comm= ents from Ray offline: > 1. we can ASSERT the SmmCpuSyncCtx is not NULL, don't need return status = to handle that. > 2. we don't need RETURN_UNSUPPORTED, GetArrivedCpuCount() should be alwa= ys supported. > With above comments, I will update the API as below to return the count d= irectly, this is also aligned with the function name to get arrived CPU Cou= nt: >=20 > /** > Get current number of arrived CPU in SMI. >=20 > 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. >=20 > If Context is NULL, then ASSERT(). >=20 > @param[in] Context Pointer to the SMM CPU Sync context object. >=20 > @retval Current number of arrived CPU in SMI. >=20 > **/ > UINTN > EFIAPI > SmmCpuSyncGetArrivedCpuCount ( > IN SMM_CPU_SYNC_CONTEXT *Context > ); Sure, if you can guarantee at the lib *class* level that this API always succeeds as long as Context is not NULL, then this update looks fine. >=20 > =20 >>> +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? >> >=20 > Good idea. We can handle this check within ASSERT. Then I will update all= similar case by adding below comments in API: >=20 > "If CpuIndex exceeds the range of all CPUs in the system, then ASSERT()." >=20 > For example: > /** > Performs an atomic operation to check in CPU. >=20 > When SMI happens, all processors including BSP enter to SMM mode by cal= ling SmmCpuSyncCheckInCpu(). >=20 > If Context is NULL, then ASSERT(). > If CpuIndex exceeds the range of all CPUs in the system, then ASSERT(). >=20 > @param[in,out] Context Pointer to the SMM CPU Sync context o= bject. > @param[in] CpuIndex Check in CPU index. >=20 > @retval RETURN_SUCCESS Check in CPU (CpuIndex) successfully. > @retval RETURN_ABORTED Check in CPU failed due to SmmCpuSync= LockDoor() has been called by one elected CPU. >=20 > **/ > RETURN_STATUS > EFIAPI > SmmCpuSyncCheckInCpu ( > IN OUT SMM_CPU_SYNC_CONTEXT *Context, > IN UINTN CpuIndex > ); Works for me. My main points are: - if we consider a particular input condition a *programming error* (and we document it as such), then asserting the opposite of that condition is fine - if we consider an input condition a particular data / environment issue, then catching it / reporting it with an explicit status code is fine= . - point is, for *any* problematic input condition, we should decide if we handle it with *either* assert (in which case the caller is responsible for preventing that condition upon input), *or* with a retval (in which case the caller is responsible for handling the circumstance after the call returns). Handling a given input state with *both* approaches at the same time is totally bogus. >=20 >=20 >> (5) Do we have a special CpuIndex value for the BSP? >> >=20 > No, the elected BSP is also the part of CPUs with its own CpuIndex value. >=20 >=20 >>> + >>> +/** >>> + Performs an atomic operation to check out CPU. >>> + >>> + CheckOutCpu() can be called in error handling flow for the CPU who c= alls >> 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) successfu= lly. >>> + @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. >> >=20 > Yes, agree. The comments from Ray is that we don't need handle the RETURN= _INVALID_PARAMETER, just ASSERT for better performance, we can avoid the if= check in release version. Just document the assert. >=20 > To better define the API behavior, I will remove RETURN_UNSUPPORTED, and = replaced with RETURN_ABORTED, which can align with the checkin behavior if = we don't want support the checkout after look door. RETURN_ABORTED clearly = document the behavior instead of RETURN_UNSUPPORTED. =20 >=20 > So, the API would be as below: > /** > Performs an atomic operation to check out CPU. >=20 > This function can be called in error handling flow for the CPU who call= s CheckInCpu() earlier. >=20 > If Context is NULL, then ASSERT(). > If CpuIndex exceeds the range of all CPUs in the system, then ASSERT(). >=20 > @param[in,out] Context Pointer to the SMM CPU Sync context o= bject. > @param[in] CpuIndex Check out CPU index. >=20 > @retval RETURN_SUCCESS Check out CPU (CpuIndex) successfully= . > @retval RETURN_ABORTED Check out CPU failed due to SmmCpuSyn= cLockDoor() has been called by one elected CPU. >=20 > **/ > RETURN_STATUS > EFIAPI > SmmCpuSyncCheckOutCpu ( > IN OUT SMM_CPU_SYNC_CONTEXT *Context, > IN UINTN CpuIndex > ); >=20 >=20 >>> +/** >>> + 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= look >> 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? >=20 > Yes, agree. The same case as above, it will be handled in the ASSERT and = documented in the comments. >=20 >> >> (8) why is CpuCount IN OUT and not just OUT? (Other than that, I can see >> how outputting CpuCout at once can be useful.) >> >=20 > Well, I agree it should only OUT.=20 > CpuCout is to tell the number of arrived CPU in SMI after look door. For = SMM CPU driver, it needs to know the number of arrived CPU in SMI after loo= k door, it's for later rendezvous & sync usage. So, it returns the CpuCount= . >=20 >=20 >> (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. >> >=20 > Actually CpuIndex might not be cared by the lib instance. It puts here ju= st for the future extension that some lib instance might need to know which= CPU lock the door. It's the information provided by the API. > I don't add the error check for those because I don't want focus the impl= ementation to do this check. >=20 > But I agree, we can document this undefined behavior. How about like bel= ow: > "The CPU specified by CpuIndex is elected to lock door. The caller shal= l make sure the CpuIndex is the actual CPU calling this function to avoid t= he undefined behavior." >=20 > With above, I will update the API like below: >=20 > /** > Performs an atomic operation lock door for CPU checkin and checkout. Af= ter this function: > CPU can not check in via SmmCpuSyncCheckInCpu(). > CPU can not check out via SmmCpuSyncCheckOutCpu(). >=20 > The CPU specified by CpuIndex is elected to lock door. The caller shall= make sure the CpuIndex > is the actual CPU calling this function to avoid the undefined behavior= . >=20 > If Context is NULL, then ASSERT(). > If CpuCount is NULL, then ASSERT(). >=20 > @param[in,out] Context Pointer to the SMM CPU Sync context o= bject. > @param[in] CpuIndex Indicate which CPU to lock door. > @param[in,out] CpuCount Number of arrived CPU in SMI after lo= ok door. >=20 > **/ > VOID > EFIAPI > SmmCpuSyncLockDoor ( > IN OUT SMM_CPU_SYNC_CONTEXT *Context, > IN UINTN CpuIndex, > OUT UINTN *CpuCount > ); Works for me! >=20 >=20 >=20 >>> + >>> +/** >>> + Used by the BSP to wait for APs. >>> + >>> + The number of APs need to be waited is specified by NumberOfAPs. The >> BSP is specified by BspIndex. >>> + >>> + Note: This function is blocking mode, and it will return only after = the >> 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 >> BSP. >>> + @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? >> >=20 > Agree, I will handle the case in the same way as above in the ASSERT. If = so, no need return the status. >=20 >=20 >> (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. >> >=20 > Agree, if BspIndex is the actual CPU calling this function, it must be ch= eckin before. So, how about adding the comments as below: > " The caller shall make sure the BspIndex is the actual CPU calling thi= s function to avoid the undefined behavior." >=20 > Based on above, I propose the API to be: >=20 > /** > Used by the BSP to wait for APs. >=20 > The number of APs need to be waited is specified by NumberOfAPs. The BS= P is specified by BspIndex. > The caller shall make sure the BspIndex is the actual CPU calling this = function to avoid the undefined behavior. > The caller shall make sure the NumberOfAPs have checked-in to avoid the= undefined behavior. >=20 > If Context is NULL, then ASSERT(). > If NumberOfAPs > All CPUs in system, then ASSERT(). > If BspIndex exceeds the range of all CPUs in the system, then ASSERT(). >=20 > Note: > This function is blocking mode, and it will return only after the numbe= r of APs released by > calling SmmCpuSyncReleaseBsp(): > BSP: WaitForAPs <-- AP: ReleaseBsp >=20 > @param[in,out] Context Pointer to the SMM CPU Sync context o= bject. > @param[in] NumberOfAPs Number of APs need to be waited by BS= P. > @param[in] BspIndex The BSP Index to wait for APs. >=20 > **/ > VOID > EFIAPI > SmmCpuSyncWaitForAPs ( > IN OUT SMM_CPU_SYNC_CONTEXT *Context, > IN UINTN NumberOfAPs, > IN UINTN BspIndex > ); OK, thanks. >=20 >>> + >>> +/** >>> + 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 relea= sed. >>> + @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? >> >=20 > Agree. >=20 >> - document that both affected CPUs need to have checked in, with the >> door potentially locked? >> >=20 > Yes, for SMM CPU driver, it shall be called after look door. For API itse= lf, it's better not restrict it. the only requirement I can see is need Cpu= Index must be checkin. So, I will refine it as below: > /** > Used by the BSP to release one AP. >=20 > The AP is specified by CpuIndex. The BSP is specified by BspIndex. > The caller shall make sure the BspIndex is the actual CPU calling this = function to avoid the undefined behavior. > The caller shall make sure the CpuIndex has checked-in to avoid the und= efined behavior. >=20 > If Context is NULL, then ASSERT(). > If CpuIndex =3D=3D BspIndex, then ASSERT(). > If BspIndex and CpuIndex exceed the range of all CPUs in the system, th= en ASSERT(). >=20 > @param[in,out] Context Pointer to the SMM CPU Sync context o= bject. > @param[in] CpuIndex Indicate which AP need to be released= . > @param[in] BspIndex The BSP Index to release AP. >=20 > **/ > VOID > EFIAPI > SmmCpuSyncReleaseOneAp ( > IN OUT SMM_CPU_SYNC_CONTEXT *Context, > IN UINTN CpuIndex, > IN UINTN BspIndex > ); OK. (Small update: the comment should say: "If BspIndex *or* CpuIndex exceed the range ...". For the other functions too, below.) >=20 >=20 >=20 >> >>> + >>> +/** >>> + 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 = the AP >> released by >>> + calling SmmCpuSyncReleaseOneAp(): >>> + BSP: ReleaseOneAp --> AP: WaitForBsp >>> + >>> + @param[in,out] SmmCpuSyncCtx Pointer to the SMM CPU Sync context >> object. >>> + @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). >> >=20 > See below proposed API: >=20 > /** > Used by the AP to wait BSP. >=20 > The AP is specified by CpuIndex. > The caller shall make sure the CpuIndex is the actual CPU calling this = function to avoid the undefined behavior. > The BSP is specified by BspIndex. >=20 > If Context is NULL, then ASSERT(). > If CpuIndex =3D=3D BspIndex, then ASSERT(). > If BspIndex and CpuIndex exceed the range of all CPUs in the system, th= en ASSERT(). >=20 > Note: > This function is blocking mode, and it will return only after the AP re= leased by > calling SmmCpuSyncReleaseOneAp(): > BSP: ReleaseOneAp --> AP: WaitForBsp >=20 > @param[in,out] Context Pointer to the SMM CPU Sync context ob= ject. > @param[in] CpuIndex Indicate which AP wait BSP. > @param[in] BspIndex The BSP Index to be waited. >=20 > **/ > VOID > EFIAPI > SmmCpuSyncWaitForBsp ( > IN OUT SMM_CPU_SYNC_CONTEXT *Context, > IN UINTN CpuIndex, > IN UINTN BspIndex > ); OK, thanks. >=20 >=20 >>> +/** >>> + 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). >> >=20 > See below proposed API: >=20 > /** > Used by the AP to release BSP. >=20 > The AP is specified by CpuIndex. > The caller shall make sure the CpuIndex is the actual CPU calling this = function to avoid the undefined behavior. > The BSP is specified by BspIndex. >=20 > If Context is NULL, then ASSERT(). > If CpuIndex =3D=3D BspIndex, then ASSERT(). > If BspIndex and CpuIndex exceed the range of all CPUs in the system, th= en ASSERT(). >=20 > @param[in,out] Context Pointer to the SMM CPU Sync context o= bject. > @param[in] CpuIndex Indicate which AP release BSP. > @param[in] BspIndex The BSP Index to be released. >=20 > **/ > VOID > EFIAPI > SmmCpuSyncReleaseBsp ( > IN OUT SMM_CPU_SYNC_CONTEXT *Context, > IN UINTN CpuIndex, > IN UINTN BspIndex > ); >=20 Thanks! Laszlo >=20 > Thanks, > Jiaxin=20 >=20 >=20 >>> + >>> +#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 >>> >>> ## @libraryclass Provides functions for manipulating smram savesta= te >> registers. >>> MmSaveStateLib|Include/Library/MmSaveStateLib.h >>> >>> + ## @libraryclass Provides functions for SMM CPU Sync Operation. >>> + SmmCpuSyncLib|Include/Library/SmmCpuSyncLib.h >>> + >>> [LibraryClasses.RISCV64] >>> ## @libraryclass Provides functions to manage MMU features on RISC= V64 >> CPUs. >>> ## >>> RiscVMmuLib|Include/Library/BaseRiscVMmuLib.h >>> >> >> These interfaces look real nice, my comments/questions are all docs-rela= ted. >> >> 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 (#112484): https://edk2.groups.io/g/devel/message/112484 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-