On 02/04/21 15:27, Ni, Ray wrote: >>> >>> (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.
Yes, absolutely. > > 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. Right, but that's not what I was trying to suggest. Instead, I'm suggesting a very simple macro like FIELD_OFS (BufferStart) that expands to MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.BufferStart That's all, really. > >> >> (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? Yes, this is totally what I meant -- I didn't even intend to ask for CTYPE_UINT32 and friends, given that they directly translate to "resd 1" and similar. The only variable size type is UINTN, so I only asked for CTYPE_UINTN. But if you can add all the CTYPE_* macros, that's best! >> (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 More or less, yes. If it's possible to embed IA32_DESCRIPTOR into MP_CPU_EXCHANGE_INFO somehow, so that we could even refer to the individual fields in it (if necessary), that would be even nicer. But it's not really a requirement -- the above should work OK too. >> (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. > Thank you! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#71253): https://edk2.groups.io/g/devel/message/71253 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] -=-=-=-=-=-=-=-=-=-=-=-
