Re: [edk2-devel] [PATCH v1 3/7] UefiCpuPkg: Adds SmmCpuSyncLib library class

["Laszlo Ersek" <lersek@redhat.com>]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-