Re: [PATCH -v1.1] x86/mm: Fix INVPCID asm constraint

From: Andy Lutomirski <luto@amacapital.net>
To: Borislav Petkov <bp@alien8.de>
Cc: Michael Matz <matz@suse.de>, Ingo Molnar <mingo@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Oleg Nesterov <oleg@redhat.com>, Brian Gerst <brgerst@gmail.com>,
	Luis Rodriguez <mcgrof@suse.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Denys Vlasenko <dvlasenk@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Toshi Kani <toshi.kani@hp.com>,
	Andrew Lutomirski <luto@kernel.org>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"linux-tip-commits@vger.kernel.org" 
	<linux-tip-commits@vger.kernel.org>
Subject: Re: [PATCH -v1.1] x86/mm: Fix INVPCID asm constraint
Date: Wed, 10 Feb 2016 18:48:50 -0800	[thread overview]
Message-ID: <CALCETrVuq1Kckf5nYoCXV+cT=S2GTRqnZAvF6AEPW-ppBpE_HQ@mail.gmail.com> (raw)
In-Reply-To: <20160210145116.GF23914@pd.tnic>

On Wed, Feb 10, 2016 at 6:51 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Feb 10, 2016 at 02:48:02PM +0100, Michael Matz wrote:
>> Hi,
>>
>> On Wed, 10 Feb 2016, Borislav Petkov wrote:
>>
>> > --- a/arch/x86/include/asm/tlbflush.h
>> > +++ b/arch/x86/include/asm/tlbflush.h
>> > @@ -23,7 +23,7 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
>> >      * invpcid (%rcx), %rax in long mode.
>> >      */
>> >     asm volatile (".byte 0x66, 0x0f, 0x38, 0x82, 0x01"
>> > -                 : : "m" (desc), "a" (type), "c" (desc) : "memory");
>> > +                 : : "m" (*desc), "a" (type), "c" (desc) : "memory");
>>
>> That still doesn't do what you want.  Arrays in C are funny.  *desc is
>> exactly equivalent to desc[0], _not_ to the whole array,
>
> Doh!
>
>> indeed there's no C syntax to name an lvalue of array type in normal
>> expressions. You need to jump through hoops for this:
>>
>>   "m" (*(struct {unsigned long x[2];} *)desc)
>
> Aha! That's why we wrapped the array in clwb() in a struct too, btw:
>
> static inline void clwb(volatile void *__p)
> {
>         volatile struct { char x[64]; } *p = __p;
>         ...
>
>> It'd probably be easier to simply declare the descriptor as a struct,
>> rather than an array, then the original syntax would have been mostly
>> correct:
>>
>>   struct {u64 d[2];} desc = { pcid, addr };
>>   asm ... "m" (desc), "c" (&desc)
>
> Sounds better. Done. How does that below look like?
>
> Thanks Micha!
>
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Wed, 10 Feb 2016 12:53:48 +0100
> Subject: [PATCH -v1.1] x86/mm: Fix INVPCID asm constraint
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> So we want to specify the dependency on both @pcid and @addr so that the
> compiler doesn't reorder accesses to them *before* the TLB flush. But
> for that to work, we need to express this properly in the inline asm and
> deref the whole desc array, not the pointer to it. See clwb() for an
> example.
>
> This fixes the build error on 32-bit:
>
>   arch/x86/include/asm/tlbflush.h: In function ‘__invpcid’:
>   arch/x86/include/asm/tlbflush.h:26:18: error: memory input 0 is not directly addressable
>

Acked-by: Andy Lutomirski <luto@kernel.org>