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

["Laszlo Ersek" <lersek@redhat.com>]

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