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.
This patch is to:
1.Adds SmmCpuSyncLib Library class in UefiCpuPkg.dec.
2.Adds SmmCpuSyncLib.h function declaration header file.
Change-Id: Ib7f5e317526e8b9e29b65e072bdb485dbd678817
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 191 +++++++++++++++++++++++++++++
UefiCpuPkg/UefiCpuPkg.dec | 3 +
2 files changed, 194 insertions(+)
create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
diff --git a/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h b/UefiCpuPkg/Include/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.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef SMM_CPU_SYNC_LIB_H_
+#define SMM_CPU_SYNC_LIB_H_
+
+#include <Uefi/UefiBaseType.h>
+
+/**
+ Creates and Init a new Smm Cpu Sync context.
+
+ @param[in] NumberOfCpus The number of processors in the system.
+
+ @return Pointer to an allocated Smm Cpu Sync context object.
+ If the creation failed, returns NULL.
+
+**/
+VOID *
+EFIAPI
+SmmCpuSyncContextInit (
+ IN UINTN NumberOfCpus
+ );
+
+/**
+ Deinit an allocated Smm Cpu Sync context object.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object 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 object to be released.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncContextReset (
+ IN VOID *SmmCpuSyncCtx
+ );
+
+/**
+ Get current arrived CPU count.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+
+ @return Current number of arrived CPU count.
+ -1: indicate the door has been locked.
+
+**/
+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.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm CPU Sync context object to be released.
+ @param[in] CpuIndex Pointer to the CPU Index to checkin.
+
+ @return Positive value (>0): CPU arrival count number after check in CPU successfully.
+ Nonpositive value (<=0): check in CPU failure.
+
+**/
+INT32
+EFIAPI
+SmmCpuSyncCheckInCpu (
+ IN VOID *SmmCpuSyncCtx,
+ 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 object to be released.
+ @param[in] CpuIndex Pointer to the Cpu Index to checkout.
+
+ @return Nonnegative value (>=0): 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
+ );
+
+/**
+ Performs an atomic operation lock door for CPU checkin or checkout.
+ With this function, CPU can not check in via SmmCpuSyncCheckInCpu () or
+ check out via SmmCpuSyncCheckOutCpu ().
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object to be released.
+ @param[in] CpuIndex Pointer to the Cpu Index to lock door.
+
+ @return CPU arrival count number.
+
+**/
+UINT32
+EFIAPI
+SmmCpuSyncLockDoor (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex
+ );
+
+/**
+ Used for BSP to wait all APs.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] NumberOfAPs Number of APs need to wait.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncWaitForAllAPs (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN NumberOfAPs,
+ IN UINTN BspIndex
+ );
+
+/**
+ Used for BSP to release one AP.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @param[in] CpuIndex Pointer to the Cpu Index, indicate which AP need to be released.
+ @param[in] BspIndex Pointer to the BSP Index.
+
+**/
+VOID
+EFIAPI
+SmmCpuSyncReleaseOneAp (
+ IN VOID *SmmCpuSyncCtx,
+ IN UINTN CpuIndex,
+ IN UINTN BspIndex
+ );
+
+/**
+ Used for AP to wait BSP.
+
+ @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object.
+ @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 object.
+ @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
## @libraryclass Provides functions for manipulating smram savestate 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 RISCV64 CPUs.
##
RiscVMmuLib|Include/Library/BaseRiscVMmuLib.h
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110639): https://edk2.groups.io/g/devel/message/110639
Mute This Topic: https://groups.io/mt/102366300/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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.
>
> This patch is to:
> 1.Adds SmmCpuSyncLib Library class in UefiCpuPkg.dec.
> 2.Adds SmmCpuSyncLib.h function declaration header file.
>
> Change-Id: Ib7f5e317526e8b9e29b65e072bdb485dbd678817
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
> UefiCpuPkg/Include/Library/SmmCpuSyncLib.h | 191 +++++++++++++++++++++++++++++
> UefiCpuPkg/UefiCpuPkg.dec | 3 +
> 2 files changed, 194 insertions(+)
> create mode 100644 UefiCpuPkg/Include/Library/SmmCpuSyncLib.h
>
> diff --git a/UefiCpuPkg/Include/Library/SmmCpuSyncLib.h b/UefiCpuPkg/Include/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.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef SMM_CPU_SYNC_LIB_H_
> +#define SMM_CPU_SYNC_LIB_H_
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +/**
> + Creates and Init a new Smm Cpu Sync context.
> +
> + @param[in] NumberOfCpus The number of processors in the system.
> +
> + @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 object 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 object 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 object 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 object 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 check in CPU successfully.
> + Nonpositive value (<=0): 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 object to be released.
> + @param[in] CpuIndex Pointer to the Cpu Index to checkout.
> +
> + @return Nonnegative value (>=0): 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 () or
> + check out via SmmCpuSyncCheckOutCpu ().
> +
> + @param[in] SmmCpuSyncCtx Pointer to the Smm Cpu Sync context object 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 object.
> + @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 object.
> + @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 object.
> + @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 object.
> + @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
>
> ## @libraryclass Provides functions for manipulating smram savestate 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 RISCV64 CPUs.
> ##
> RiscVMmuLib|Include/Library/BaseRiscVMmuLib.h
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
I provided 4 comments in below, starting with "[Ray".
Sorry for the poor new Outlook that I am not able to put ">" in the original email.
Thanks,
Ray
________________________________
(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 {
...
};
[Ray.1] Good suggestion. I still remember you taught me this technique when I
wrote the FrameBufferBltLib.
(3) By definition, in a function that resets the internals of an object,
you cannot specify "IN" for that function. It must be OUT.
[Ray.2] I have a different opinion about IN/OUT. I think we should use "IN OUT".
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?
[Ray.3] Good suggestions.
The lib provides 3 sets of APIs:
1. Init/Deinit
Init() is called in driver's entrypoint for allocating the storage and initialize the content of sync object.
Deinit() is called in driver's unload function for undoing what has been done in Init().
2. CheckInCpu/CheckOutCpu/LockDoor/GetArrivedCpuCount
When SMI happens, all processors including BSP enter to SMM mode by calling CheckInCpu().
The elected BSP calls LockDoor() so that CheckInCpu() after that returns failure code.
CheckOutCpu() is called in error handling flow for the CPU who calls CheckInCpu() earlier.
GetArrivedCpuCount() returns the number of checked-in CPUs.
3. WaitForAllAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
First 2 APIs are called from BSP.
Latter 2 APIs are called from APs.
The 4 APIs are used to synchronize the running flow among BSP and APs.
> +
> + @return CPU arrival count number.
(14) why is it necessary for outputting the arrived number when locking?
[Ray.4] As I explained above, when BSP locks the door, it might need to know how many CPUs are in the SMM "room".
Maybe today the number is not cared by BSP.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111118): https://edk2.groups.io/g/devel/message/111118
Mute This Topic: https://groups.io/mt/102366300/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 11/13/23 04:15, Ni, Ray wrote:
> I provided 4 comments in below, starting with "[Ray".
>
> Sorry for the poor new Outlook that I am not able to put ">" in the
> original email.
>
> Thanks,
> Ray
>
> ------------------------------------------------------------------------
>
> (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 {
> ...
> };
>
> [Ray.1] Good suggestion. I still remember you taught me this technique
> when I
> wrote the FrameBufferBltLib.
>
> (3) By definition, in a function that resets the internals of an object,
> you cannot specify "IN" for that function. It must be OUT.
>
> [Ray.2] I have a different opinion about IN/OUT. I think we should use
> "IN OUT".
OK! I can see that "reset" may rely on previous information in the
structure. Furthermore, "IN OUT" may indeed better reflect the
requirement that the object being reset must have been initialized
previously (i.e. that it must be a valid object at the time of the
"reset" call).
>
>
> 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?
>
> [Ray.3] Good suggestions.
> The lib provides 3 sets of APIs:
> 1. Init/Deinit
> Init() is called in driver's entrypoint for allocating the storage and
> initialize the content of sync object.
> Deinit() is called in driver's unload function for undoing what has been
> done in Init().
>
> 2. CheckInCpu/CheckOutCpu/LockDoor/GetArrivedCpuCount
> When SMI happens, all processors including BSP enter to SMM mode by
> calling CheckInCpu().
> The elected BSP calls LockDoor() so that CheckInCpu() after that returns
> failure code.
> CheckOutCpu() is called in error handling flow for the CPU who calls
> CheckInCpu() earlier.
> GetArrivedCpuCount() returns the number of checked-in CPUs.
>
> 3. WaitForAllAPs/ReleaseOneAp/WaitForBsp/ReleaseBsp
> First 2 APIs are called from BSP.
> Latter 2 APIs are called from APs.
> The 4 APIs are used to synchronize the running flow among BSP and APs.
This sounds really nice in fact; I'd be glad to see it captured in the
comments!
>
>> +
>> + @return CPU arrival count number.
>
> (14) why is it necessary for outputting the arrived number when locking?
> [Ray.4] As I explained above, when BSP locks the door, it might need to
> know how many CPUs are in the SMM "room".
> Maybe today the number is not cared by BSP.
>
This helps, thanks. Please do capture it in the comments.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111134): https://edk2.groups.io/g/devel/message/111134
Mute This Topic: https://groups.io/mt/102366300/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 11/7/23 11:26, Laszlo Ersek wrote: > Furthermore, counts of objects should be generally represented as UINTN > values, unless you have a good (explained!) reason for using a different > type. If the argument is that we need atomics for manipulating CPU counts, and that we don't want to assume wider-than-32-bit atomics, then that is a *valid* argument, but it absolutely must be documented (and then *all* CPU counts in the library class header must be UINT32 *consistently*; not mixing UINT32 with UINTN with INT32). Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110837): https://edk2.groups.io/g/devel/message/110837 Mute This Topic: https://groups.io/mt/102366300/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.