Re: [PATCH 0/2] UefiCpuPkg: Add RSB stuffing before rsm instruction

[Laszlo Ersek <lersek@redhat.com>]

On 08/16/18 05:07, Wu, Hao A wrote:
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
>> Ersek
>> Sent: Friday, August 10, 2018 11:06 PM

>> (2) If I understand correctly, the same pattern is used everywhere -- a
>> loop body is executed 32 times, and in the loop body, we jump (via
>> subroutine calls) twice, and each call is followed by a "trap" for
>> speculative execution. At the end of the loop, we forcefully unwind the
>> stack, and then we proceed to RSM.
>>
>> I think this should be implemented with a lot less code duplication.
>> NASM supports macros with labels that are local to macro *invocation*
>> (not macro *definition*); please see the %%skip example here:
>>
>>   https://www.nasm.us/doc/nasmdoc4.html
>>   4.3.2 Macro-Local Labels
>>
>> In addition, it should be possible to pass parameters to macros, such as:
>> - the register to use as counter (eax vs. rax),
>> - the stack pointer to restore (esp vs. rsp),
>> - the size of a stack frame (4 vs. 8)
>>
>> Using all those tools, it should be possible to define the macro only
>> once, in a UefiCpuPkg-level ".inc" file (for example,
>> "UefiCpuPkg/Include/StuffRsb.inc"), and then only invoke the macro near
>> all 10 RSM instructions:
> 
> Yes. Extracting the common logic to a INC file is a good idea.
> 
> However, I found that when compiling .NASM files, the current build rule
> does not support including files other than the .NASM file directory.
> So including a package-level INC file is not supported at this moment.
> 
> I have filed a Bugzilla for adding $(INC)-like support when compiling
> .NASM files:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1085
> 
> After some discussion with the BaseTools owners, some investigation is
> needed for the above support. Hence, I plan to perform a 2-stage change
> when extracting the common RSB stuffing logics to INC file:
> 
> 1. Duplicate the INC file and place them together with the NASM files that
> uses the RSB stuffing logics.
> 
> 2. After NASM compiling support the $(INC)-like feature, propose another
> patch to remove those duplicated INC files and create one under
> UefiCpuPkg/Include/.
> 
> Please help to share your thought on this. Thanks in advance.

Sounds good to me. This approach still eliminates as much code
duplication as it is possible at the current level of BaseTools support.
Also we'll have a reminder (a BZ) for completing the unification.

Thanks!
Laszlo