On 11/21/23 03:35, Yuanhao Xie wrote:
> No functional changes in this patch.
>
> Updates the comments of _CPU_MP_DATA to delcared that duplications in
> CpuMpData are present to avoid to be direct accessed and comprehended
> in assembly code. CpuMpData: Intended for use in C code while
> ExchangeInfo are used in assembly code in this module.
(1) The space characters at the front of these last two lines seem
superfluous.
>
> This patch deletes the unnecessary comments in CpuMpData, since
> CpuMpData is no longer responsible for passing information from PEI to
> DXE.
>
> Signed-off-by: Yuanhao Xie <yuanhao....@intel.com>
> Cc: Laszlo Ersek ler...@redhat.com
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 2 ++
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 13 +++++++------
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> index 72af196513..317e627b58 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc
> @@ -67,6 +67,8 @@ endstruc
>
> ;
> ; Equivalent NASM structure of MP_CPU_EXCHANGE_INFO
> +; Assembly routines should refrain from directly interacting with
> +; the internal details of CPU_MP_DATA.
> ;
> struc MP_CPU_EXCHANGE_INFO
> .StackStart: CTYPE_UINTN 1
OK!
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index af296f6ac0..a96a6389c1 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -203,6 +203,8 @@ typedef struct _CPU_MP_DATA CPU_MP_DATA;
> // MP CPU exchange information for AP reset code
> // This structure is required to be packed because fixed field offsets
> // into this structure are used in assembly code in this module
> +// Assembly routines should refrain from directly interacting with
> +// the internal details of CPU_MP_DATA.
> //
> typedef struct {
> UINTN StackStart;
OK!
> @@ -239,17 +241,16 @@ typedef struct {
> #pragma pack()
>
> //
> -// CPU MP Data save in memory
> +// CPU MP Data save in memory, and intended for use in C code.
> +// There are some duplicated fields, such as XD status, between
> +// CpuMpData and ExchangeInfo. These duplications in CpuMpData
> +// are present to avoid to be direct accessed and comprehended
> +// in assembly code.
> //
(2) So, my following comment applies to both the commit message and the
comment here.
I think we should not attempt to explain the *direction* of the
duplications. I think there have been examples for either direction --
at some point, some information was initially only used by the assembly
code, and then needed duplication for the C code's sake; at another
time, duplication in the the opposite direction was necessary (because
stuff used by the C code was needed by the assembly code too).
In particular, the statement "duplicate a field from ExchangeInfo to
CpuMpData so that assembly code need not understand it" seems wrong. For
easing the job of the assembly code, fields would be duplicated in the
*opposite* direction (from CpuMpData to ExchangeInfo).
Note that in patch#1 in this series, we indeed duplicate a field from
ExchangeInfo to CpuMpData -- but there the reason is different: we want
to make the C code's job easier! The "run loop" wait mode does not
involve *rebooting* APs, so there is no assembly code for AP startup,
and no ExchangeInfo either. Therefore, we're making the C code's job
easier, so that it can function with just CpuMpData.
Thus, I would rather avoid any language on the *direction* of
duplication. I'd just say:
MP_CPU_EXCHANGE_INFO is to be consumed by assembly code only, and
CPU_MP_DATA is to be consumed by C code only. If both kinds of code
need to consume a piece of information, then that information must be
duplicated between both structures.
... I'm sorry that I'm splitting hairs for so long on this issue, but I
find this principle behind the duplications *really* non-obvious. So I
would like us to get the documentation right.
(Ray's original explanation was here:
<https://edk2.groups.io/g/devel/message/111336>.)
> struct _CPU_MP_DATA {
> UINT64 CpuInfoInHob;
> UINT32 CpuCount;
> UINT32 BspNumber;
> - //
> - // The above fields data will be passed from PEI to DXE
> - // Please make sure the fields offset same in the different
> - // architecture.
> - //
> SPIN_LOCK MpLock;
> UINTN Buffer;
> UINTN CpuApStackSize;
This is OK.
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111615): https://edk2.groups.io/g/devel/message/111615
Mute This Topic: https://groups.io/mt/102721681/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
