[edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset
Ni, Ray
ray.ni at intel.com
Thu Feb 4 14:27:51 UTC 2021
> >
> > (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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list