Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset
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. > +movsi, 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 > movsi, MP_CPU_EXCHANGE_INFO_OFFSET + > MP_CPU_EXCHANGE_INFO.BufferStart > can be: > movsi, 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_UINT32resd 1 > %define CTYPE_UINT64resq 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_UINT32resd 1 > %define CTYPE_UINT64resq 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] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset
> > > > (1) please align the "res*" on the other lines with this > > > > (in the X64 file too) > > OK. > >> +movsi, 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 movsi, MP_CPU_EXCHANGE_INFO_OFFSET + MP_CPU_EXCHANGE_INFO.BufferStart can be: movsi, 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_UINT32resd 1 %define CTYPE_UINT64resq 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_UINT32resd 1 %define CTYPE_UINT64resq 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] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset
On 02/04/21 10:32, Laszlo Ersek wrote: > On 02/04/21 03:59, Ni, Ray wrote: >> In Windows environment, "dumpbin /disasm" is used to verify the >> disassembly before and after using NASM struc doesn't change. >> >> Signed-off-by: Ray Ni >> Cc: Eric Dong >> Cc: Laszlo Ersek >> Cc: Rahul Kumar >> --- >> UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 52 ++ >> .../Library/MpInitLib/Ia32/MpFuncs.nasm | 35 ++-- >> UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc| 54 ++- >> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 46 >> 4 files changed, 98 insertions(+), 89 deletions(-) >> >> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc >> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc >> index 4f5a7c859a..244c1e72b7 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc >> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc >> @@ -1,5 +1,5 @@ >> >> ;-- >> ; >> -; Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved. >> +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved. >> ; SPDX-License-Identifier: BSD-2-Clause-Patent >> ; >> ; Module Name: >> @@ -19,25 +19,31 @@ CPU_SWITCH_STATE_IDLE equ0 >> CPU_SWITCH_STATE_STORED equ1 >> CPU_SWITCH_STATE_LOADED equ2 >> >> -LockLocation equ(SwitchToRealProcEnd - >> RendezvousFunnelProcStart) >> -StackStartAddressLocation equLockLocation + 04h >> -StackSizeLocation equLockLocation + 08h >> -ApProcedureLocation equLockLocation + 0Ch >> -GdtrLocation equLockLocation + 10h >> -IdtrLocation equLockLocation + 16h >> -BufferStartLocation equLockLocation + 1Ch >> -ModeOffsetLocationequLockLocation + 20h >> -ApIndexLocation equLockLocation + 24h >> -CodeSegmentLocation equLockLocation + 28h >> -DataSegmentLocation equLockLocation + 2Ch >> -EnableExecuteDisableLocation equLockLocation + 30h >> -Cr3Location equLockLocation + 34h >> -InitFlagLocation equLockLocation + 38h >> -CpuInfoLocation equLockLocation + 3Ch >> -NumApsExecutingLocation equLockLocation + 40h >> -InitializeFloatingPointUnitsAddress equ LockLocation + 48h >> -ModeTransitionMemoryLocationequ LockLocation + 4Ch >> -ModeTransitionSegmentLocation equ LockLocation + 50h >> -ModeHighMemoryLocation equ LockLocation + 52h >> -ModeHighSegmentLocation equ LockLocation + 56h >> - >> +MP_CPU_EXCHANGE_INFO_OFFSET equ (SwitchToRealProcEnd - >> RendezvousFunnelProcStart) >> +struc MP_CPU_EXCHANGE_INFO >> + .Lock: resd 1 >> + .StackStart: resd 1 >> + .StackSize:resd 1 >> + .CFunction:resd 1 >> + .GdtrProfile: resb 6 >> + .IdtrProfile: resb 6 >> + .BufferStart: resd 1 >> + .ModeOffset: resd 1 >> + .ApIndex: resd 1 >> + .CodeSegment: resd 1 >> + .DataSegment: resd 1 >> + .EnableExecuteDisable: resd 1 >> + .Cr3: resd 1 >> + .InitFlag: resd 1 >> + .CpuInfo: resd 1 >> + .NumApsExecuting: resd 1 >> + .CpuMpData:resd 1 >> + .InitializeFloatingPointUnits: resd 1 > > (1) please align the "res*" on the other lines with this > > (in the X64 file too) > >> + .ModeTransitionMemory: resd 1 >> + .ModeTransitionSegment:resw 1 >> + .ModeHighMemory: resd 1 >> + .ModeHighSegment: resw 1 >> + .Enable5LevelPaging: resb 1 >> + .SevEsIsEnabled: resb 1 >> + .GhcbBase: resd 1 >> +endstruc >> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm >> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm >> index 7e81d24aa6..908c2eb447 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm >> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm >> @@ -1,5 +1,5 @@ >> >> ;-- >> ; >> -; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved. >> +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved. >> ; SPDX-License-Identifier: BSD-2-Clause-Patent >> ; >> ; Module Name: >> @@ -39,21 +39,21 @@ BITS 16 >> movfs, ax >> movgs, ax >> >> -movsi, BufferStartLocation >> +movsi, 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. (3) I have a further
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Use NASM struc to avoid hardcode offset
On 02/04/21 03:59, Ni, Ray wrote: > In Windows environment, "dumpbin /disasm" is used to verify the > disassembly before and after using NASM struc doesn't change. > > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Rahul Kumar > --- > UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 52 ++ > .../Library/MpInitLib/Ia32/MpFuncs.nasm | 35 ++-- > UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc| 54 ++- > UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 46 > 4 files changed, 98 insertions(+), 89 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc > b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc > index 4f5a7c859a..244c1e72b7 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc > @@ -1,5 +1,5 @@ > > ;-- > ; > -; Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved. > +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved. > ; SPDX-License-Identifier: BSD-2-Clause-Patent > ; > ; Module Name: > @@ -19,25 +19,31 @@ CPU_SWITCH_STATE_IDLE equ0 > CPU_SWITCH_STATE_STORED equ1 > CPU_SWITCH_STATE_LOADED equ2 > > -LockLocation equ(SwitchToRealProcEnd - > RendezvousFunnelProcStart) > -StackStartAddressLocation equLockLocation + 04h > -StackSizeLocation equLockLocation + 08h > -ApProcedureLocation equLockLocation + 0Ch > -GdtrLocation equLockLocation + 10h > -IdtrLocation equLockLocation + 16h > -BufferStartLocation equLockLocation + 1Ch > -ModeOffsetLocationequLockLocation + 20h > -ApIndexLocation equLockLocation + 24h > -CodeSegmentLocation equLockLocation + 28h > -DataSegmentLocation equLockLocation + 2Ch > -EnableExecuteDisableLocation equLockLocation + 30h > -Cr3Location equLockLocation + 34h > -InitFlagLocation equLockLocation + 38h > -CpuInfoLocation equLockLocation + 3Ch > -NumApsExecutingLocation equLockLocation + 40h > -InitializeFloatingPointUnitsAddress equ LockLocation + 48h > -ModeTransitionMemoryLocationequ LockLocation + 4Ch > -ModeTransitionSegmentLocation equ LockLocation + 50h > -ModeHighMemoryLocation equ LockLocation + 52h > -ModeHighSegmentLocation equ LockLocation + 56h > - > +MP_CPU_EXCHANGE_INFO_OFFSET equ (SwitchToRealProcEnd - > RendezvousFunnelProcStart) > +struc MP_CPU_EXCHANGE_INFO > + .Lock: resd 1 > + .StackStart: resd 1 > + .StackSize:resd 1 > + .CFunction:resd 1 > + .GdtrProfile: resb 6 > + .IdtrProfile: resb 6 > + .BufferStart: resd 1 > + .ModeOffset: resd 1 > + .ApIndex: resd 1 > + .CodeSegment: resd 1 > + .DataSegment: resd 1 > + .EnableExecuteDisable: resd 1 > + .Cr3: resd 1 > + .InitFlag: resd 1 > + .CpuInfo: resd 1 > + .NumApsExecuting: resd 1 > + .CpuMpData:resd 1 > + .InitializeFloatingPointUnits: resd 1 (1) please align the "res*" on the other lines with this (in the X64 file too) > + .ModeTransitionMemory: resd 1 > + .ModeTransitionSegment:resw 1 > + .ModeHighMemory: resd 1 > + .ModeHighSegment: resw 1 > + .Enable5LevelPaging: resb 1 > + .SevEsIsEnabled: resb 1 > + .GhcbBase: resd 1 > +endstruc > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index 7e81d24aa6..908c2eb447 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -1,5 +1,5 @@ > > ;-- > ; > -; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved. > +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved. > ; SPDX-License-Identifier: BSD-2-Clause-Patent > ; > ; Module Name: > @@ -39,21 +39,21 @@ BITS 16 > movfs, ax > movgs, ax > > -movsi, BufferStartLocation > +movsi, 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. Thanks Laszlo > movebx, [si] > > -movsi, DataSegmentLocation > +movsi, MP_CPU_EXCHANGE_INFO_OFFSET + > MP_CPU_EXCHANGE_INFO.DataSegment >
