llvm dev - Feb 2018 - retpoline mitigation and 6.0

On Wed, Feb 07, 2018 at 08:44:32PM +0000, David Woodhouse
wrote:
> On Wed, 2018-02-07 at 10:11 -0800, Guenter Roeck wrote:
> 
> > On Wed, Feb 07, 2018 at 10:49:25AM +0000, David Woodhouse wrote:
> > > Hm, please could we also have the %V asm constraint modifier?
That
> > > allows us to emit calls to the thunks from inline asm using the
> > > register that the compiler chose for us:
> > > 
> > >  asm volatile ("call
__x86_indirect_thunk_%V[thunk_target]" : :
> > > 		[thunk_target] "r" (the_function)); 
> > > 
> > > Other than that, I get the following errors with LLVM+Clang
master, and
> > > my tree at
> > >
http://git.infradead.org/users/dwmw2/linux-retpoline.git/shortlog/refs/heads/ibpb
> > > 
> > I tried ToT clang with Linux upstream as well as chromeos-4.14,
> > with 'defconfig'. I don't see any errors when building
x86_64.
> > Lots and lots of warnings, though.
> 
> The defconfig doesn't build here either; it still includes the cpuidle
> bits which don't build. I'll update LLVM/clang and try again, in
case
> it was fixed between your attempt and mine...
> 
Here are my exact versions:
    llvm:	3afd566557f3 ("AMDGPU: Add 32-bit constant address space")
    clang:	848874aed95a ("[clang-format] Fix ObjC message arguments
formatting.")

I used cmake-3.10.2, and the following commands:
    mkdir build
    cd build
    ../cmake-3.10.2/bin/cmake -DCMAKE_BUILD_TYPE=Release -G "Unix
Makefiles" ../llvm
    make -j80

To build the kernel:

PATH=${HOME}/src/clang/build/bin:${PATH}
make CC=clang defconfig
make CC=clang -j80

The build is a bit noisy right now due to 66f793099a63 ("x86/retpoline:
Avoid
retpolines for built-in __init functions"), since clang doesn't
understand
"__attribute__((indirect_branch("keep")))".
> 
> > I can disable the i915 driver, but the "invalid output size for
> > constraint '=q'" happens all over the place. Ultimately
this means
> > that I can not really test a 32-bit build, though it would not build
> > anyway because it requires the following symbols
> > 
> > 	U __x86_indirect_thunk_esp
> > 	U __x86_indirect_thunk
> 
> The latter I can live with, as discussed, for 32-bit only. We don't
> care about CET compatibility there, so I'm OK to implement the bare
> ret-equivalent __x86_indirect_thunk.
> 
> The former... wtf? Can you show me the code that actually *calls* that.
> I am having difficulty imagining any situation in which that's sane.
Turns out this was PBKAC. That was with chromeos-4.14, which does not yet
include the upstream patch removing __x86_indirect_thunk_esp. I don't
see it with ToT. The "U" was from ./arch/x86/lib/lib-ksyms.o, meaning
it
was internally generated. Sorry for the noise.

Guenter

On Wed, 2018-02-07 at 13:16 -0800, Guenter Roeck wrote:
> Here are my exact versions:
>     llvm:	3afd566557f3 ("AMDGPU: Add 32-bit constant address
space")
>     clang:	848874aed95a ("[clang-format] Fix ObjC message arguments
formatting.")
OK, mine are slightly newer than that now, but I now get a working 64-
bit defconfig build. It'll still break with any PV guest support
enabled because we need %V asm constraint modifiers.
> PATH=${HOME}/src/clang/build/bin:${PATH}
> make CC=clang defconfig
> make CC=clang -j80
> 
> The build is a bit noisy right now due to 66f793099a63
("x86/retpoline: Avoid
> retpolines for built-in __init functions"), since clang doesn't
understand
> "__attribute__((indirect_branch("keep")))".
I fixed that (well, worked around it) in my tree.
> > > I can disable the i915 driver, but the "invalid output size
for
> > > constraint '=q'" happens all over the place.
Ultimately this means
> > > that I can not really test a 32-bit build, though it would not
build
> > > anyway because it requires the following symbols
> > > 
> > > 	U __x86_indirect_thunk_esp
> > > 	U __x86_indirect_thunk
> >
> > The latter I can live with, as discussed, for 32-bit only. We
don't
> > care about CET compatibility there, so I'm OK to implement the
bare
> > ret-equivalent __x86_indirect_thunk.
> > 
> > The former... wtf? Can you show me the code that actually *calls*
that.
> > I am having difficulty imagining any situation in which that's
sane.
>
> Turns out this was PBKAC. That was with chromeos-4.14, which does not yet
> include the upstream patch removing __x86_indirect_thunk_esp. I don't
> see it with ToT. The "U" was from ./arch/x86/lib/lib-ksyms.o,
meaning it
> was internally generated. Sorry for the noise.
OK... which __x86_indirect_thunk* symbols *are* being used by Clang in
32-bit mode? I've added __x86_indirect_thunk for 32-bit now, and if
that's *all* the Clang is using then I'll possibly switch GCC into that
mode too.

Can you take care of filing the tickets for %V0 and "=q"
and attribute__((indirect_branch("keep"))) please? With those fixed, I
think we should be OK again.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5213 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180207/0bec21d4/attachment-0001.bin>

Quick response to a detail, I'll respond to more of this when i have more
time.

On Wed, Feb 7, 2018 at 1:55 PM David Woodhouse <dwmw2 at infradead.org>
wrote:
> OK... which __x86_indirect_thunk* symbols *are* being used by Clang in
> 32-bit mode?

__x86_indirect_thunk
__x86_indirect_thunk_eax
__x86_indirect_thunk_ecx
__x86_indirect_thunk_edx


> I've added __x86_indirect_thunk for 32-bit now, and if
> that's *all* the Clang is using then I'll possibly switch GCC into
that
> mode too.
>
> Can you take care of filing the tickets for %V0 and "=q"
> and attribute__((indirect_branch("keep"))) please? With those
fixed, I
> think we should be OK again.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180207/57454d18/attachment.html>

On Wed, 2018-02-07 at 21:55 +0000, David Woodhouse via llvm-dev
wrote:
> Can you take care of filing the tickets for %V0 and "=q"
> and attribute__((indirect_branch("keep"))) please? With those
fixed, I
> think we should be OK again.
Here's %V0 support, which makes the hypervisor guest support build.

diff --git a/lib/Target/X86/X86AsmPrinter.cpp b/lib/Target/X86/X86AsmPrinter.cpp
index 4da7d59df46..f498c098288 100644
--- a/lib/Target/X86/X86AsmPrinter.cpp
+++ b/lib/Target/X86/X86AsmPrinter.cpp
@@ -370,6 +370,8 @@ static void printIntelMemReference(X86AsmPrinter &P,
const MachineInstr *MI,
 static bool printAsmMRegister(X86AsmPrinter &P, const MachineOperand
&MO,
                               char Mode, raw_ostream &O) {
   unsigned Reg = MO.getReg();
+  bool emit_pct = true;
+
   switch (Mode) {
   default: return true;  // Unknown mode.
   case 'b': // Print QImode register
@@ -384,6 +386,9 @@ static bool printAsmMRegister(X86AsmPrinter &P, const
MachineOperand &MO,
   case 'k': // Print SImode register
     Reg = getX86SubSuperRegister(Reg, 32);
     break;
+  case 'V':
+    emit_pct = false;
+    /* fall through */
   case 'q':
     // Print 64-bit register names if 64-bit integer registers are available.
     // Otherwise, print 32-bit register names.
@@ -391,7 +396,10 @@ static bool printAsmMRegister(X86AsmPrinter &P, const
MachineOperand &MO,
     break;
   }
 
-  O << '%' << X86ATTInstPrinter::getRegisterName(Reg);
+  if (emit_pct)
+    O << '%';
+
+  O << X86ATTInstPrinter::getRegisterName(Reg);
   return false;
 }
 
@@ -464,6 +472,7 @@ bool X86AsmPrinter::PrintAsmOperand(const MachineInstr *MI,
unsigned OpNo,
     case 'w': // Print HImode register
     case 'k': // Print SImode register
     case 'q': // Print DImode register
+    case 'V': // Print native register without '%'
       if (MO.isReg())
         return printAsmMRegister(*this, MO, ExtraCode[0], O);
       printOperand(*this, MI, OpNo, O);

-- 
dwmw2