From: Robin Murphy <robin.murphy@arm.com>
To: Marc Zyngier <marc.zyngier@arm.com>,
linux-arm-kernel@lists.infradead.org
Cc: Mark Rutland <mark.rutland@arm.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Will Deacon <will.deacon@arm.com>
Subject: Re: [PATCH 1/4] arm64: Restrict ARM64_ERRATUM_1188873 mitigation to AArch32
Date: Tue, 9 Apr 2019 10:49:08 +0100 [thread overview]
Message-ID: <f122874b-d091-96f6-b752-1984804ad6ff@arm.com> (raw)
In-Reply-To: <379e2ab4-ff49-c426-e931-2d52fcd7d494@arm.com>
On 09/04/2019 10:18, Marc Zyngier wrote:
> On 08/04/2019 19:34, Robin Murphy wrote:
>> Hi Marc,
>>
>> On 08/04/2019 17:02, Marc Zyngier wrote:
>>> We currently deal with ARM64_ERRATUM_1188873 by always trapping EL0
>>> accesses for both instruction sets. Although nothing wrong comes out
>>> of that, people trying to squeeze the last drop of performance from
>>> buggy HW find this over the top. Oh well.
>>>
>>> Let's change the mitigation by flipping the counter enable bit
>>> on return to userspace. Non-broken HW gets an extra branch on
>>> the fast path, which is hopefully not the end of the world.
>>> The arch timer workaround it also removed.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>> arch/arm64/kernel/entry.S | 23 +++++++++++++++++++++++
>>> drivers/clocksource/arm_arch_timer.c | 15 ---------------
>>> 2 files changed, 23 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>> index c50a7a75f2e0..e0aed21ab402 100644
>>> --- a/arch/arm64/kernel/entry.S
>>> +++ b/arch/arm64/kernel/entry.S
>>> @@ -336,6 +336,29 @@ alternative_if ARM64_WORKAROUND_845719
>>> alternative_else_nop_endif
>>> #endif
>>> 3:
>>> +#ifdef CONFIG_ARM64_ERRATUM_1188873
>>> +alternative_if_not ARM64_WORKAROUND_1188873
>>> + b 1f
>>> +alternative_else_nop_endif
>>> +
>>> + ubfx x0, x22, #4, #1 // Extract PSR_MODE32
>>> + eor x0, x0, #1 // Negate it
>>> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>>> + mrs x1, cntkctl_el1
>>> +alternative_else
>>> + mrs x1, cnthctl_el2
>>> +alternative_endif
>>> + ubfx x2, x1, #1, #1 // Extract EL0VCTEN
>>> + cmp x2, x0
>>> + b.eq 1f // Matches, nothing to do
>>> + bfi x1, x0, #1, #1 // Move EL0VCTEN in place
>>> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>>> + msr cntkctl_el1, x1
>>> +alternative_else
>>> + msr cnthctl_el2, x1
>>> +alternative_endif
>>
>> Unless I've gone horribly wrong in my reasoning somewhere, I reckon this
>> could be a fair bit shorter...
>>
>>
>> alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>> mrs x1, cntkctl_el1
>> alternative_else
>> mrs x1, cnthctl_el2
>> alternative_endif
>> eon x0, x1, x22, lsr #3
>> eor x1, x1, #1
>> tbz x0, #1, 1f
>> alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>> msr cntkctl_el1, x1
>> alternative_else
>> msr cnthctl_el2, x1
>> alternative_endif
>>
>>
>> ...although whether that's a good idea at all probably depends mostly on
>> how many comments you think it needs :)
>
> Neat! Totally impossible to decipher, and thus perfect! ;-) The way I
> understand it that pstate.m32 and cntkctl_el1.el0vcten must always have
> opposite values. If they don't, we flip el0vcten.
Yup, that's the trick!
> We could move the eor after the tbz, I think. And we can also get rid of
> the VHE alternative, as cntkctl_el1 is a valid accessor for cnthctl_el2
> in that case.
Indeed I initially wrote the logic TBZ-first, but then decided to flip
them in the spirit of "trying to squeeze the last drop of performance"
:) Reason being that on most of our microarchitectures a
shifted-register operand tends to incur +1 cycle of result latency, so
in theory we can slip in the EOR for free while waiting on the x0
dependency. I may have got a bit carried away there, since on reflection
this is of course only going to be run by the affected CPUs which should
be big and OoO enough to make that entirely irrelevant, so dialling it
back a bit for the sake of a little clarity seems a sensible thing to do.
> Shal we settle for this?
>
> +#ifdef CONFIG_ARM64_ERRATUM_1188873
> +alternative_if_not ARM64_WORKAROUND_1188873
> + b 1f
> +alternative_else_nop_endif
> +
> + // if (!x22.mode32 != cntkctl_el1.el0vcten)
> + // cntkctl_el1.el0vcten = !cntkctl_el1.el0vcten
> +
> + mrs x1, cntkctl_el1
> +
> + eon x0, x1, x22, lsr #3
> + tbz x0, #1, 1f
> +
> + eor x1, x1, #1
> + msr cntkctl_el1, x1
> +1:
> +#endif
Nice, that looks about as tidy as it'll ever get!
Cheers,
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-04-09 9:49 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-08 16:02 [PATCH 0/4] arm64: Rework handling of erratum 1188873 Marc Zyngier
2019-04-08 16:02 ` [PATCH 1/4] arm64: Restrict ARM64_ERRATUM_1188873 mitigation to AArch32 Marc Zyngier
2019-04-08 18:34 ` Robin Murphy
2019-04-09 9:18 ` Marc Zyngier
2019-04-09 9:49 ` Robin Murphy [this message]
2019-04-12 13:17 ` Will Deacon
2019-04-15 8:15 ` Marc Zyngier
2019-04-15 8:26 ` Daniel Lezcano
2019-04-08 16:02 ` [PATCH 2/4] arm64: Make ARM64_ERRATUM_1188873 depend on COMPAT Marc Zyngier
2019-04-08 16:02 ` [PATCH 3/4] arm64: Add part number for Neoverse N1 Marc Zyngier
2019-04-08 16:02 ` [PATCH 4/4] arm64: Apply ARM64_ERRATUM_1188873 to Neoverse-N1 Marc Zyngier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f122874b-d091-96f6-b752-1984804ad6ff@arm.com \
--to=robin.murphy@arm.com \
--cc=catalin.marinas@arm.com \
--cc=daniel.lezcano@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=marc.zyngier@arm.com \
--cc=mark.rutland@arm.com \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).