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

[Andy Lutomirski]

On Wed, Feb 10, 2016 at 6:51 AM, Borislav Petkov <bp@xxxxxxxxx> 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@xxxxxxx>
> 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@xxxxxxxxxx>