[MODERATED] Re: [patch V7 03/15] SBB 3

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

..snip..
> The alternative asm is
> 
> 	"movl %[msr], %%ecx\n\t"       \
> 	"movl %[val], %%eax\n\t"       \
> 	"movl $0, %%edx\n\t"           \
> 	"wrmsr",                       \

My compiler (ancient gcc (GCC) 4.4.4 20100503 (Red Hat 4.4.4-2)) seems to have created:

ffffffff8249adf4:       b9 48 00 00 00          mov    $0x48,%ecx
ffffffff8249adf9:       8b 45 c8                mov    -0x38(%rbp),%eax
ffffffff8249adfc:       ba 00 00 00 00          mov    $0x0,%edx
ffffffff8249ae01:       0f 30                   wrmsr   
ffffffff8249ae03:       b9 48 00 00 00          mov    $0x48,%ecx
ffffffff8249ae08:       8b 45 c8                mov    -0x38(%rbp),%eax
ffffffff8249ae0b:       ba 00 00 00 00          mov    $0x0,%edx
ffffffff8249ae10:       0f 30                   wrmsr   
..

which obviously means there is a preamble to stick the value in
the stack, which means an call, which means exactly what you mean.

I originally did the simple change to minimize the amount of backporting churn.

That is when stable@vger.kernel.org trees emails would start flooding
in I could easily and correctly provide the right incantations.

which reminds me, should all those patches have 'xCc: stable@vger.kernel.org'
or such on them? (the x so git format-patch won't include the email in the CC).

> 
> and those new constraints are quite dubious:
> 
> 	: : [msr] "i" (_msr), [val] "m" (_val)     \
> 	: "eax", "ecx", "edx", "memory")
> 
> and that *forces* gcc to spill "val" to memory (regardless of 
> alternative), only for the inline asm to then load it from memory.
> 
> It would make more sense to say "g" than "m", I think. We don't much care 
> where "val" comes from.

I tried that:

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 7a1be0b..0e66b75 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -266,7 +266,7 @@ static inline void vmexit_fill_RSB(void)
                                 "movl $0, %%edx\n\t"           \
                                 "wrmsr",                       \
                                 _feature)                      \
-                    : : [msr] "i" (_msr), [val] "m" (_val)     \
+                    : : [msr] "i" (_msr), [val] "g" (_val)     \
                     : "eax", "ecx", "edx", "memory")
 
 static inline void indirect_branch_prediction_barrier(void)

and got:

/home/konrad/ssd/konrad/linux/arch/x86/platform/efi/efi_64.c: Assembler messages:
/home/konrad/ssd/konrad/linux/arch/x86/platform/efi/efi_64.c:869: Error: suffix or operands invalid for `mov'
/home/konrad/ssd/konrad/linux/arch/x86/platform/efi/efi_64.c:869: Error: suffix or operands invalid for `mov'

Sorry for not digging in this further, my brain is tired tonight.
> 
> But more importantly, why do we have those odd "mov" instructions at all?

It probably was there to make the GCC gas shut up when this was using 'i' ?

> 
> Why does this code not just do
> 
>     { unsigned int ax, dx, cx;
>       asm volatile(ALTERNATIVE("","wrmsr",_feature)
> 	:"=a" (ax), "=c" (cx), "=d" (dx)
> 	:"0" (_val), "1" (_msr)
> 	:"memory");
>     }
> 
> which seems simpler and better for gcc to set up? Sure, it makes the 
> setup be unconditional, but it effectively already was (because of that 
> memory setup).
> 
> But I guess it doesn't much _matter_.

Right as we only do those on BIOS/EFI calls. Perhaps an follow up clean up patch?

> 
>             Linus