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

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

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