CoreSight August 2023

coresight@lists.linaro.org

12 participants
71 discussions

Re: [RFC PATCH 1/3] arm64: KVM: Add support for exclude_guest and exclude_host for ETM

by James Clark

On 08/08/2023 09:27, Marc Zyngier wrote: > On Fri, 04 Aug 2023 11:13:11 +0100, > James Clark <james.clark(a)arm.com> wrote: >> >> Add an interface for the Coresight driver to use to set the current >> exclude settings for the current CPU. This will be used to configure >> TRFCR_EL1. > > Can you start by stating the problem? There is *some* rationale in the > cover letter, but not enough to get the full picture. Specially if you > haven't looked at the trace subsystem in the past... 7 years or so. > Yeah I will expand on it a bit. >> >> The settings must be copied to the vCPU before each run in the same >> way that PMU events are because the per-cpu struct isn't accessible in >> protected mode. > > I'm pretty sure that for protected guests, we'd like to disable > tracing altogether (debug mode excepted). > Do you mean disable the ability to trace guests from the host? Or to disable generation of any trace altogether when in the guest, even if the guest started the session? Currently the ETM driver isn't exposed to guests, so it's not possible for the guest to start a session. That leaves only the host being able to trace the guest. If you think that shouldn't happen outside of debug mode then maybe we need another change to prevent that. Another issue is that this only works with Feat_TRF, without that we'd have to do something else again to prevent protected guests from being traced, as currently there is nothing to stop it. I'm not sure about debug mode though? Is that a kernel config or some kvm option? I suppose this could be disabled for users by not loading the Coresight driver, but there's currently nothing a guest can do to stop its self from being traced. >> >> This is only needed for nVHE, otherwise it works automatically with > > How about hVHE, which uses VHE at EL2 only? Doesn't it require the > same treatment? > Yes it sounds like it would. Does it use the same code as nVHE in arch/arm64/kvm/hyp/nvhe? I think it might already work in that case, but I need to read a bit about hVHE. >> TRFCR_EL{1,2}. Unfortunately it can't be gated on CONFIG_CORESIGHT >> because Coresight can be built as a module. It can however be gated on >> CONFIG_PERF_EVENTS because that is required by Coresight. > > Why does it need to be gated *at all*? We need this for the PMU > because of the way we call into the perf subsystem, but I don't see > anything like that here. In general, conditional compilation sucks, > and I'd like to avoid it as much as possible. > No real reason really, I was probably trying to minimise the impact because it added a few bytes here and there, but it's tiny. If you're ok with it I can quite easily remove the conditional compilation and it should simplify it a bit. >> >> Signed-off-by: James Clark <james.clark(a)arm.com> >> --- >> arch/arm64/include/asm/kvm_host.h | 10 ++++++- >> arch/arm64/kvm/Makefile | 1 + >> arch/arm64/kvm/arm.c | 1 + >> arch/arm64/kvm/etm.c | 48 +++++++++++++++++++++++++++++++ >> include/kvm/etm.h | 43 +++++++++++++++++++++++++++ >> 5 files changed, 102 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm64/kvm/etm.c >> create mode 100644 include/kvm/etm.h >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index d7b1403a3fb2..f33262217c84 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -35,6 +35,7 @@ >> #include <kvm/arm_vgic.h> >> #include <kvm/arm_arch_timer.h> >> #include <kvm/arm_pmu.h> >> +#include <kvm/etm.h> >> >> #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS >> >> @@ -500,7 +501,7 @@ struct kvm_vcpu_arch { >> u8 cflags; >> >> /* Input flags to the hypervisor code, potentially cleared after use */ >> - u8 iflags; >> + u16 iflags; > > If you make the iflags bigger, what ripple effect does it have on the > alignment of the other data structures? Consider reordering things if > it helps filling holes. > I think you might be right. Possibly flipping iflags and sflags will be enough but will have to check the exact alignment up to that point. >> >> /* State flags for kernel bookkeeping, unused by the hypervisor code */ >> u8 sflags; >> @@ -541,6 +542,9 @@ struct kvm_vcpu_arch { >> u64 pmscr_el1; >> /* Self-hosted trace */ >> u64 trfcr_el1; >> + /* exclude_guest settings for nVHE */ >> + struct kvm_etm_event etm_event; >> + > > Spurious blank line. More importantly, how is that related to the > trfcr_el1 field just above? > It's not really related, it's just the same place that the code that runs at a similar time and does a similar thing has its data stored. Something that you mentioned in another comment might be relevant here. If I do it only in terms of registers that are saved and restored then I don't need to save the etm event settings and the low level code doesn't need to know anything about it. >> } host_debug_state; >> >> /* VGIC state */ >> @@ -713,6 +717,8 @@ struct kvm_vcpu_arch { >> #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6)) >> /* vcpu running in HYP context */ >> #define VCPU_HYP_CONTEXT __vcpu_single_flag(iflags, BIT(7)) >> +/* Save TRFCR and apply exclude_guest rules */ >> +#define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(8)) >> >> /* SVE enabled for host EL0 */ >> #define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0)) >> @@ -1096,6 +1102,8 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu); >> void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr); >> void kvm_clr_pmu_events(u32 clr); >> bool kvm_set_pmuserenr(u64 val); >> +void kvm_set_etm_events(struct perf_event_attr *attr); >> +void kvm_clr_etm_events(void); >> #else >> static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {} >> static inline void kvm_clr_pmu_events(u32 clr) {} >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile >> index c0c050e53157..0faff57423c4 100644 >> --- a/arch/arm64/kvm/Makefile >> +++ b/arch/arm64/kvm/Makefile >> @@ -23,6 +23,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \ >> vgic/vgic-its.o vgic/vgic-debug.o >> >> kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o pmu.o >> +kvm-$(CONFIG_PERF_EVENTS) += etm.o >> >> always-y := hyp_constants.h hyp-constants.s >> >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >> index b1a9d47fb2f3..7bd5975328a3 100644 >> --- a/arch/arm64/kvm/arm.c >> +++ b/arch/arm64/kvm/arm.c >> @@ -952,6 +952,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >> kvm_vgic_flush_hwstate(vcpu); >> >> kvm_pmu_update_vcpu_events(vcpu); >> + kvm_etm_update_vcpu_events(vcpu); >> >> /* >> * Ensure we set mode to IN_GUEST_MODE after we disable >> diff --git a/arch/arm64/kvm/etm.c b/arch/arm64/kvm/etm.c >> new file mode 100644 >> index 000000000000..359c37745de2 >> --- /dev/null >> +++ b/arch/arm64/kvm/etm.c >> @@ -0,0 +1,48 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> + >> +#include <linux/kvm_host.h> >> + >> +#include <kvm/etm.h> >> + >> +static DEFINE_PER_CPU(struct kvm_etm_event, kvm_etm_events); >> + >> +struct kvm_etm_event *kvm_get_etm_event(void) >> +{ >> + return this_cpu_ptr(&kvm_etm_events); >> +} >> + >> +void kvm_etm_set_events(struct perf_event_attr *attr) >> +{ >> + struct kvm_etm_event *etm_event; >> + >> + /* >> + * Exclude guest option only requires extra work with nVHE. >> + * Otherwise it works automatically with TRFCR_EL{1,2} >> + */ >> + if (has_vhe()) >> + return; >> + >> + etm_event = kvm_get_etm_event(); >> + >> + etm_event->exclude_guest = attr->exclude_guest; >> + etm_event->exclude_host = attr->exclude_host; >> + etm_event->exclude_kernel = attr->exclude_kernel; >> + etm_event->exclude_user = attr->exclude_user; >> +} >> +EXPORT_SYMBOL_GPL(kvm_etm_set_events); >> + >> +void kvm_etm_clr_events(void) >> +{ >> + struct kvm_etm_event *etm_event; >> + >> + if (has_vhe()) >> + return; >> + >> + etm_event = kvm_get_etm_event(); >> + >> + etm_event->exclude_guest = false; >> + etm_event->exclude_host = false; >> + etm_event->exclude_kernel = false; >> + etm_event->exclude_user = false; >> +} >> +EXPORT_SYMBOL_GPL(kvm_etm_clr_events); > > Does it really need its own compilation unit if we were to build it at > all times? > I can probably throw it in arch/arm64/kvm/debug.c. I think I did that originally but it had the conditional stuff around it so I made the new file instead. But yeah if I remove the conditional stuff it's probably cleaner to put somewhere else. >> diff --git a/include/kvm/etm.h b/include/kvm/etm.h >> new file mode 100644 >> index 000000000000..95c4809fa2b0 >> --- /dev/null >> +++ b/include/kvm/etm.h >> @@ -0,0 +1,43 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> + >> +#ifndef __KVM_DEBUG_H >> +#define __KVM_DEBUG_H >> + >> +struct perf_event_attr; >> +struct kvm_vcpu; >> + >> +#if IS_ENABLED(CONFIG_KVM) && IS_ENABLED(CONFIG_PERF_EVENTS) >> + >> +struct kvm_etm_event { >> + bool exclude_host; >> + bool exclude_guest; >> + bool exclude_kernel; >> + bool exclude_user; >> +}; >> + >> +struct kvm_etm_event *kvm_get_etm_event(void); >> +void kvm_etm_clr_events(void); >> +void kvm_etm_set_events(struct perf_event_attr *attr); >> + >> +/* >> + * Updates the vcpu's view of the etm events for this cpu. Must be >> + * called before every vcpu run after disabling interrupts, to ensure >> + * that an interrupt cannot fire and update the structure. >> + */ >> +#define kvm_etm_update_vcpu_events(vcpu) \ >> + do { \ >> + if (!has_vhe() && vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) \ >> + vcpu->arch.host_debug_state.etm_event = *kvm_get_etm_event(); \ >> + } while (0) >> + > > Why is it a macro and not a function, which would avoid exposing > kvm_get_etm_event? > I think I saw that kvm_pmu_update_vcpu_events() was done that way, and I couldn't inline it without some circular header issue so I made it a macro. But yes a non inlined function works fine and then kvm_get_etm_event can be hidden so I'll do that. > Thanks, > > M. > Thanks for the review. James

1 year, 4 months

Re: [PATCH v4 5/6] perf vendor events arm64: Update stall_slot workaround for N2 r0p3

by James Clark

On 08/08/2023 11:18, John Garry wrote: > On 07/08/2023 15:20, James Clark wrote: > > Hi James, > > Thanks for the effort in doing this. > >> N2 r0p3 doesn't require the workaround [1], so gating on (#slots - 5) no >> longer works because all N2s have 5 slots. Add a new expression builtin >> that identifies the need for the workaround correctly. >> >> [1]: >> https://urldefense.com/v3/__https://gitlab.arm.com/telemetry-solution/telem… >> Signed-off-by: James Clark <james.clark(a)arm.com> >> --- >> tools/perf/arch/arm64/util/pmu.c | 21 +++++++++++++++++++ >> .../arm64/arm/neoverse-n2-v2/metrics.json | 8 +++---- >> tools/perf/util/expr.c | 4 ++++ >> tools/perf/util/pmu.c | 6 ++++++ >> tools/perf/util/pmu.h | 1 + >> 5 files changed, 36 insertions(+), 4 deletions(-) >> >> diff --git a/tools/perf/arch/arm64/util/pmu.c >> b/tools/perf/arch/arm64/util/pmu.c >> index 561de0cb6b95..30e2385a83cf 100644 >> --- a/tools/perf/arch/arm64/util/pmu.c >> +++ b/tools/perf/arch/arm64/util/pmu.c >> @@ -2,6 +2,7 @@ >> #include <internal/cpumap.h> >> #include "../../../util/cpumap.h" >> +#include "../../../util/header.h" >> #include "../../../util/pmu.h" >> #include "../../../util/pmus.h" >> #include <api/fs/fs.h> >> @@ -62,3 +63,23 @@ double perf_pmu__cpu_slots_per_cycle(void) >> return slots ? (double)slots : NAN; >> } >> + >> +double perf_pmu__no_stall_errata(void) > > While I like the approach of encoding the per-CPU support in the metric > expression, I find that literal "no stall errata" is vague and could > apply to any "stall errata" for any SoC for any architecture. > > If we were going to do it this way, then we would need a more specific > name, like perf_pmu__no_stall_errata_arm64_errata123456(), but that > should not be in the core perf code. > > Could we instead add a function to check cpuid and have something like > this in the JSON: > > "MetricExpr": "(op_retired / op_spec) * (1 - (stall_slot if > (cpuid_less_than(410fd493)) else (stall_slot - cpu_cycles)) / (#slots * > cpu_cycles))" > > I'm currently figuring out how cpuid_less_than() would be implemented > (I'm no great python wrangler), but it would be along the lines of what > Ian added for "has_event" in > https://lore.kernel.org/linux-perf-users/20230623151016.4193660-1-irogers@g… > > Thanks, > John Yeah it looks like it could be done that way. Also, the way I added it, it doesn't have access to the PMU type, it just does a generic pmu__find_core_pmu() so won't work very well for heterogeneous systems. If we're going to do a deeper modification of the expression parser like with has_event() it might be possible to pass in the actual CPU ID that the metric is running on which would be better. I'll have a look. James

1 year, 4 months

Re: [PATCH v4 4/6] perf vendor events arm64: Update scale units and descriptions of common topdown metrics

by James Clark

On 08/08/2023 09:46, John Garry wrote: > On 07/08/2023 15:20, James Clark wrote: >> Metrics will be published here [1] going forwards, but they have >> slightly different scale units. To allow autogenerated metrics to be >> added more easily, update the scale units to match. >> >> The more detailed descriptions have also been taken and added to the >> common file. > > It's unfortunate that we can't have a concise description - like which > we have now - and a full description. > >> > > Anyway, > Reviewed-by: John Garry <john.g.garry(a)oracle.com> > Yes unfortunately in the source data the metric short description ("title") pretty much just matches the metric name but without underscores. For that reason we chose not to use it for Perf's short description otherwise the list command showed a lot of duplication. "frontend_bound": { "title": "Frontend Bound", "description": "This metric is the percentage of... The PMU events on the other hand do have a short description in the title field rather than just repeating the name: ITLB_WALK": { "code": "0x0035", "title": "Instruction TLB access with at least one translation table walk", "description": "Counts instruction memory translation table walks caused by a miss in the L2... I can raise this internally as it does seem to be a bit of an inconsistency. James

1 year, 4 months

Re: [PATCH V3 1/4] arm_pmu: acpi: Refactor arm_spe_acpi_register_device()

by Anshuman Khandual

On 8/8/23 18:51, Will Deacon wrote: > On Mon, Aug 07, 2023 at 11:03:40AM +0530, Anshuman Khandual wrote: >> On 8/4/23 22:09, Will Deacon wrote: >>> On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote: >>>> On 8/3/23 11:26, Anshuman Khandual wrote: >>>>> + /* >>>>> + * Sanity check all the GICC tables for the same interrupt >>>>> + * number. For now, only support homogeneous ACPI machines. >>>>> + */ >>>>> + for_each_possible_cpu(cpu) { >>>>> + struct acpi_madt_generic_interrupt *gicc; >>>>> + >>>>> + gicc = acpi_cpu_get_madt_gicc(cpu); >>>>> + if (gicc->header.length < len) >>>>> + return gsi ? -ENXIO : 0; >>>>> + >>>>> + this_gsi = parse_gsi(gicc); >>>>> + if (!this_gsi) >>>>> + return gsi ? -ENXIO : 0; >>>> >>>> Moved parse_gsi() return code checking to its original place just to >>>> make it similar in semantics to existing 'gicc->header.length check'. >>>> If 'gsi' is valid i.e atleast a single cpu has been probed, return >>>> -ENXIO indicating mismatch, otherwise just return 0. >>> >>> Wouldn't that still be the case without the check in this hunk? We'd run >>> into the homogeneous check and return -ENXIO from there, no? >> Although the return code will be the same i.e -ENXIO, but not for the same reason. >> >> this_gsi = parse_gsi(gicc); >> if (!this_gsi) >> return gsi ? -ENXIO : 0; >> >> This returns 0 when IRQ could not be parsed for the first cpu, but returns -ENXIO >> for subsequent cpus. Although return code -ENXIO here still indicates IRQ parsing >> to have failed. >> >> } else if (hetid != this_hetid || gsi != this_gsi) { >> pr_warn("ACPI: %s: must be homogeneous\n", pdev->name); >> return -ENXIO; >> } >> >> This returns -ENXIO when there is a IRQ mismatch. But if the above check is not >> there, -ENXIO return code here could not be classified into IRQ parse problem or >> mismatch without looking into the IRQ value. > > Sorry, but I don't understand your point here. If any of this fails, there's > going to be some debugging needed to look at the ACPI tables; the only > difference with my suggestion is that you'll get a message indicating that > the devices aren't homogeneous, which I think is helpful. I dont have strong opinion either way. Hence will move 'this_gsi' check inside the !gsi conditional check like you had suggested earlier.

1 year, 4 months

[PATCH] coresight: etm: Make cycle count threshold user configurable

by Anshuman Khandual

Cycle counting is enabled, when requested and supported but with a default threshold value ETM_CYC_THRESHOLD_DEFAULT i.e 0x100 getting into TRCCCCTLR, representing the minimum interval between cycle count trace packets. This makes cycle threshold user configurable, from the user space via perf event attributes. Although it falls back using ETM_CYC_THRESHOLD_DEFAULT, in case no explicit request. As expected it creates a sysfs file as well. /sys/bus/event_source/devices/cs_etm/format/cc_threshold New 'cc_threshold' uses 'event->attr.config3' as no more space is available in 'event->attr.config1' or 'event->attr.config2'. Cc: Suzuki K Poulose <suzuki.poulose(a)arm.com> Cc: Mike Leach <mike.leach(a)linaro.org> Cc: James Clark <james.clark(a)arm.com> Cc: Leo Yan <leo.yan(a)linaro.org> Cc: coresight(a)lists.linaro.org Cc: linux-arm-kernel(a)lists.infradead.org Cc: linux-doc(a)vger.kernel.org Cc: linux-kernel(a)vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual(a)arm.com> --- Changes in V2: - s/treshhold/threshhold Changes in V1: https://lore.kernel.org/all/20230804044720.1478900-1-anshuman.khandual@arm.… Documentation/trace/coresight/coresight.rst | 2 ++ drivers/hwtracing/coresight/coresight-etm-perf.c | 2 ++ drivers/hwtracing/coresight/coresight-etm4x-core.c | 12 ++++++++++-- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst index 4a71ea6cb390..a698b07206b5 100644 --- a/Documentation/trace/coresight/coresight.rst +++ b/Documentation/trace/coresight/coresight.rst @@ -624,6 +624,8 @@ They are also listed in the folder /sys/bus/event_source/devices/cs_etm/format/ * - timestamp - Session local version of the system wide setting: :ref:`ETMv4_MODE_TIMESTAMP <coresight-timestamp>` + * - cc_threshold + - Cycle count threshold value How to use the STM module ------------------------- diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 5ca6278baff4..09f75dffae60 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -68,6 +68,7 @@ PMU_FORMAT_ATTR(preset, "config:0-3"); PMU_FORMAT_ATTR(sinkid, "config2:0-31"); /* config ID - set if a system configuration is selected */ PMU_FORMAT_ATTR(configid, "config2:32-63"); +PMU_FORMAT_ATTR(cc_threshold, "config3:0-11"); /* @@ -101,6 +102,7 @@ static struct attribute *etm_config_formats_attr[] = { &format_attr_preset.attr, &format_attr_configid.attr, &format_attr_branch_broadcast.attr, + &format_attr_cc_threshold.attr, NULL, }; diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 9d186af81ea0..a353c0784bab 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -644,7 +644,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev, struct etmv4_config *config = &drvdata->config; struct perf_event_attr *attr = &event->attr; unsigned long cfg_hash; - int preset; + int preset, cc_threshold; /* Clear configuration from previous run */ memset(config, 0, sizeof(struct etmv4_config)); @@ -667,7 +667,15 @@ static int etm4_parse_event_config(struct coresight_device *csdev, if (attr->config & BIT(ETM_OPT_CYCACC)) { config->cfg |= TRCCONFIGR_CCI; /* TRM: Must program this for cycacc to work */ - config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT; + cc_threshold = attr->config3 & ETM_CYC_THRESHOLD_MASK; + if (cc_threshold) { + if (cc_threshold < drvdata->ccitmin) + config->ccctlr = drvdata->ccitmin; + else + config->ccctlr = cc_threshold; + } else { + config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT; + } } if (attr->config & BIT(ETM_OPT_TS)) { /* -- 2.25.1

1 year, 4 months

[PATCH v4 0/6] perf vendor events arm64: Update N2 and V2 metrics and events using Arm telemetry repo

by James Clark

This is a completely new approach from V3 [1], although the metrics and event descriptions are autogenerated, the topdown metrics have been manually edited to use #no_stall_errata. This removes the need to duplicate the whole set of JSONs when only the topdown metrics are different between N2 and V2. The CPU ID comparison function still needs to change so that the new literal can compare on versions, but now no change is needed to mapfile or the PMU event generation code because we still only support one set of JSONs per CPU. [1]: https://lore.kernel.org/lkml/20230711100218.1651995-1-james.clark@arm.com/ ------ Changes since v3: * Instead of duplicating all the metrics, add a new expression literal that can be used to share the same metrics between N2 and V2 * Move tests to arch/arm64/tests * Remove changes from jevents.py and mapfile.csv Changes since v2: * version -> variant in second commit message * Add a bit more detail about version matching in the second commit message * Update the comments in pmu-events/arch/arm64/mapfile.csv to say that variant and revision fields are now used * Increase the CC list Changes since v1: * Split last change into two so it doesn't hit the mailing list size limit James Clark (6): perf: cs-etm: Don't duplicate FIELD_GET() perf arm64: Allow version comparisons of CPU IDs perf test: Add a test for the new Arm CPU ID comparison behavior perf vendor events arm64: Update scale units and descriptions of common topdown metrics perf vendor events arm64: Update stall_slot workaround for N2 r0p3 perf vendor events arm64: Update N2 and V2 metrics and events using Arm telemetry repo tools/perf/arch/arm64/include/arch-tests.h | 3 + tools/perf/arch/arm64/tests/Build | 1 + tools/perf/arch/arm64/tests/arch-tests.c | 4 + tools/perf/arch/arm64/tests/cpuid-match.c | 38 ++ tools/perf/arch/arm64/util/header.c | 64 ++- tools/perf/arch/arm64/util/pmu.c | 21 + .../arch/arm64/arm/neoverse-n2-v2/branch.json | 8 - .../arch/arm64/arm/neoverse-n2-v2/bus.json | 18 +- .../arch/arm64/arm/neoverse-n2-v2/cache.json | 155 -------- .../arm64/arm/neoverse-n2-v2/exception.json | 45 ++- .../arm/neoverse-n2-v2/fp_operation.json | 22 ++ .../arm64/arm/neoverse-n2-v2/general.json | 10 + .../arm64/arm/neoverse-n2-v2/instruction.json | 143 ------- .../arm64/arm/neoverse-n2-v2/l1d_cache.json | 54 +++ .../arm64/arm/neoverse-n2-v2/l1i_cache.json | 14 + .../arm64/arm/neoverse-n2-v2/l2_cache.json | 50 +++ .../arm64/arm/neoverse-n2-v2/l3_cache.json | 22 ++ .../arm64/arm/neoverse-n2-v2/ll_cache.json | 10 + .../arch/arm64/arm/neoverse-n2-v2/memory.json | 39 +- .../arm64/arm/neoverse-n2-v2/metrics.json | 365 ++++++++++-------- .../arm64/arm/neoverse-n2-v2/pipeline.json | 23 -- .../arm64/arm/neoverse-n2-v2/retired.json | 30 ++ .../arch/arm64/arm/neoverse-n2-v2/spe.json | 12 +- .../arm/neoverse-n2-v2/spec_operation.json | 110 ++++++ .../arch/arm64/arm/neoverse-n2-v2/stall.json | 30 ++ .../arch/arm64/arm/neoverse-n2-v2/sve.json | 50 +++ .../arch/arm64/arm/neoverse-n2-v2/tlb.json | 66 ++++ .../arch/arm64/arm/neoverse-n2-v2/trace.json | 27 +- tools/perf/pmu-events/arch/arm64/sbsa.json | 24 +- tools/perf/util/cs-etm.c | 14 +- tools/perf/util/expr.c | 4 + tools/perf/util/pmu.c | 6 + tools/perf/util/pmu.h | 1 + 33 files changed, 894 insertions(+), 589 deletions(-) create mode 100644 tools/perf/arch/arm64/tests/cpuid-match.c delete mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/branch.json delete mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/cache.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/fp_operation.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/general.json delete mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/instruction.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/l1d_cache.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/l1i_cache.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/l2_cache.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/l3_cache.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/ll_cache.json delete mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/pipeline.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/retired.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/spec_operation.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/stall.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/sve.json create mode 100644 tools/perf/pmu-events/arch/arm64/arm/neoverse-n2-v2/tlb.json -- 2.34.1

1 year, 4 months

Re: [bug report] coresight: etm4x: Change etm4_platform_driver driver for MMIO devices

by Anshuman Khandual

Hello Dan, On 7/29/23 12:50, Dan Carpenter wrote: > Hello Anshuman Khandual, > > The patch 73d779a03a76: "coresight: etm4x: Change > etm4_platform_driver driver for MMIO devices" from Jul 10, 2023 > (linux-next), leads to the following Smatch static checker warning: > > drivers/hwtracing/coresight/coresight-etm4x-core.c:2272 etm4_remove_platform_dev() > error: we previously assumed 'drvdata' could be null (see line 2268) > > drivers/hwtracing/coresight/coresight-etm4x-core.c > 2264 static int __exit etm4_remove_platform_dev(struct platform_device *pdev) > 2265 { > 2266 struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev); > 2267 > 2268 if (drvdata) > > This check can probably be removed, right? etm4_remove_dev(drvdata) cannot be called if drvdata is NULL. But there might be an implicit assumption that 'drvdata' could never be NULL in this path. > > 2269 etm4_remove_dev(drvdata); > 2270 pm_runtime_disable(&pdev->dev); > 2271 > --> 2272 if (drvdata->pclk) > > Not checked here We could either have the following change which ensures that 'drvdata' is checked again before the clock is handled. diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 703b6fcbb6a5..9d186af81ea0 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -2267,11 +2267,13 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev) if (drvdata) etm4_remove_dev(drvdata); - pm_runtime_disable(&pdev->dev); - if (drvdata->pclk) - clk_put(drvdata->pclk); + pm_runtime_disable(&pdev->dev); + if (drvdata) { + if (drvdata->pclk && !IS_ERR(drvdata->pclk)) + clk_put(drvdata->pclk); + } return 0; } OR remove the first drvdata check as suggested. diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 703b6fcbb6a5..6e1ffddbd4be 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -2265,11 +2265,9 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev) { struct etmv4_drvdata *drvdata = dev_get_drvdata(&pdev->dev); - if (drvdata) - etm4_remove_dev(drvdata); + etm4_remove_dev(drvdata); pm_runtime_disable(&pdev->dev); - - if (drvdata->pclk) + if (drvdata->pclk && !IS_ERR(drvdata->pclk) clk_put(drvdata->pclk); return 0; > > 2273 clk_put(drvdata->pclk); > 2274 > 2275 return 0; > 2276 } Also adding Suzuki and James for their input. - Anshuman

1 year, 4 months

[PATCH V3 0/4] coresight: trbe: Enable ACPI based devices

by Anshuman Khandual

This series enables detection of ACPI based TRBE devices via a stand alone purpose built representative platform device. But as a pre-requisite this changes coresight_platform_data structure assignment for the TRBE device. This series is based on v6.5-rc4 kernel, is also dependent on the following EDK2 changes posted earlier by Sami. https://edk2.groups.io/g/devel/message/107239 https://edk2.groups.io/g/devel/message/107241 Changes in V3: - Changed ARMV8_TRBE_PDEV_NAME from "arm-trbe-acpi" into "arm,trbe" - Dropped local variable 'matched' - Replaced 'matched' with 'valid gsi' as being already matched once - Moved find_acpi_cpu_topology_hetero_id() outside conditional check Changes in V2: https://lore.kernel.org/all/20230801094052.750416-1-anshuman.khandual@arm.c… - Refactored arm_spe_acpi_register_device() in a separate patch - Renamed trbe_acpi_resources as trbe_resources - Renamed trbe_acpi_dev as trbe_dev Changes in V1: https://lore.kernel.org/all/20230728112733.359620-1-anshuman.khandual@arm.c… Cc: Sami Mujawar <sami.mujawar(a)arm.com> Cc: Catalin Marinas <catalin.marinas(a)arm.com> Cc: Will Deacon <will(a)kernel.org> Cc: Mark Rutland <mark.rutland(a)arm.com> Cc: Suzuki K Poulose <suzuki.poulose(a)arm.com> Cc: Mike Leach <mike.leach(a)linaro.org> Cc: Leo Yan <leo.yan(a)linaro.org> Cc: Alexander Shishkin <alexander.shishkin(a)linux.intel.com> Cc: James Clark <james.clark(a)arm.com> Cc: coresight(a)lists.linaro.org Cc: linux-arm-kernel(a)lists.infradead.org Cc: linux-kernel(a)vger.kernel.org Anshuman Khandual (4): arm_pmu: acpi: Refactor arm_spe_acpi_register_device() arm_pmu: acpi: Add a representative platform device for TRBE coresight: trbe: Add a representative coresight_platform_data for TRBE coresight: trbe: Enable ACPI based TRBE devices arch/arm64/include/asm/acpi.h | 3 + drivers/hwtracing/coresight/coresight-trbe.c | 15 +- drivers/hwtracing/coresight/coresight-trbe.h | 1 + drivers/perf/arm_pmu_acpi.c | 142 +++++++++++++------ include/linux/perf/arm_pmu.h | 1 + 5 files changed, 119 insertions(+), 43 deletions(-) -- 2.25.1

1 year, 4 months

Re: [PATCH V3 1/4] arm_pmu: acpi: Refactor arm_spe_acpi_register_device()

by Anshuman Khandual

On 8/4/23 22:09, Will Deacon wrote: > On Thu, Aug 03, 2023 at 11:43:27AM +0530, Anshuman Khandual wrote: >> >> >> On 8/3/23 11:26, Anshuman Khandual wrote: >>> + /* >>> + * Sanity check all the GICC tables for the same interrupt >>> + * number. For now, only support homogeneous ACPI machines. >>> + */ >>> + for_each_possible_cpu(cpu) { >>> + struct acpi_madt_generic_interrupt *gicc; >>> + >>> + gicc = acpi_cpu_get_madt_gicc(cpu); >>> + if (gicc->header.length < len) >>> + return gsi ? -ENXIO : 0; >>> + >>> + this_gsi = parse_gsi(gicc); >>> + if (!this_gsi) >>> + return gsi ? -ENXIO : 0; >> >> Hello Will, >> >> Moved parse_gsi() return code checking to its original place just to >> make it similar in semantics to existing 'gicc->header.length check'. >> If 'gsi' is valid i.e atleast a single cpu has been probed, return >> -ENXIO indicating mismatch, otherwise just return 0. > > Wouldn't that still be the case without the check in this hunk? We'd run > into the homogeneous check and return -ENXIO from there, no? Although the return code will be the same i.e -ENXIO, but not for the same reason. this_gsi = parse_gsi(gicc); if (!this_gsi) return gsi ? -ENXIO : 0; This returns 0 when IRQ could not be parsed for the first cpu, but returns -ENXIO for subsequent cpus. Although return code -ENXIO here still indicates IRQ parsing to have failed. } else if (hetid != this_hetid || gsi != this_gsi) { pr_warn("ACPI: %s: must be homogeneous\n", pdev->name); return -ENXIO; } This returns -ENXIO when there is a IRQ mismatch. But if the above check is not there, -ENXIO return code here could not be classified into IRQ parse problem or mismatch without looking into the IRQ value.

1 year, 4 months

Re: [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE

by Suzuki K Poulose

Hi Marc On 04/08/2023 20:09, Marc Zyngier wrote: > On Fri, 04 Aug 2023 11:13:10 +0100, > James Clark <james.clark(a)arm.com> wrote: >> >> Hi, >> >> I'm looking for help in testing this and for feedback on whether it's >> useful to anyone. Testing it requires hardware that has Feat_TRF (v8.4) >> but no TRBE. This is because TRBE usage is disabled in nVHE guests. >> >> I don't currently have any access to any hardware, and the FVP model >> can only do self hosted trace using TRBE. >> >> Currently with nVHE you would always get trace from guests, and >> filtering out isn't possible without this patchset. In comparison, with >> VHE guests, they never generate guest trace without [1]. I think the >> existence of trace rather than lack of could suggest that this change is >> less useful than [1]. Also the restricted set of hardware that it works >> on supports that too. > > It'd be nice to have some sort of feature parity, but it seems like a > vanishingly small target of users having access to an ETM sink. > >> >> Apart from compilation and checking that the exclude guest settings >> are correctly programmed on guest switch, this is untested by me. > > I'll have a look at the series, but none of my HW fits in this > description (my ARMv8.4+ boxes don't have any form of tracing). While I have your attention, we have another series that manages the trace filtering for Guests on VHE, completely within the Coresight etm4x driver here [0]. I personally think, it is good to have the guest filtering for both nVHE and VHE under the KVM control, like we do in this series. I would like your opinion on this. [0] https://lkml.kernel.org/r/20230804085219.260790-1-james.clark@arm.com Suzuki > > Thanks, > > M. >

1 year, 4 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight August 2023