From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from aserp2130.oracle.com ([141.146.126.79]) by Galois.linutronix.de with esmtps (TLS1.2:RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fCytk-00078y-U2 for speck@linutronix.de; Mon, 30 Apr 2018 04:50:42 +0200 Received: from pps.filterd (aserp2130.oracle.com [127.0.0.1]) by aserp2130.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w3U2kmLH132718 for ; Mon, 30 Apr 2018 02:50:34 GMT Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by aserp2130.oracle.com with ESMTP id 2hmeg5jfuq-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 30 Apr 2018 02:50:34 +0000 Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id w3U2oXbk005083 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 30 Apr 2018 02:50:33 GMT Received: from abhmp0017.oracle.com (abhmp0017.oracle.com [141.146.116.23]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id w3U2oXot022614 for ; Mon, 30 Apr 2018 02:50:33 GMT Date: Sun, 29 Apr 2018 22:50:32 -0400 From: Konrad Rzeszutek Wilk Subject: [MODERATED] Re: [patch V7 03/15] SBB 3 Message-ID: <20180430025031.GA411@char.us.oracle.com> References: <20180429193045.711908246@linutronix.de> <20180429193937.704474597@linutronix.de> MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: ..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