From: Ionela Voinescu <firstname.lastname@example.org> To: Suzuki K Poulose <Suzuki.Poulose@arm.com> Cc: email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, Jonathan Corbet <firstname.lastname@example.org> 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.
next prev parent 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 \ --email@example.com \ --cc=Suzuki.Poulose@arm.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --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).