Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1693893 > unrolled thread

[PATCH 00/16] arm/arm64: Workaround misprogrammed CNTFRQ

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/16] arm/arm64: Workaround misprogrammed CNTFRQ Marc Zyngier <marc.zyngier@arm.com> - 2017-07-21 19:20 +0200
    Re: [PATCH 00/16] arm/arm64: Workaround misprogrammed CNTFRQ Will Deacon <will.deacon@arm.com> - 2017-07-24 14:30 +0200
      Re: [PATCH 00/16] arm/arm64: Workaround misprogrammed CNTFRQ Marc Zyngier <marc.zyngier@arm.com> - 2017-07-24 14:50 +0200
        Re: [PATCH 00/16] arm/arm64: Workaround misprogrammed CNTFRQ Will Deacon <will.deacon@arm.com> - 2017-07-24 15:20 +0200
          Re: [PATCH 00/16] arm/arm64: Workaround misprogrammed CNTFRQ Marc Zyngier <marc.zyngier@arm.com> - 2017-07-24 15:40 +0200
            Re: [PATCH 00/16] arm/arm64: Workaround misprogrammed CNTFRQ Will Deacon <will.deacon@arm.com> - 2017-07-25 13:50 +0200

#1693893 — [PATCH 00/16] arm/arm64: Workaround misprogrammed CNTFRQ

It is an unfortunate situation that CNTFRQ{,_EL0} is often
misprogrammed from the firmware side, leaving it up to the kernel to
work around it. This is usually done by providing an alternative
frequency in the Device Tree.

Unfortunately, CNTFRQ is accessible from EL0, giving userspace the
wrong frequency, and potentially a different frequency per CPU, which
is definitely not what you want. A possible workaround is to trap this
into the kernel and to emulate it (together with the VDSO being
disabled), and this is what this series is achieving.

As it is obvious from the shortlog below, most of the required code is
actually architecture specific, and required to handle these traps
properly.

On arm64, we mostly have things in order already for AArch64 code. But
we lack the infrastructure to deal with cp15 traps from a 32bit
userspace. We could do it by reading userspace the userspace code and
use the undef_hook mechanism, but that's clearly very ugly.

Instead, we mimic the way we trap system register accesses for 64bit
code. An added complexity comes from the fact that we need to handle
condition execution in both ARM and Thumb modes (including the IT
state) when trapping such accesses.

32bit ARM is also lacking any form of conditional Thumb handling, so
we add that before adding the undef handlers.

This has been tested as KVM guests, using kvmtool and the
--override-bad-firmware-cntfrq option to trick the VM into using a
different frequency.

Marc Zyngier (16):
  arm64: Use arch_timer_get_rate when trapping CNTFRQ_EL0
  arm64: Add decoding macros for CP15_32 and CP15_64 traps
  arm64: compat: Add separate CP15 trapping hook
  arm64: compat: Add condition code checks and IT advance
  arm64: compat: Add cp15_32 and cp15_64 handler arrays
  arm64: compat: Add CNTVCT trap handler
  arm64: compat: Add CNTFRQ trap handler
  ARM: Let arm_check_condition work with Thumb
  ARM: Check condition code before trying to handle an UNDEF
  ARM: Add arm_advance_itstate helper
  ARM: Advance the IT state on successful emulation of an UNDEF
  ARM: Simplify condition checks in swp_handler
  ARM: Handle trapping of CNTVCT from userspace
  ARM: Handle trapping of CNTFRQ from userspace
  clocksource/arm_arch_timer: Add helper to disable VDSO fastpath
  clocksource/arm_arch_timer: Trap user access to CNT{VCT,FRQ} if CNTFRQ
    is invalid

 arch/arm/include/asm/opcodes.h       |   2 +
 arch/arm/kernel/opcodes.c            |  68 +++++++++++++++-
 arch/arm/kernel/swp_emulate.c        |  15 +---
 arch/arm/kernel/traps.c              |  64 ++++++++++++++-
 arch/arm64/include/asm/esr.h         |  58 +++++++++++++
 arch/arm64/kernel/entry.S            |  14 +++-
 arch/arm64/kernel/traps.c            | 152 ++++++++++++++++++++++++++++++++++-
 drivers/clocksource/arm_arch_timer.c |  56 +++++++++----
 8 files changed, 397 insertions(+), 32 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1694677

On Fri, Jul 21, 2017 at 06:15:26PM +0100, Marc Zyngier wrote:
> It is an unfortunate situation that CNTFRQ{,_EL0} is often
> misprogrammed from the firmware side, leaving it up to the kernel to
> work around it. This is usually done by providing an alternative
> frequency in the Device Tree.
> 
> Unfortunately, CNTFRQ is accessible from EL0, giving userspace the
> wrong frequency, and potentially a different frequency per CPU, which
> is definitely not what you want. A possible workaround is to trap this
> into the kernel and to emulate it (together with the VDSO being
> disabled), and this is what this series is achieving.

Which userspace is actually affected by a broken CNTFRQ register? I suspect
most users will be more upset at losing their (perfectly functional) vDSO
acceleration than they are about having a broken CNTFRQ value that is hardly
ever used, especially since this affects quite a few systems.

Will

[toc] | [prev] | [next] | [standalone]

#1694680

On 24/07/17 13:19, Will Deacon wrote:
> On Fri, Jul 21, 2017 at 06:15:26PM +0100, Marc Zyngier wrote:
>> It is an unfortunate situation that CNTFRQ{,_EL0} is often
>> misprogrammed from the firmware side, leaving it up to the kernel to
>> work around it. This is usually done by providing an alternative
>> frequency in the Device Tree.
>>
>> Unfortunately, CNTFRQ is accessible from EL0, giving userspace the
>> wrong frequency, and potentially a different frequency per CPU, which
>> is definitely not what you want. A possible workaround is to trap this
>> into the kernel and to emulate it (together with the VDSO being
>> disabled), and this is what this series is achieving.
> 
> Which userspace is actually affected by a broken CNTFRQ register? I suspect
> most users will be more upset at losing their (perfectly functional) vDSO
> acceleration than they are about having a broken CNTFRQ value that is hardly
> ever used, especially since this affects quite a few systems.

OpenMPI is one of the things I'm aware of (we broke it when implementing
the first set of timer workarounds), and from trawling the Debian code
search, at least HHVM is another candidate. How this will affect them is
anybody's guess.

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1694705

On Mon, Jul 24, 2017 at 01:48:37PM +0100, Marc Zyngier wrote:
> On 24/07/17 13:19, Will Deacon wrote:
> > On Fri, Jul 21, 2017 at 06:15:26PM +0100, Marc Zyngier wrote:
> >> It is an unfortunate situation that CNTFRQ{,_EL0} is often
> >> misprogrammed from the firmware side, leaving it up to the kernel to
> >> work around it. This is usually done by providing an alternative
> >> frequency in the Device Tree.
> >>
> >> Unfortunately, CNTFRQ is accessible from EL0, giving userspace the
> >> wrong frequency, and potentially a different frequency per CPU, which
> >> is definitely not what you want. A possible workaround is to trap this
> >> into the kernel and to emulate it (together with the VDSO being
> >> disabled), and this is what this series is achieving.
> > 
> > Which userspace is actually affected by a broken CNTFRQ register? I suspect
> > most users will be more upset at losing their (perfectly functional) vDSO
> > acceleration than they are about having a broken CNTFRQ value that is hardly
> > ever used, especially since this affects quite a few systems.
> 
> OpenMPI is one of the things I'm aware of (we broke it when implementing
> the first set of timer workarounds), and from trawling the Debian code
> search, at least HHVM is another candidate. How this will affect them is
> anybody's guess.

The latest mcrouter sources pulled into HHVM don't use cntfrq, but you're
right about OpenMPI. However, these things are using the counter directly
as a performance optimisation: the moment we start trapping then they've
lost. I doubt it's much better than giving the wrong data for the
frequency (i.e. they're just as broken in both cases).

So, if they want to run on these systems, their best bet is to use the
vDSO-accelerated clock_gettime implementation. Yes, there's a dispatch cost
compared to an inline asm, but it will beat the pants off a trap to the
kernel. The problem is that this patch series prevents them from doing that
and just means they're screwed whatever they do. We can point at the broken
firmware, but it doesn't feel to me like this workaround is really helping
anybody :/.

Will

[toc] | [prev] | [next] | [standalone]

#1694714

On 24/07/17 14:12, Will Deacon wrote:
> On Mon, Jul 24, 2017 at 01:48:37PM +0100, Marc Zyngier wrote:
>> On 24/07/17 13:19, Will Deacon wrote:
>>> On Fri, Jul 21, 2017 at 06:15:26PM +0100, Marc Zyngier wrote:
>>>> It is an unfortunate situation that CNTFRQ{,_EL0} is often
>>>> misprogrammed from the firmware side, leaving it up to the kernel to
>>>> work around it. This is usually done by providing an alternative
>>>> frequency in the Device Tree.
>>>>
>>>> Unfortunately, CNTFRQ is accessible from EL0, giving userspace the
>>>> wrong frequency, and potentially a different frequency per CPU, which
>>>> is definitely not what you want. A possible workaround is to trap this
>>>> into the kernel and to emulate it (together with the VDSO being
>>>> disabled), and this is what this series is achieving.
>>>
>>> Which userspace is actually affected by a broken CNTFRQ register? I suspect
>>> most users will be more upset at losing their (perfectly functional) vDSO
>>> acceleration than they are about having a broken CNTFRQ value that is hardly
>>> ever used, especially since this affects quite a few systems.
>>
>> OpenMPI is one of the things I'm aware of (we broke it when implementing
>> the first set of timer workarounds), and from trawling the Debian code
>> search, at least HHVM is another candidate. How this will affect them is
>> anybody's guess.
> 
> The latest mcrouter sources pulled into HHVM don't use cntfrq, but you're
> right about OpenMPI. However, these things are using the counter directly
> as a performance optimisation: the moment we start trapping then they've
> lost. I doubt it's much better than giving the wrong data for the
> frequency (i.e. they're just as broken in both cases).
> 
> So, if they want to run on these systems, their best bet is to use the
> vDSO-accelerated clock_gettime implementation. Yes, there's a dispatch cost
> compared to an inline asm, but it will beat the pants off a trap to the
> kernel. The problem is that this patch series prevents them from doing that
> and just means they're screwed whatever they do. We can point at the broken
> firmware, but it doesn't feel to me like this workaround is really helping
> anybody :/.

Fair enough.

It still remains that our trapping story is inconsistent. When we
workaround timer issues, we do disable the VDSO and trap both cntvct and
cntfrq. These will trap to EL1, and we'll deliver a SIGILL to userspace,
while 64bit userspace will just work. Is that something we want to address?

With a 32bit kernel, we don't trap anything yet (because we don't work
around any of the errata existing on 64bit systems), so I guess this
could be postponed until we actually make these workarounds available on
32bit (for KVM guests).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1695639

On Mon, Jul 24, 2017 at 02:35:08PM +0100, Marc Zyngier wrote:
> On 24/07/17 14:12, Will Deacon wrote:
> > On Mon, Jul 24, 2017 at 01:48:37PM +0100, Marc Zyngier wrote:
> >> On 24/07/17 13:19, Will Deacon wrote:
> >>> On Fri, Jul 21, 2017 at 06:15:26PM +0100, Marc Zyngier wrote:
> >>>> It is an unfortunate situation that CNTFRQ{,_EL0} is often
> >>>> misprogrammed from the firmware side, leaving it up to the kernel to
> >>>> work around it. This is usually done by providing an alternative
> >>>> frequency in the Device Tree.
> >>>>
> >>>> Unfortunately, CNTFRQ is accessible from EL0, giving userspace the
> >>>> wrong frequency, and potentially a different frequency per CPU, which
> >>>> is definitely not what you want. A possible workaround is to trap this
> >>>> into the kernel and to emulate it (together with the VDSO being
> >>>> disabled), and this is what this series is achieving.
> >>>
> >>> Which userspace is actually affected by a broken CNTFRQ register? I suspect
> >>> most users will be more upset at losing their (perfectly functional) vDSO
> >>> acceleration than they are about having a broken CNTFRQ value that is hardly
> >>> ever used, especially since this affects quite a few systems.
> >>
> >> OpenMPI is one of the things I'm aware of (we broke it when implementing
> >> the first set of timer workarounds), and from trawling the Debian code
> >> search, at least HHVM is another candidate. How this will affect them is
> >> anybody's guess.
> > 
> > The latest mcrouter sources pulled into HHVM don't use cntfrq, but you're
> > right about OpenMPI. However, these things are using the counter directly
> > as a performance optimisation: the moment we start trapping then they've
> > lost. I doubt it's much better than giving the wrong data for the
> > frequency (i.e. they're just as broken in both cases).
> > 
> > So, if they want to run on these systems, their best bet is to use the
> > vDSO-accelerated clock_gettime implementation. Yes, there's a dispatch cost
> > compared to an inline asm, but it will beat the pants off a trap to the
> > kernel. The problem is that this patch series prevents them from doing that
> > and just means they're screwed whatever they do. We can point at the broken
> > firmware, but it doesn't feel to me like this workaround is really helping
> > anybody :/.
> 
> Fair enough.
> 
> It still remains that our trapping story is inconsistent. When we
> workaround timer issues, we do disable the VDSO and trap both cntvct and
> cntfrq. These will trap to EL1, and we'll deliver a SIGILL to userspace,
> while 64bit userspace will just work. Is that something we want to address?

That's a good question, and I guess it depends on whether or not we have
people asking for that. Are we aware of userspace that is being broken by
this? If so, then we can address it, otherwise we can wait until somebody
shouts. It's not unlikely that any 32-bit code trying to use the counter
registers directly already handles SIGILL, since that's what will happen
on older CPUs without the arch timer anyway.

Will

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web