linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ionela Voinescu <ionela.voinescu@arm.com>
To: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com,
	maz@kernel.org, sudeep.holla@arm.com, dietmar.eggemann@arm.com,
	peterz@infradead.org, mingo@redhat.com, ggherdovich@suse.cz,
	vincent.guittot@linaro.org, linux-arm-kernel@lists.infradead.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jonathan Corbet <corbet@lwn.net>
Subject: Re: [PATCH v2 4/6] Documentation: arm64: document support for the AMU extension
Date: Fri, 31 Jan 2020 09:54:12 +0000	[thread overview]
Message-ID: <20200131095412.GA17655@arm.com> (raw)
In-Reply-To: <20200130182653.GA123407@ewhatever.cambridge.arm.com>

On Thursday 30 Jan 2020 at 18:26:53 (+0000), Suzuki K Poulose wrote:
[..]
> > > > +Firmware (code running at higher exception levels, e.g. arm-tf) support is
> > > > +needed to:
> > > > + - Enable access for lower exception levels (EL2 and EL1) to the AMU
> > > > +   registers.
> > > > + - Enable the counters. If not enabled these will read as 0.
> > > > + - Save/restore the counters before/after the CPU is being put/brought up
> > > > +   from the 'off' power state.
> > > > +
> > > > +When using kernels that have this configuration enabled but boot with
> > > > +broken firmware the user may experience panics or lockups when accessing
> > > > +the counter registers. Even if these symptoms are not observed, the
> > > > +values returned by the register reads might not correctly reflect reality.
> > > > +Most commonly, the counters will read as 0, indicating that they are not
> > > > +enabled. If proper support is not provided in firmware it's best to disable
> > > > +CONFIG_ARM64_AMU_EXTN.
> > > 
> > > For the sake of one kernel runs everywhere, do we need some other
> > > mechanism to disable the AMU. e.g kernel parameter to disable amu
> > > at runtime ?
> > >
> > 
> > The reason I've not added this is twofold:
> >  - Even if we add this, it should be in order to disable the use of the
> >    counters for a certain purpose, in this case  frequency invariance.
> >    On its own AMU provides the counters but it does not mandate their
> >    use.
> >  - I could add something to disable the use of the core and cycle
> >    counters for frequency invariance at runtime, but I doubt that
> >    anyone would use it. Logically it makes sense to use the counters
> >    order to have a more accurate view of the performance that the CPUs
> >    are actually providing. Therefore, until anyone asks for this, I
> >    thought it's better to keep it simple and not add extra switches,
> >    until there is a use for them.
> > 
> > Does it make sense?
> 
> The comment is about addressing someone who must run an "AMU" enabled
> kernel ("one kernel") on a system with potentially "broken firmware",
> where there is no option to use the system as you mention above,
> the firmware could panic. How common is the "broken firmware" ?
> Right now there is no way to ensure "firmware" is sane and if
> someone detects that firmware is broken, there is no way to
> disable the AMU if they are running a standard distro kernel.
> A kernel parameter could prevent the AMU capability from
> being detected on a broken system and thus make it usable
> (without the AMU of course). Now, if the "broken firmware"
> is extremely rare, we could simply ignore this case and
> ignore the suggestion.
> 
> Suzuki
> 
>

Sorry Suzuki, I initially interpreted the question independently from
the context and only thought about cases where they are working
correctly but users might want to disable the use of them.

In this case, I don't see any harm in adding a command line parameter
to disable the use of the unit, even if it's only to support firmware
that does not support AMU at all, rather than the implementation being
broken.

I'm not really sure how common bad firmware would be. I suppose that
firmware as bad as to cause firmware panics and lockups would be quite
rare, but scenarios where firmware might not properly support AMU and
result in kernel lockups could be more often, and this would handle
both.

Thank you,
Ionela.

  reply	other threads:[~2020-01-31  9:54 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-18 18:26 [PATCH v2 0/6] arm64: ARMv8.4 Activity Monitors support Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 1/6] arm64: add support for the AMU extension v1 Ionela Voinescu
2020-01-23 17:04   ` Valentin Schneider
2020-01-23 18:32     ` Ionela Voinescu
2020-01-24 12:00       ` Valentin Schneider
2020-01-28 11:00         ` Ionela Voinescu
2020-01-28 16:34   ` Suzuki Kuruppassery Poulose
2020-01-29 16:42     ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 2/6] arm64: trap to EL1 accesses to AMU counters from EL0 Ionela Voinescu
2020-01-23 17:04   ` Valentin Schneider
2020-01-23 17:34     ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 3/6] arm64/kvm: disable access to AMU registers from kvm guests Ionela Voinescu
2020-01-27 15:33   ` Valentin Schneider
2020-01-28 15:48     ` Ionela Voinescu
2020-01-28 17:26     ` Suzuki Kuruppassery Poulose
2020-01-28 17:37       ` Valentin Schneider
2020-01-28 17:52         ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 4/6] Documentation: arm64: document support for the AMU extension Ionela Voinescu
2020-01-27 16:47   ` Valentin Schneider
2020-01-28 16:53     ` Ionela Voinescu
2020-01-28 18:36       ` Valentin Schneider
2020-01-30 15:04   ` Suzuki Kuruppassery Poulose
2020-01-30 16:45     ` Ionela Voinescu
2020-01-30 18:26       ` Suzuki K Poulose
2020-01-31  9:54         ` Ionela Voinescu [this message]
2019-12-18 18:26 ` [PATCH v2 5/6] TEMP: sched: add interface for counter-based frequency invariance Ionela Voinescu
2020-01-29 19:37   ` Peter Zijlstra
2020-01-30 15:33     ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 6/6] arm64: use activity monitors for " Ionela Voinescu
2020-01-23 11:49   ` Lukasz Luba
2020-01-23 17:07     ` Ionela Voinescu
2020-01-24  1:19       ` Lukasz Luba
2020-01-24 13:12         ` Ionela Voinescu
2020-01-24 15:17           ` Lukasz Luba
2020-01-28 17:36             ` Ionela Voinescu
2020-01-29 17:13   ` Valentin Schneider
2020-01-29 17:52     ` Ionela Voinescu
2020-01-29 23:39     ` Valentin Schneider
2020-01-30 15:49       ` Ionela Voinescu
2020-01-30 16:11         ` Valentin Schneider

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=20200131095412.GA17655@arm.com \
    --to=ionela.voinescu@arm.com \
    --cc=Suzuki.Poulose@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=corbet@lwn.net \
    --cc=dietmar.eggemann@arm.com \
    --cc=ggherdovich@suse.cz \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=sudeep.holla@arm.com \
    --cc=vincent.guittot@linaro.org \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v2 4/6] Documentation: arm64: document support for the AMU extension' \
    /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

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