Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
Cc:     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,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [RFCv2 05/10] x86/mm: Provide untagged_addr() helper
In-Reply-To: <20220511022751.65540-7-kirill.shutemov@linux.intel.com>
References: <20220511022751.65540-1-kirill.shutemov@linux.intel.com>
 <20220511022751.65540-7-kirill.shutemov@linux.intel.com>
Date:   Thu, 12 May 2022 15:06:38 +0200
Message-ID: <87a6bmx5lt.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Wed, May 11 2022 at 05:27, Kirill A. Shutemov wrote:
> +#define __untagged_addr(addr, n)	\
> +	((__force __typeof__(addr))sign_extend64((__force u64)(addr), n))

How is this supposed to be correct? This sign extends based on bit 47
resp. 56, i.e. the topmost bit of the userspace address space for the
LAM mode.

So if that bit _is_ set, then the result has bit 48-63 resp. 57-63 set
as well. Not really what you want, right?

This has to mask out bit 48-62 resp. 57-62 and leave all other bits
alone.

> +#define untagged_addr(addr)	({					\
> +	u64 __addr = (__force u64)(addr);				\
> +	if (__addr >> 63 == 0) {					\
> +		if (current->thread.features & X86_THREAD_LAM_U57)	\
> +			__addr &= __untagged_addr(__addr, 56);		\
> +		else if (current->thread.features & X86_THREAD_LAM_U48)	\
> +			__addr &= __untagged_addr(__addr, 47);		\
> +	}								\
> +	(__force __typeof__(addr))__addr;				\
> +})

So this wants something like this:

#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.

Thanks,

        tglx