Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>, x86@kernel.org,
        Andrey Ryabinin <aryabinin@virtuozzo.com>,
        Alexander Potapenko <glider@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        "H . J . Lu" <hjl.tools@gmail.com>,
        Andi Kleen <ak@linux.intel.com>,
        Rick Edgecombe <rick.p.edgecombe@intel.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [RFCv2 05/10] x86/mm: Provide untagged_addr() helper
In-Reply-To: <87sfpevl1g.ffs@tglx>
References: <20220511022751.65540-1-kirill.shutemov@linux.intel.com>
 <20220511022751.65540-7-kirill.shutemov@linux.intel.com>
 <87a6bmx5lt.ffs@tglx> <Yn0YdPNG/Q3lf+4G@hirez.programming.kicks-ass.net>
 <87sfpevl1g.ffs@tglx>
Date:   Fri, 13 May 2022 01:14:51 +0200
Message-ID: <87wneqtkb8.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Thu, May 12 2022 at 17:16, Thomas Gleixner wrote:
> On Thu, May 12 2022 at 16:23, Peter Zijlstra wrote:
>> On Thu, May 12, 2022 at 03:06:38PM +0200, Thomas Gleixner wrote:
>>
>>> #define untagged_addr(addr)	({			\
>>> 	u64 __addr = (__force u64)(addr);		\
>>> 							\
>>> 	__addr &= current->thread.lam_untag_mask;	\
>>> 	(__force __typeof__(addr))__addr;		\
>>> })
>>> 
>>> No conditionals, fast _and_ correct. Setting this untag mask up once
>>> when LAM is enabled is not rocket science.
>>
>> But that goes wrong if someone ever wants to untag a kernel address and
>> not use the result for access_ok().
>>
>> I'd feel better about something like:
>>
>> 	s64 __addr = (addr);
>> 	s64 __sign = __addr;
>>
>> 	__sign >>= 63;
>> 	__sign &= lam_untag_mask;
>
> that needs to be
>
>  	__sign &= ~lam_untag_mask;
>
>> 	__addr &= lam_untag_mask;
>> 	__addr |= __sign;
>>
>> 	__addr;
>>
>> Which simply extends bit 63 downwards -- although possibly there's an
>> easier way to do that, this is pretty gross.
>
> For the price of a conditional:
>
>     __addr &= lam_untag_mask;
>     if (__addr & BIT(63))
>         __addr |= ~lam_untag_mask;
>
> Now you have the choice between gross and ugly.

Though we can also replace your flavour of gross with a different
flavour of gross:

	s64 sign = (s64)(addr) >> 63;

	addr ^= sign;
	addr &= mask;
	addr ^= sign;

After twisting my brain around replacing gross by something differently
gross and coming up with the gem above I actually did compile the
variants and discovered that GCC compiles your flavour of gross exactly
to this:

        mov    %rdi,%rax
        sar    $0x3f,%rax
        xor    %rax,%rdi
        and    %rsi,%rdi
        xor    %rdi,%rax

I have to admit that compilers are sometimes pretty smart. I might have
to rethink my prejudice. :)

But then clang converts your flavour of 'gross' to:

     	mov    %rsi,%rax
     	mov    %rsi,%rcx
     	and    %rdi,%rax
     	sar    $0x3f,%rdi
     	not    %rcx
     	and    %rdi,%rcx
     	or     %rcx,%rax

and my explicit flavour to:

      	mov    %rdi,%rax
      	mov    %rdi,%rcx
      	sar    $0x3f,%rcx
      	xor    %rcx,%rax
      	and    %rsi,%rax
      	xor    %rcx,%rax

which is at least slightly less retarted, but still has a pointless mov
there. Note, that this was compiled in user space with noinline
functions. I did some inlined variants as well and clang still insists
on using an extra register for no obvious reason. This might be more
efficient in reality, but I haven't bothered to write a test which
might give an answer via perf.

The ugly with the conditional resolves for both compilers to:

       	mov    %rsi,%rax
       	mov    %rsi,%rcx
       	not    %rcx
       	or     %rdi,%rcx
       	and    %rdi,%rax
       	test   %rdi,%rdi
       	cmovs  %rcx,%rax

At least they agree on that one.

But whatever we chose, it's sad, that we need to have support for
interfaces which swallow any pointer (user or kernel) because otherwise
this really boils down to a single OR resp. AND operation plus the
according mov to retrieve the mask.

Thanks,

        tglx