linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Catalin Marinas <catalin.marinas@arm.com>
To: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: will@kernel.org, mark.rutland@arm.com, maz@kernel.org,
	suzuki.poulose@arm.com, sudeep.holla@arm.com,
	lukasz.luba@arm.com, valentin.schneider@arm.com,
	dietmar.eggemann@arm.com, rjw@rjwysocki.net,
	pkondeti@codeaurora.org, peterz@infradead.org, mingo@redhat.com,
	vincent.guittot@linaro.org, viresh.kumar@linaro.org,
	linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v5 1/7] arm64: add support for the AMU extension v1
Date: Tue, 3 Mar 2020 16:58:45 +0000	[thread overview]
Message-ID: <20200303165845.GF823373@arrakis.emea.arm.com> (raw)
In-Reply-To: <20200302142326.GA15709@arm.com>

On Mon, Mar 02, 2020 at 02:23:26PM +0000, Ionela Voinescu wrote:
> On Friday 28 Feb 2020 at 10:32:34 (+0000), Catalin Marinas wrote:
> > On Wed, Feb 26, 2020 at 01:29:41PM +0000, Ionela Voinescu wrote:
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index dbc22d684627..49f0c436928f 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -318,6 +318,15 @@
> > >  			Format: <a>,<b>
> > >  			See also Documentation/input/joydev/joystick.rst
> > >  
> > > +	amu=		[ARM64]
> > > +			Enables or disables detection, enablement and access to
> > > +			counter registers of the Activity Monitors Unit (AMU).
> > > +			Format: amu=[0/1/on/off/y/n]
> > > +			amu=[0/off/n] ensures access to AMU's counter registers
> > > +				      is not attempted.
> > > +			amu=[1/on/y] (default) enables detection and access to
> > > +				     AMU's counter registers.
> > 
> > Is the only reason for this parameter to be able to disable the feature
> > if the firmware doesn't support it? According to the Kconfig entry, you
> > may see weird behaviour, firmware lock-up. Is the user supposed to try
> > again with amu=0?
> > 
> > I'm not particularly fond of adding kernel parameters to work around
> > broken firmware. We have other architecture features (e.g. PtrAuth) that
> > need enabling at EL3 but we don't have such parameters. If it's likely
> > that we hit this issue in practice, I'd rather have the firmware
> > describing the presence of AMU via some DT entry. But I'd rather not
> > bother at all, just get the vendors to update their firmware.
> 
> The firmware is supposed to do three actions for the kernel to be able
> to use the counters: enable access to EL2/EL1, enable the counters and
> save/restore the counters before/after core-off.
[...]
> Therefore, the amu kernel parameter is not only there if the firmware
> does not support AMU, but it's also there if the firmware support is
> broken/improper. The kernel parameter was added at Suzuki's
> recommendation to be able to bypass its use in single kernels that are
> meant to run on multiple platforms.

Single kernel images are supposed to run on multiple platforms while
using the same command line arguments.

There are many other ways firmware can screw up but I'm not keen on
working on such assumption and preemptively adding options to ignore CPU
features.

> I also believe this is nice to have even for platforms that properly
> support AMU, but they might not want the use of the feature in the
> kernel.

Are there any downsides to this feature? If you want it for testing
purposes, i.e. different scheduler behaviour, fine by me but I'd change
the text in the Kconfig to not even imply that firmware is allowed to be
broken as we have a workaround in the kernel.

> > If we drop this parameter, patch 1 would need to change. Otherwise the
> > patches look fine.
> 
> This being said, I agree this was added as a 'just in case' and not as
> support for a likely scenario, therefore, I don't fully disagree to drop
> it for now.

If you need it for testing different scheduler behaviours, maybe
big.LITTLE where AMU is only supported on some CPUs, than keep it. If
it's only on the assumption that the firmware may be broken, please
remove it.

Thanks.

-- 
Catalin

  reply	other threads:[~2020-03-03 16:58 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-26 13:29 [PATCH v5 0/7] arm64: ARMv8.4 Activity Monitors support Ionela Voinescu
2020-02-26 13:29 ` [PATCH v5 1/7] arm64: add support for the AMU extension v1 Ionela Voinescu
2020-02-28 10:32   ` Catalin Marinas
2020-03-02 14:23     ` Ionela Voinescu
2020-03-03 16:58       ` Catalin Marinas [this message]
2020-03-04  0:24         ` Ionela Voinescu
2020-02-26 13:29 ` [PATCH v5 2/7] arm64: trap to EL1 accesses to AMU counters from EL0 Ionela Voinescu
2020-02-28 16:44   ` James Morse
2020-03-04  0:29     ` Ionela Voinescu
2020-02-26 13:29 ` [PATCH v5 3/7] arm64/kvm: disable access to AMU registers from kvm guests Ionela Voinescu
2020-02-27 19:58   ` Marc Zyngier
2020-03-02 14:32     ` Ionela Voinescu
2020-03-09 14:25     ` Ionela Voinescu
2020-03-09 14:57       ` Marc Zyngier
2020-02-26 13:29 ` [PATCH v5 4/7] Documentation: arm64: document support for the AMU extension Ionela Voinescu
2020-02-26 13:29 ` [PATCH v5 5/7] cpufreq: add function to get the hardware max frequency Ionela Voinescu
2020-02-26 13:29 ` [PATCH v5 6/7] arm64: use activity monitors for frequency invariance Ionela Voinescu
2020-02-27  8:43   ` Lukasz Luba
2020-02-26 13:29 ` [PATCH v5 7/7] clocksource/drivers/arm_arch_timer: validate arch_timer_rate Ionela Voinescu
2020-02-26 18:24 ` [PATCH v5 0/7] arm64: ARMv8.4 Activity Monitors support 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=20200303165845.GF823373@arrakis.emea.arm.com \
    --to=catalin.marinas@arm.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=ionela.voinescu@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lukasz.luba@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=pkondeti@codeaurora.org \
    --cc=rjw@rjwysocki.net \
    --cc=sudeep.holla@arm.com \
    --cc=suzuki.poulose@arm.com \
    --cc=valentin.schneider@arm.com \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --cc=will@kernel.org \
    /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).