Re: [PATCH] kbuild, x86: revert macros in extended asm workarounds

[Nadav Amit]

> On Dec 16, 2018, at 2:00 AM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> 
> On Sun, Dec 16, 2018 at 02:33:39AM +0000, Nadav Amit wrote:
>> In general, I think that from the start it was clear that the motivation for
>> the patch-set is not just performance and also better code. For example, I
>> see no reason to revert the PV-changes or the lock-prefix changes that
>> improved the code readability.
> 
> One thing that has caught my eye with the asm macros, which actually
> decreases readability, is that I can't see the macro properly expanded
> when I do
> 
> make <filename>.s
> 
> For example, I get
> 
> #APP
> # 164 "./arch/x86/include/asm/cpufeature.h" 1
>        STATIC_CPU_HAS bitnum=$8 cap_byte="boot_cpu_data+35(%rip)" feature=123 t_yes=.L75 t_no=.L78 always=117  #, MEM[(const char *)&boot_cpu_data + 35B],,,,
> # 0 "" 2
>        .loc 11 164 2 view .LVU480
> #NO_APP
> 
> but I'd like to see the actual asm as it is really helpful when hacking
> on inline asm stuff. And I haven't found a way to make gcc expand asm
> macros in .s output.

Youâre right, although there were already 72 assembly macros for defined in
x86 .h files, and some may find the unexpanded macro in the â.sâ file more
friendly (well, a small comment in inline assembly could have resolved this
issue).

Anyhow, using gnu asm listings should be a relatively reasonable workaround
for this limitation (unless you want to hack the code before assembly).

For example, using ` as -alm arch/x86/kernel/macros.s arch/x86/kvm/vmx.s `

would give you:

 421                    # ./arch/x86/include/asm/cpufeature.h:164:      asm_volatile_goto("STATIC_CPU_HAS bitnum=%[bitnum] "
 422                            .file 8 "./arch/x86/include/asm/cpufeature.h"
 423                            .loc 8 164 0
 424                    #APP
 425                    # 164 "./arch/x86/include/asm/cpufeature.h" 1
 426                            STATIC_CPU_HAS bitnum=$2 cap_byte="boot_cpu_data+38(%rip)" feature=145 t_yes=.L17 t_no=.L18 always
 426                    > 1:
 426 00d8 E9000000      >  jmp 6f
 426      00
 426                    > 2:
 426                    >  .skip -(((5f-4f)- (2b-1b))>0)* ((5f-4f)- (2b-1b)),0x90
 426                    > 3:
 426                    >  .section .altinstructions,"a"
 426 000d 00000000      >  .long 1b - .
 426 0011 00000000      >  .long 4f - .
 426 0015 7500          >  .word 117

â

This can be incorporated into a makefile option, I suppose.


> Now, assuming the gcc inline patch will be backported to gcc8, I think
> we should be covered on all modern distros going forward. So I think we
> should revert at least the more complex macros.

I understand, and perhaps STATIC_CPU_HAS is not a good use-case (once
inlining is resolved in a different manner). I think that the main question
should be whether the whole infrastructure should be removed or to be
selective. 

In the case of exception tables, for instance, the result is much cleaner,
as it allows to consolidate the C and assembly implementations. There is an
alternative solution of turning the assembly macros into C macros, which
would make the Make system hacks go away, but would make the code not as
nice.