From: Ionela Voinescu <email@example.com> To: Valentin Schneider <firstname.lastname@example.org> 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, firstname.lastname@example.org, Jonathan Corbet <email@example.com> Subject: Re: [PATCH v2 4/6] Documentation: arm64: document support for the AMU extension Date: Tue, 28 Jan 2020 16:53:35 +0000 [thread overview] Message-ID: <20200128165325.GA16417@arm.com> (raw) In-Reply-To: <firstname.lastname@example.org> On Monday 27 Jan 2020 at 16:47:29 (+0000), Valentin Schneider wrote: > On 18/12/2019 18:26, Ionela Voinescu wrote: > > +Basic support > > +------------- > > + > > +The kernel can safely run a mix of CPUs with and without support for the > > +activity monitors extension. Therefore, when CONFIG_ARM64_AMU_EXTN is > > +selected we unconditionally enable the capability to allow any late CPU > > +(secondary or hotplugged) to detect and use the feature. > > + > > +When the feature is detected on a CPU, a per-CPU variable (amu_feat) is > > +set, but this does not guarantee the correct functionality of the > > +counters, only the presence of the extension. > > + > > +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. > > Just to make sure I understand - if AMUs are physically present but not > enabled by FW, we'll still > - see them as implemented in ID_AA64PFR0_EL1.AMU Yes, this feature register only shows the physical presence on the unit in hardware. > - see some counters as available with e.g. AMCGCR_ELO.CG0NC > 0 > Yes, the same as above, this only shows their physical presence. For AMUv1 - AMCGCR_ELO.CG0NC: the value of this field is set to 4. AMCGCR_ELO.CG1NC will show the number of auxiliary counters implemented in hardware. > But reading some AMEVCNTR<g><n> will return 0? Or you won't be able to access them at all. Lacking firmware support accesses to AMU registers could be trapped in EL3. If access for EL1 and EL2 is enabled from EL3, it's still possible that the counters themselves are not enabled - that means they are not enabled to count the events they are designed to be counting. That's why in this case the event counter register could read 0. But if we read 0, it does not necessarily mean that the counter is disabled. It could also mean that the events is meant to count did not happen yet. > > > + - 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. > > Yikes > > > 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. > > + > > I haven't seen something that would try to catch this on the kernel side. > Can we try to detect that (e.g. at least one counter returns > 0) in > cpu_amu_enable() and thus not write to the CPU-local 'amu_feat'? > I'm reluctant to do this especially given that platforms might choose to keep some counters disabled while enabling some counters that might not have counted any events by the time we reach cpu_enable. We would end up mistakenly disabling the feature. I would rather leave the validation of the counters to be done at the location and for the purpose of their use: see patch 6/6 - the use of counters for frequency invariance. > While we're on the topic of detecting broken stuff, what if some CPUs > implement some auxiliary counters that some others don't? > I think it should be up to the user of that counter to decide if the usecase is at CPU level or system level. My intention of this base support was to keep it simple and allow users of some counters to decide on their own how to validate and make use of either architected or auxiliary counters. For example, in the case of frequency invariance, given a platform that does not support cpufreq based invariance, I would validate all CPUs for the use of AMU core and constant counters. If it happens that some CPUs do not support those counters or they are not enabled, we'd have to disable frequency invariance at system level. For some other scenarios only partial support is needed - only a subset of CPUs need to support the counters for their use to be feasible. But I believe only the user of the counters can decide, whether this is happening in architecture code, driver code, generic code. > > +The fixed counters of AMUv1 are accessible though the following system > > +register definitions: > > + - SYS_AMEVCNTR0_CORE_EL0 > > + - SYS_AMEVCNTR0_CONST_EL0 > > + - SYS_AMEVCNTR0_INST_RET_EL0 > > + - SYS_AMEVCNTR0_MEM_STALL_EL0 > > + > > +Auxiliary platform specific counters can be accessed using > > +SYS_AMEVCNTR1_EL0(n), where n is a value between 0 and 15. > > + > > +Details can be found in: arch/arm64/include/asm/sysreg.h. > > + > > diff --git a/Documentation/arm64/booting.rst b/Documentation/arm64/booting.rst > > index 5d78a6f5b0ae..a3f1a47b6f1c 100644 > > --- a/Documentation/arm64/booting.rst > > +++ b/Documentation/arm64/booting.rst > > @@ -248,6 +248,20 @@ Before jumping into the kernel, the following conditions must be met: > > - HCR_EL2.APK (bit 40) must be initialised to 0b1 > > - HCR_EL2.API (bit 41) must be initialised to 0b1 > > > > + For CPUs with Activity Monitors Unit v1 (AMUv1) extension present: > > + - If EL3 is present: > > + CPTR_EL3.TAM (bit 30) must be initialised to 0b0 > > + CPTR_EL2.TAM (bit 30) must be initialised to 0b0 > > + AMCNTENSET0_EL0 must be initialised to 0b1111 > > Nit: Or be a superset of the above, right? AIUI v1 only mandates the lower > 4 bits to be set. Probably doesn't matter that much... > Right! This is more of a guideline: it can be a subset as well, if platforms don't want some counters enabled. It can set the lower 4 bits for enablement of all 4 architecture counters for v1, or more for future versions with more architected counters. Thanks, Ionela.
next prev parent reply other threads:[~2020-01-28 16:53 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 [this message] 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 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=20200128165325.GA16417@arm.com \ --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 \ --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).