CoreSight

coresight@lists.linaro.org

7 participants
2453 discussions

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

by Anshuman Khandual

On 8/11/23 15:49, Will Deacon wrote: > On Wed, Aug 09, 2023 at 01:54:55PM +0100, Suzuki K Poulose wrote: >> On 08/08/2023 14:16, Will Deacon wrote: >>> On Tue, Aug 08, 2023 at 09:48:16AM +0100, Suzuki K Poulose wrote: >>>> On 08/08/2023 09:22, Anshuman Khandual wrote: >>>>> Sanity checking all the GICC tables for same interrupt number, and ensuring >>>>> a homogeneous ACPI based machine, could be used for other platform devices >>>>> as well. Hence this refactors arm_spe_acpi_register_device() into a common >>>>> helper arm_acpi_register_pmu_device(). >>>>> >>>>> Cc: Catalin Marinas <catalin.marinas(a)arm.com> >>>>> Cc: Will Deacon <will(a)kernel.org> >>>>> Cc: Mark Rutland <mark.rutland(a)arm.com> >>>>> Cc: linux-arm-kernel(a)lists.infradead.org >>>>> Cc: linux-kernel(a)vger.kernel.org >>>>> Co-developed-by: Will Deacon <will(a)kernel.org> >>>>> Signed-off-by: Will Deacon <will(a)kernel.org> >>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual(a)arm.com> >>>>> --- >>>>> drivers/perf/arm_pmu_acpi.c | 105 ++++++++++++++++++++++-------------- >>>>> 1 file changed, 65 insertions(+), 40 deletions(-) >>>>> >>>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c >>>>> index 90815ad762eb..72454bef2a70 100644 >>>>> --- a/drivers/perf/arm_pmu_acpi.c >>>>> +++ b/drivers/perf/arm_pmu_acpi.c >>>>> + pdev->resource[0].start = irq; >>>>> + ret = platform_device_register(pdev); >>>>> + if (ret < 0) { >>>>> + pr_warn("ACPI: %s: Unable to register device\n", pdev->name); >>>>> + acpi_unregister_gsi(gsi); >>>>> + } >>>>> + return ret; >>>> >>>> A postivie return value here could confuse the caller. Also, with my comment >>>> below, we don't really need to return something from here. >>> >>> How does this return a positive value? >> >> Right now, there aren't. My point is this function returns a "return value" >> of another function. And the caller of this function doesn't >> really follow the "check" it needs. e.g.: >> >> ret = foo(); >> if (ret < 0) >> error; >> return ret; >> >> >> >> And the caller only checks for >> >> if (ret) >> error; >> >> This seems fragile. > > Yeah, the '< 0' check is weird. I'd be inclined to drop that entirely > from the helper function tbh... static int __maybe_unused arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *)) { ..... ..... ret = platform_device_register(pdev); if (ret < 0) { pr_warn("ACPI: %s: Unable to register device\n", pdev->name); acpi_unregister_gsi(gsi); } return ret; } We would still need to call acpi_unregister_gsi() in the helper itself in case previous platform_device_register() did not work correctly. AFAICS, acpi_unregister_gsi() cannot be moved to the caller. We could change the error check as 'if (ret)' which is the case in many other places in the tree. Also drop the above generic pr_warn() error message. static int __maybe_unused arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *)) { ..... ..... ret = platform_device_register(pdev); if (ret) acpi_unregister_gsi(gsi); return ret; } > >>>>> + int ret = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE, >>>>> + arm_spe_parse_gsi); >>>>> + if (ret) >>>>> pr_warn("ACPI: SPE: Unable to register device\n"); >>>> >>>> With this change, a system without SPE interrupt description always >>>> generates the above message. Is this intended ? >>> >>> If there are no irqs, why doesn't this return 0? >> >> Apologies, I missed that. >> >>> arm_acpi_register_pmu_device() should only fail if either: >>> >>> - The static resources passed in are broken >>> - The tables are not homogeneous >>> - We fail to register the interrupt >>> >>> so something is amiss. >> >> Agreed. We don't need duplicate messages about an error ? >> i.e., one in arm_acpi_register_pmu_device() and another >> one in the caller ? (Of course adding any missing error msgs). > > ... and then just print the registration failure message in the caller. But we already have such messages in respective callers. static void arm_spe_acpi_register_device(void) { int ret = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE, arm_spe_parse_gsi); if (ret) pr_warn("ACPI: SPE: Unable to register device\n"); } static void arm_trbe_acpi_register_device(void) { int ret = arm_acpi_register_pmu_device(&trbe_dev, ACPI_MADT_GICC_TRBE, arm_trbe_parse_gsi); if (ret) pr_warn("ACPI: TRBE: Unable to register device\n"); }

2 years

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

by Anshuman Khandual

On 8/11/23 16:30, Will Deacon wrote: > On Fri, Aug 11, 2023 at 03:55:43PM +0530, Anshuman Khandual wrote: >> >> >> On 8/11/23 15:42, Will Deacon wrote: >>> On Fri, Aug 11, 2023 at 02:13:42PM +0530, Anshuman Khandual wrote: >>>> On 8/8/23 13:52, 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; >>>>> + >>>>> + this_hetid = find_acpi_cpu_topology_hetero_id(cpu); >>>>> + if (!gsi) { >>>>> + hetid = this_hetid; >>>>> + gsi = this_gsi; >>>>> + } else if (hetid != this_hetid || gsi != this_gsi) { >>>>> + pr_warn("ACPI: %s: must be homogeneous\n", pdev->name); >>>>> + return -ENXIO; >>>>> + } >>>>> + } >>>> >>>> As discussed on the previous version i.e V3 thread, will move the >>>> 'this_gsi' check after parse_gsi(), inside if (!gsi) conditional >>>> block. This will treat subsequent cpu parse_gsi()'s failure as a >>>> mismatch thus triggering the pr_warn() message. >>>> >>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c >>>> index 845683ca7c64..6eae772d6298 100644 >>>> --- a/drivers/perf/arm_pmu_acpi.c >>>> +++ b/drivers/perf/arm_pmu_acpi.c >>>> @@ -98,11 +98,11 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, >>>> return gsi ? -ENXIO : 0; >>>> >>>> this_gsi = parse_gsi(gicc); >>>> - if (!this_gsi) >>>> - return gsi ? -ENXIO : 0; >>>> - >>>> this_hetid = find_acpi_cpu_topology_hetero_id(cpu); >>>> if (!gsi) { >>>> + if (!this_gsi) >>>> + return 0; >>> >>> Why do you need this hunk? >> >> Otherwise '0' gsi on all cpus would just clear the above homogeneity >> test, and end up in acpi_register_gsi() making it fail, but with the >> following warning before returning with -ENXIO. >> >> irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH); >> if (irq < 0) { >> pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi); >> return -ENXIO; >> } > > Ah gotcha, thanks. > >> Is this behaviour better than returning 0 after detecting '0' gsi in >> the first cpu to avoid the above mentioned scenario ? Although 0 gsi >> followed by non-zero ones will still end up warning about a mismatch. > > Can we move the check _after_ the loop, then? That way, we still detect > mismatches but we'll quietly return 0 if nobody has an interrupt. Sure, will fold in the following changes instead. diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c index 845683ca7c64..d7beb035345a 100644 --- a/drivers/perf/arm_pmu_acpi.c +++ b/drivers/perf/arm_pmu_acpi.c @@ -98,9 +98,6 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, return gsi ? -ENXIO : 0; this_gsi = parse_gsi(gicc); - if (!this_gsi) - return gsi ? -ENXIO : 0; - this_hetid = find_acpi_cpu_topology_hetero_id(cpu); if (!gsi) { hetid = this_hetid; @@ -111,6 +108,15 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, } } + /* + * This is a special case where no cpu on + * the system has the interrupt and which + * could not have been detected via above + * homogeneous mismatch test. + */ + if (!this_gsi) + return 0; + irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH); if (irq < 0) { pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi);

2 years

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

by Arnaldo Carvalho de Melo

Em Fri, Aug 11, 2023 at 03:53:49PM +0100, John Garry escreveu: > On 11/08/2023 15:39, 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. > > > > [1]:https://urldefense.com/v3/__https://gitlab.arm.com/telemetry-solution/te… > > > > Acked-by: Ian Rogers<irogers(a)google.com> > > Signed-off-by: James Clark<james.clark(a)arm.com> > > Reviewed-by: John Garry <john.g.garry(a)oracle.com> Applied as well, will wait for the others to get the review comment addressed. - Arnaldo

2 years

[PATCH v5 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 (now directly comparing on the CPUID in v5). 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 v4: * Replace the #no_stall_errata literal with a more generic function for comparing CPU IDs. This will hopefully keep configuration out of the code and inside the JSONs 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 | 18 +- .../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/pmu-events/metric.py | 17 +- tools/perf/util/cs-etm.c | 14 +- tools/perf/util/expr.c | 18 + tools/perf/util/expr.h | 1 + tools/perf/util/expr.l | 1 + tools/perf/util/expr.y | 8 +- tools/perf/util/pmu.c | 17 + tools/perf/util/pmu.h | 1 + 37 files changed, 923 insertions(+), 609 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

2 years

Re: [PATCH v5 2/6] perf arm64: Allow version comparisons of CPU IDs

by James Clark

On 14/08/2023 14:07, John Garry wrote: > On 11/08/2023 15:39, James Clark wrote: >> Currently variant and revision fields are masked out of the MIDR so >> it's not possible to compare different versions of the same CPU. >> In a later commit a workaround will be removed just for N2 r0p3, so >> enable comparisons on version. >> >> This has the side effect of changing the MIDR stored in the header of >> the perf.data file to no longer have masked version fields. > > Did you consider adding a raw version of _get_cpuid(), which returns the > full MIDR just for the purpose of caller strcmp_cpuid_str()? I did, but I thought that seeing as it would only be used in one place, and that changing the existing one didn't break anything, that it was better to not fragment the CPU ID interface. I thought it might also have repercussions for the other architectures as well. It would also mean that the MIDR that's stored in the header wouldn't have the version information, which if we're starting to do things with that could be bad. There are already callers of strcmp_cpuid_str() so it's probably best to keep it using the same get_cpuid() string. Unless there is a reason _not_ to do it? There isn't really anything that can't be done with it accepting/returning the full unmasked MIDR. If you want the old behavior, you just set the version fields to 0, which I've also used in a later patch and is already done in mapfile.csv > > I can't comment on how it will be called in relation to > strcmp_cpuid_str(), as I am unsure whether strcmp_cpuid_str() should be > just in arch arm64 code - see comment on later patch. > > It also >> affects the lookups in mapfile.csv, but as that currently only has >> zeroed version fields, it has no actual effect. The mapfile.csv >> documentation also states to zero the version fields, so unless this >> isn't done it will continue to have no effect. >> >> Signed-off-by: James Clark <james.clark(a)arm.com> >> --- >> tools/perf/arch/arm64/util/header.c | 64 ++++++++++++++++++++++------- >> 1 file changed, 50 insertions(+), 14 deletions(-) >> >> diff --git a/tools/perf/arch/arm64/util/header.c >> b/tools/perf/arch/arm64/util/header.c >> index 80b9f6287fe2..8f74e801e1ab 100644 >> --- a/tools/perf/arch/arm64/util/header.c >> +++ b/tools/perf/arch/arm64/util/header.c >> @@ -1,3 +1,6 @@ >> +#include <linux/kernel.h> >> +#include <linux/bits.h> >> +#include <linux/bitfield.h> >> #include <stdio.h> >> #include <stdlib.h> >> #include <perf/cpumap.h> >> @@ -10,14 +13,12 @@ >> #define MIDR "/regs/identification/midr_el1" >> #define MIDR_SIZE 19 >> -#define MIDR_REVISION_MASK 0xf >> -#define MIDR_VARIANT_SHIFT 20 >> -#define MIDR_VARIANT_MASK (0xf << MIDR_VARIANT_SHIFT) >> +#define MIDR_REVISION_MASK GENMASK(3, 0) >> +#define MIDR_VARIANT_MASK GENMASK(23, 20) >> static int _get_cpuid(char *buf, size_t sz, struct perf_cpu_map >> *cpus) >> { >> const char *sysfs = sysfs__mountpoint(); >> - u64 midr = 0; >> int cpu; >> if (!sysfs || sz < MIDR_SIZE) >> @@ -44,21 +45,11 @@ static int _get_cpuid(char *buf, size_t sz, struct >> perf_cpu_map *cpus) >> } >> fclose(file); >> - /* Ignore/clear Variant[23:20] and >> - * Revision[3:0] of MIDR >> - */ >> - midr = strtoul(buf, NULL, 16); >> - midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK)); >> - scnprintf(buf, MIDR_SIZE, "0x%016lx", midr); >> /* got midr break loop */ >> break; >> } >> perf_cpu_map__put(cpus); >> - >> - if (!midr) >> - return EINVAL; >> - >> return 0; >> } >> @@ -99,3 +90,48 @@ char *get_cpuid_str(struct perf_pmu *pmu) >> return buf; >> } >> + >> +/* >> + * Return 0 if idstr is a higher or equal to version of the same part as >> + * mapcpuid. >> + * >> + * Therefore, if mapcpuid has 0 for revision and variant then any >> version of >> + * idstr will match as long as it's the same CPU type. >> + */ >> +int strcmp_cpuid_str(const char *mapcpuid, const char *idstr) > > should we check implementator and other fields as a sanity check? > I'm not sure what we could check them against? There are no existing sanity checks around the MIDR stuff, it just takes whatever MIDR is in mapfile.csv and from the system. Unless there is something specific to this change that would benefit from it? Otherwise it sounds like it could be a separate additional change to what's already in Perf. >> +{ >> + u64 map_id = strtoull(mapcpuid, NULL, 16); >> + char map_id_variant = FIELD_GET(MIDR_VARIANT_MASK, map_id); >> + char map_id_revision = FIELD_GET(MIDR_REVISION_MASK, map_id); >> + u64 id = strtoull(idstr, NULL, 16); >> + char id_variant = FIELD_GET(MIDR_VARIANT_MASK, id); >> + char id_revision = FIELD_GET(MIDR_REVISION_MASK, id); >> + u64 id_fields = ~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK); >> + >> + /* Compare without version first */ >> + if ((map_id & id_fields) != (id & id_fields)) >> + return 1; >> + >> + /* >> + * ID matches, now compare version. >> + * >> + * Arm revisions (like r0p0) are compared here like two digit semver >> + * values eg. 1.3 < 2.0 < 2.1 < 2.2. The events json file with the >> + * highest matching version is used. >> + * >> + * r = high value = 'Variant' field in MIDR >> + * p = low value = 'Revision' field in MIDR >> + * >> + */ >> + if (id_variant > map_id_variant) >> + return 0; >> + >> + if (id_variant == map_id_variant && id_revision >= map_id_revision) >> + return 0; >> + >> + /* >> + * variant is less than mapfile variant or variants are the same but >> + * the revision doesn't match. Return no match. >> + */ >> + return 1; >> +} > > Thanks, > John >

2 years

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

by James Clark

On 14/08/2023 14:02, John Garry wrote: > >> try: >> parsed = ast.parse(py, mode='eval') >> except SyntaxError as e: >> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c >> index 7410a165f68b..0985a3cbc6f9 100644 >> --- a/tools/perf/util/expr.c >> +++ b/tools/perf/util/expr.c >> @@ -13,6 +13,8 @@ >> #include <util/expr-bison.h> >> #include <util/expr-flex.h> >> #include "util/hashmap.h" >> +#include "util/header.h" >> +#include "util/pmu.h" >> #include "smt.h" >> #include "tsc.h" >> #include <api/fs/fs.h> >> @@ -495,3 +497,19 @@ double expr__has_event(const struct >> expr_parse_ctx *ctx, bool compute_ids, const >> evlist__delete(tmp); >> return ret; >> } >> + >> +double expr__strcmp_cpuid_str(const struct expr_parse_ctx *ctx >> __maybe_unused, >> + bool compute_ids __maybe_unused, const char *test_id) >> +{ >> + double ret; >> + struct perf_pmu *pmu = pmu__find_core_pmu(); >> + char *cpuid = perf_pmu__getcpuid(pmu); >> + >> + if (!cpuid) >> + return NAN; >> + >> + ret = !strcmp_cpuid_str(test_id, cpuid); > > It seems that strcmp_cpuid_str() is only added in arm64 arch code - > should there be a weak version for other archs? I think there is one in tools/perf/util/header.c. I tested the build on x86 as well as arm so it should be working. > >> + >> + free(cpuid); >> + return ret; >> +} >> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h >> index 3c1e49b3e35d..c0cec29ddc29 100644 >> --- a/tools/perf/util/expr.h >> +++ b/tools/perf/util/expr.h >> @@ -55,5 +55,6 @@ double expr_id_data__value(const struct expr_id_data >> *data); >> double expr_id_data__source_count(const struct expr_id_data *data); >> double expr__get_literal(const char *literal, const struct >> expr_scanner_ctx *ctx); >> double expr__has_event(const struct expr_parse_ctx *ctx, bool >> compute_ids, const char *id); >> +double expr__strcmp_cpuid_str(const struct expr_parse_ctx *ctx, bool >> compute_ids, const char *id); >> #endif >> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l >> index dbb117414710..0feef0726c48 100644 >> --- a/tools/perf/util/expr.l >> +++ b/tools/perf/util/expr.l >> @@ -114,6 +114,7 @@ if { return IF; } >> else { return ELSE; } >> source_count { return SOURCE_COUNT; } >> has_event { return HAS_EVENT; } >> +strcmp_cpuid_str { return STRCMP_CPUID_STR; } >> {literal} { return literal(yyscanner, sctx); } >> {number} { return value(yyscanner); } >> {symbol} { return str(yyscanner, ID, sctx->runtime); } >> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y >> index 65d54a6f29ad..6c93b358cc2d 100644 >> --- a/tools/perf/util/expr.y >> +++ b/tools/perf/util/expr.y >> @@ -39,7 +39,7 @@ int expr_lex(YYSTYPE * yylval_param , void *yyscanner); >> } ids; >> } >> -%token ID NUMBER MIN MAX IF ELSE LITERAL D_RATIO SOURCE_COUNT >> HAS_EVENT EXPR_ERROR >> +%token ID NUMBER MIN MAX IF ELSE LITERAL D_RATIO SOURCE_COUNT >> HAS_EVENT STRCMP_CPUID_STR EXPR_ERROR >> %left MIN MAX IF >> %left '|' >> %left '^' >> @@ -207,6 +207,12 @@ expr: NUMBER >> $$.ids = NULL; >> free($3); >> } >> +| STRCMP_CPUID_STR '(' ID ')' >> +{ >> + $$.val = expr__strcmp_cpuid_str(ctx, compute_ids, $3); >> + $$.ids = NULL; >> + free($3); >> +} >> | expr '|' expr >> { >> if (is_const($1.val) && is_const($3.val)) { >> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c >> index b6654b9f55d2..b6948021fe29 100644 >> --- a/tools/perf/util/pmu.c >> +++ b/tools/perf/util/pmu.c >> @@ -1779,3 +1779,20 @@ void perf_pmu__delete(struct perf_pmu *pmu) >> zfree(&pmu->alias_name); >> free(pmu); >> } >> + >> +struct perf_pmu *pmu__find_core_pmu(void) > > Why was this relocated? I don't see anywhere changed which referenced > pmu__find_core_pmu() in this patch > It's in expr__strcmp_cpuid_str(), I added a new call to it. >> +{ >> + struct perf_pmu *pmu = NULL; >> + >> + while ((pmu = perf_pmus__scan_core(pmu))) { >> + /* >> + * The cpumap should cover all CPUs. Otherwise, some CPUs may >> + * not support some events or have different event IDs. >> + */ >> + if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu) >> + return NULL; >> + >> + return pmu; >> + } >> + return NULL; >> +} >> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h >> index 203b92860e3c..580b8d65bd65 100644 >> --- a/tools/perf/util/pmu.h >> +++ b/tools/perf/util/pmu.h >> @@ -288,5 +288,6 @@ int perf_pmu__pathname_fd(int dirfd, const char >> *pmu_name, const char *filename, >> struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, >> const char *lookup_name); >> struct perf_pmu *perf_pmu__create_placeholder_core_pmu(struct >> list_head *core_pmus); >> void perf_pmu__delete(struct perf_pmu *pmu); >> +struct perf_pmu *pmu__find_core_pmu(void); >> #endif /* __PMU_H */ >

2 years

[PATCH v2 0/3] Add support for a streaming interface for TMC ETR

by Mao Jinlong

This patch series is to add support for a streaming interface for TMC ETR to allow for continuous log collection to secondary storage. An interrupt based mechanism is used to stream out the data from the device. QDSS_CS_QDSSCSR_ETRIRQCTRL register is used to set the IRQ byte counter value. The value of this registers defines the number of bytes that when moved by the ETR AXI interface. It will casues an interrupt which can be used by an userspace program to know how much data is present in memory requiring copy to some other location. A zero setting disables the interrupt.A one setting means 8 bytes, two 16 bytes, etc. In other words, the value in this register is the interrupt threshold times 8 bytes. ETR must be enabled when use this interrupt function. Sample: echo 4096 > /sys/bus/coresight/devices/csr0/etr_byte_cntr_val echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink echo 1 > /sys/bus/coresight/devices/stm0/enabl_source cat /dev/byte-cntr > /data/qdss.bin & The log collection will stop after disabling the ETR. codelinaro link: https://git.codelinaro.org/clo/linux-kernel/coresight/-/commits/coresight-b… Change in V2: Make CSR device as a helper device of ETR device. When enable ETR, it will enable CSR byte-cntr function. Mao Jinlong (3): Coresight: Add driver to support for CSR dt-bindings: arm: Adds CoreSight CSR hardware definitions coresight-csr: Add support for streaming interface for ETR .../testing/sysfs-bus-coresight-devices-csr | 6 + .../bindings/arm/qcom,coresight-csr.yaml | 130 ++++++ MAINTAINERS | 2 +- drivers/hwtracing/coresight/Kconfig | 12 + drivers/hwtracing/coresight/Makefile | 2 + drivers/hwtracing/coresight/coresight-core.c | 31 +- .../coresight/coresight-csr-bytecntr.c | 275 ++++++++++++ .../hwtracing/coresight/coresight-csr-core.c | 393 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-csr.h | 112 +++++ drivers/hwtracing/coresight/coresight-priv.h | 8 + .../hwtracing/coresight/coresight-tmc-etr.c | 3 +- drivers/hwtracing/coresight/coresight-tmc.h | 2 + include/dt-bindings/arm/coresight-csr-dt.h | 12 + include/linux/coresight.h | 3 +- 14 files changed, 984 insertions(+), 7 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-csr create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-csr.yaml create mode 100644 drivers/hwtracing/coresight/coresight-csr-bytecntr.c create mode 100644 drivers/hwtracing/coresight/coresight-csr-core.c create mode 100644 drivers/hwtracing/coresight/coresight-csr.h create mode 100644 include/dt-bindings/arm/coresight-csr-dt.h -- 2.17.1

2 years

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

by Anshuman Khandual

On 8/11/23 15:42, Will Deacon wrote: > On Fri, Aug 11, 2023 at 02:13:42PM +0530, Anshuman Khandual wrote: >> On 8/8/23 13:52, 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; >>> + >>> + this_hetid = find_acpi_cpu_topology_hetero_id(cpu); >>> + if (!gsi) { >>> + hetid = this_hetid; >>> + gsi = this_gsi; >>> + } else if (hetid != this_hetid || gsi != this_gsi) { >>> + pr_warn("ACPI: %s: must be homogeneous\n", pdev->name); >>> + return -ENXIO; >>> + } >>> + } >> >> As discussed on the previous version i.e V3 thread, will move the >> 'this_gsi' check after parse_gsi(), inside if (!gsi) conditional >> block. This will treat subsequent cpu parse_gsi()'s failure as a >> mismatch thus triggering the pr_warn() message. >> >> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c >> index 845683ca7c64..6eae772d6298 100644 >> --- a/drivers/perf/arm_pmu_acpi.c >> +++ b/drivers/perf/arm_pmu_acpi.c >> @@ -98,11 +98,11 @@ arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, >> return gsi ? -ENXIO : 0; >> >> this_gsi = parse_gsi(gicc); >> - if (!this_gsi) >> - return gsi ? -ENXIO : 0; >> - >> this_hetid = find_acpi_cpu_topology_hetero_id(cpu); >> if (!gsi) { >> + if (!this_gsi) >> + return 0; > > Why do you need this hunk? Otherwise '0' gsi on all cpus would just clear the above homogeneity test, and end up in acpi_register_gsi() making it fail, but with the following warning before returning with -ENXIO. irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH); if (irq < 0) { pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi); return -ENXIO; } Is this behaviour better than returning 0 after detecting '0' gsi in the first cpu to avoid the above mentioned scenario ? Although 0 gsi followed by non-zero ones will still end up warning about a mismatch.

2 years

[PATCH V4 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 V4: - Added in-code comment for arm_trbe_device_probe() - Reverted back using IS_ENABLED() for SPE PMU platform device - Replaced #ifdef with IS_ENABLED() for TRBE platform device - Protected arm_trbe_acpi_match with ACPI_PTR() - preventing a build failure when CONFIG_ACPI is not enabled - Added __maybe_unused for arm_acpi_register_pmu_device() and dropped config checks with IS_ENABLED() Changes in V3: https://lore.kernel.org/all/20230803055652.1322801-1-anshuman.khandual@arm.… - 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 | 26 +++- drivers/hwtracing/coresight/coresight-trbe.h | 2 + drivers/perf/arm_pmu_acpi.c | 138 +++++++++++++------ include/linux/perf/arm_pmu.h | 1 + 5 files changed, 128 insertions(+), 42 deletions(-) -- 2.25.1

2 years

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

by Suzuki K Poulose

On 08/08/2023 14:16, Will Deacon wrote: > On Tue, Aug 08, 2023 at 09:48:16AM +0100, Suzuki K Poulose wrote: >> On 08/08/2023 09:22, Anshuman Khandual wrote: >>> Sanity checking all the GICC tables for same interrupt number, and ensuring >>> a homogeneous ACPI based machine, could be used for other platform devices >>> as well. Hence this refactors arm_spe_acpi_register_device() into a common >>> helper arm_acpi_register_pmu_device(). >>> >>> Cc: Catalin Marinas <catalin.marinas(a)arm.com> >>> Cc: Will Deacon <will(a)kernel.org> >>> Cc: Mark Rutland <mark.rutland(a)arm.com> >>> Cc: linux-arm-kernel(a)lists.infradead.org >>> Cc: linux-kernel(a)vger.kernel.org >>> Co-developed-by: Will Deacon <will(a)kernel.org> >>> Signed-off-by: Will Deacon <will(a)kernel.org> >>> Signed-off-by: Anshuman Khandual <anshuman.khandual(a)arm.com> >>> --- >>> drivers/perf/arm_pmu_acpi.c | 105 ++++++++++++++++++++++-------------- >>> 1 file changed, 65 insertions(+), 40 deletions(-) >>> >>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c >>> index 90815ad762eb..72454bef2a70 100644 >>> --- a/drivers/perf/arm_pmu_acpi.c >>> +++ b/drivers/perf/arm_pmu_acpi.c >>> @@ -69,6 +69,63 @@ static void arm_pmu_acpi_unregister_irq(int cpu) >>> acpi_unregister_gsi(gsi); >>> } >>> +static int __maybe_unused >>> +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len, >>> + u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *)) >>> +{ >>> + int cpu, this_hetid, hetid, irq, ret; >>> + u16 this_gsi, gsi = 0; >>> + >>> + /* >>> + * Ensure that platform device must have IORESOURCE_IRQ >>> + * resource to hold gsi interrupt. >>> + */ >>> + if (pdev->num_resources != 1) >>> + return -ENXIO; >>> + >>> + if (pdev->resource[0].flags != IORESOURCE_IRQ) >>> + return -ENXIO; >>> + >>> + /* >>> + * 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; >>> + >>> + this_hetid = find_acpi_cpu_topology_hetero_id(cpu); >>> + if (!gsi) { >>> + hetid = this_hetid; >>> + gsi = this_gsi; >>> + } else if (hetid != this_hetid || gsi != this_gsi) { >>> + pr_warn("ACPI: %s: must be homogeneous\n", pdev->name); >>> + return -ENXIO; >>> + } >>> + } >>> + >>> + irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH); >>> + if (irq < 0) { >>> + pr_warn("ACPI: %s Unable to register interrupt: %d\n", pdev->name, gsi); >>> + return -ENXIO; >>> + } >>> + >>> + pdev->resource[0].start = irq; >>> + ret = platform_device_register(pdev); >>> + if (ret < 0) { >>> + pr_warn("ACPI: %s: Unable to register device\n", pdev->name); >>> + acpi_unregister_gsi(gsi); >>> + } >>> + return ret; >> >> A postivie return value here could confuse the caller. Also, with my comment >> below, we don't really need to return something from here. > > How does this return a positive value? Right now, there aren't. My point is this function returns a "return value" of another function. And the caller of this function doesn't really follow the "check" it needs. e.g.: ret = foo(); if (ret < 0) error; return ret; And the caller only checks for if (ret) error; This seems fragile. > >>> + int ret = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE, >>> + arm_spe_parse_gsi); >>> + if (ret) >>> pr_warn("ACPI: SPE: Unable to register device\n"); >> >> With this change, a system without SPE interrupt description always >> generates the above message. Is this intended ? > > If there are no irqs, why doesn't this return 0? Apologies, I missed that. > arm_acpi_register_pmu_device() should only fail if either: > > - The static resources passed in are broken > - The tables are not homogeneous > - We fail to register the interrupt > > so something is amiss. Agreed. We don't need duplicate messages about an error ? i.e., one in arm_acpi_register_pmu_device() and another one in the caller ? (Of course adding any missing error msgs). > >> Could we not drop the above message as all the other possible error >> scenarios are reported. We could simply make the above helper void, see my >> comment above. > > I disagree. If the ACPI tables are borked, we should print a message saying > so. Ok, fair point. Suzuki > > Will

2 years

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight