Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset

["Ni, Ray" <ray.ni@intel.com>]

> >
> > (1) please align the "res*" on the other lines with this
> >
> > (in the X64 file too)
> >

OK.

> >> +    mov        si,  MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.BufferStart
> >
> > (2) please introduce a macro for this; in my opinion, with the currently
> > proposed change, the code is *harder* to read and modify than before.
> > Now we have a lot of fluff to spell out, for every single field reference.

I want to use the struc instead of original hardcode offset because
I have headache when removing the Lock field from the C structure.
All the hardcode value should be changed carefully.
Using struc, I can simply remove that field Lock from the struc.

I originally tried to supply the second parameter to struc for the initial offset
following https://www.nasm.us/doc/nasmdoc5.html#section-5.9.1
  struc MP_CPU_EXCHANGE_INFO (SwitchToRealProcEnd - RendezvousFunnelProcStart)

So that
  mov        si,  MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.BufferStart
can be:
  mov        si,  MP_CPU_EXCHANGE_INFO.BufferStart
But somehow NASM compiler doesn't like it.

> 
> (3) I have a further request / suggestion:
> 
> (3a) We should extend the following files:
> 
>   MdePkg/Include/Ia32/Nasm.inc
>   MdePkg/Include/X64/Nasm.inc
> 
> with a macro that maps UINTN to "resd" versus "resq", as appropriate,

I am not an expert of NASM or ASM.
Do you mean to use %define as below in Ia32/Nasm.inc?
%define CTYPE_UINTN     resd 1
%define CTYPE_UINT32    resd 1
%define CTYPE_UINT64    resq 1 
%define CTYPE_UINT8       resb 1
%define CTYPE_BOOLEAN resb 1
%define CTYPE_UINT16     resw 1

And define below in X64/Nasm.inc?
%define CTYPE_UINTN     resq 1
%define CTYPE_UINT32    resd 1
%define CTYPE_UINT64    resq 1 
%define CTYPE_UINT8       resb 1
%define CTYPE_BOOLEAN resb 1
%define CTYPE_UINT16     resw 1

So, the struc definition will be as below?
  .StackStart:           CTYPE_UINTN

I intend to use CTYPE_xxx as prefix because simply using UINTN may cause
people think that UINTN is the C keyword UINTN.
Using CTYPE_UINTN so people can at least search this string to understand
the magic.

Anyway, I just want to use a different name other than UINTN.
Do you agree? Any suggestions on the name?

> 
> (3b) we should reserve space for the IA32_DESCRIPTOR fields in terms of
> UINT16 + UINTN -- you can use a separate "struc IA32_DESCRIPTOR" for
> this that uses the UINTN trick from step (3a)

Do you mean this?

struc IA32_DESCRIPTOR
  .Limit CTYPE_UINT16
  .Base  CTYPE_UINTN
endstruc

struc MP_CPU_EXCHANGE_INFO
...
  .IdtrProfile:          resb IA32_DESCRIPTOR_size

> 
> (3c) use a common definition for "struc MP_CPU_EXCHANGE_INFO", hiding
> the UINTN and IA32_DESCRIPTOR size differences through the above steps.

I think it's doable.