> > > > (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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#71251): https://edk2.groups.io/g/devel/message/71251 Mute This Topic: https://groups.io/mt/80372085/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
