Re: [PATCH 8/8] arm64: Rewrite __arch_clear_user()

From: Mark Rutland <mark.rutland@arm.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: will@kernel.org, catalin.marinas@arm.com,
	linux-arm-kernel@lists.infradead.org, yangyingliang@huawei.com,
	shenkai8@huawei.com
Subject: Re: [PATCH 8/8] arm64: Rewrite __arch_clear_user()
Date: Wed, 12 May 2021 14:06:58 +0100	[thread overview]
Message-ID: <20210512130418.GF88854@C02TD0UTHF1T.local> (raw)
In-Reply-To: <b1689c7f-6c61-41ca-0976-a32ceaa7eeeb@arm.com>

On Wed, May 12, 2021 at 12:31:39PM +0100, Robin Murphy wrote:
> On 2021-05-12 11:48, Mark Rutland wrote:
> > On Tue, May 11, 2021 at 05:12:38PM +0100, Robin Murphy wrote:
> > > Rewrite __arch_clear_user() with regular
> > > USER() annotations so that it's clearer what's going on, and take the
> > > opportunity to minimise the branchiness in the most common paths, which
> > > also allows the exception fixup to return a more accurate result.
> > 
> > IIUC this isn't always accurate for the {4,2,1}-byte cases; example
> > below. I'm not sure whether that's intentional since the commit message
> > says "more accurate" rather than "accurate".
> 
> Indeed, the "more" was definitely significant :)

:)

> > If we think that under-estimating is fine, I reckon it'd be worth a
> > comment to make that clear.
> 
> Indeed for smaller amounts there's no change in fixup behaviour at all, but
> I have to assume that underestimating by up to 100% is probably OK since
> we've been underestimating by fully 100% for nearly 10 years now. I don't
> believe it's worth having any more complexity than necessary for the fault
> case - grepping for clear_user() usage suggests that nobody really cares
> about the return value beyond whether it's zero or not, so the minor
> "improvement" here is more of a nice-to-have TBH.
> 
> The existing comment doesn't actually explain anything either, which is why
> I didn't replace it, but I'm happy to add something if you like.

I don't have strong feelings either way, but I think that we should at
least document this, since that'll at least save us rehashing the same
point in future. :)

That said, IIUC to make this always accurate we only need two ADDs (diff
below). Since those will only be executed at most once each, I suspect
they won't have a measureable impact in practice.

So maybe it's worth adding them to avoid any risk that someone needs
this to be accurate in future?

Mark.

---->8----

diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S
index 1005345b4066..7ef553ec2677 100644
--- a/arch/arm64/lib/clear_user.S
+++ b/arch/arm64/lib/clear_user.S
@@ -32,12 +32,14 @@ USER(9f, sttr       xzr, [x2, #-8])
 
 2:     tbz     x1, #2, 3f
 USER(9f, sttr  wzr, [x0])
+       add     x0, x0, #4
 USER(9f, sttr  wzr, [x2, #-4])
        mov     x0, #0
        ret
 
 3:     tbz     x1, #1, 4f
 USER(9f, sttrh wzr, [x0])
+       add     x0, x0, #2
 4:     tbz     x1, #0, 5f
 USER(9f, sttrb wzr, [x2, #-1])
 5:     mov     x0, #0

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