Re: [GIT PULL] arm64 fixes for 5.11-rc6

From: Catalin Marinas <catalin.marinas@arm.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	Will Deacon <will@kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [GIT PULL] arm64 fixes for 5.11-rc6
Date: Sun, 31 Jan 2021 18:54:44 +0000	[thread overview]
Message-ID: <20210131185443.GA29083@gaia> (raw)
In-Reply-To: <CAHk-=wh=1K+i6cd-Y_St3ktJAdrqriXf=ct-DcFUR2GkrraLaA@mail.gmail.com>

On Fri, Jan 29, 2021 at 02:09:05PM -0800, Linus Torvalds wrote:
> On Fri, Jan 29, 2021 at 11:03 AM Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> >
> > arm64 fixes:
> >
> > - Fix the virt_addr_valid() returning true for < PAGE_OFFSET addresses.
> 
> That's a really odd fix.
> 
> It went from an incorrect bitwise operation (masking) to an _odd_
> bitwise operation (xor).
> 
> Yes, PAGE_OFFSET has the bit pattern of all upper bits set, so "(addr
> ^ PAGE_OFFSET)" by definition reverses the upper bits - and for a
> valid case turns them to zero.
> 
> But isn't the *logical* thing to do to use a subtract instead? For the
> valid cases, the two do the same thing (clear the upper bits), but
> just conceptually, isn't the operation that you actually want to do
> "(addr - PAGE_OFFSET)"?
> 
> IOW, why is it using that odd xor pattern that doesn't make much
> sense? I believe it _works_, but it looks very strange to me.

This macro used to test a single bit and it evolved into a bitmask. So,
yes, basically what we need is:

#define __is_lm_address(addr)	((u64)(addr) >= PAGE_OFFSET && \
				 (u64)(addr) < PAGE_END)

I wasn't sure whether the code generation with two comparisons is
similar to the xor variant but the compiler should probably be smart
enough to use CMP and CCMP. In the grand scheme, it probably doesn't
even matter.

Unless I miss something, I don't see any overflow issues even if we do
(((u64)addr - PAGE_OFFSET) < (PAGE_END - PAGE_OFFSET)).

We can backport the fix already upstream and clean-up the code in
mainline going forward (after some sanity check on the code generation).
It would be easier to parse in the future.

> Also, shouldn't _lm_to_phys() do the same? It does that "mask upper
> bits" too that was problematic in __is_lm_address(). Again, shouldn't
> that logically be a subtract op?

Yes, that's similar and a subtract should do.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel