CoreSight

coresight@lists.linaro.org

8 participants
2394 discussions

Re: How to debug SysReg access to ETE causing system crash

by Suzuki K Poulose

Hi Yabin On 17/08/2023 22:06, Yabin Cui wrote: > Hi coresight driver maintainers, > > I am from the Android toolchain team. I am trying to use ETE (Embedded > Trace Extension). But on my device, the system crashes when the ETM > driver is trying to read TRCDEVARCH via sysreg interface. When you say crashes, what exactly is reported ? Does the kernel get an exception ? Or system hangs completely ? I suspect : 1) You are running some hyp at EL2, which doesn't allow accessing the TRC* registers and injects a Illegal instruction ? 2) If not the above, the ETE is not powered (very unlikely) Suzuki > In > https://github.com/torvalds/linux/blob/master/drivers/hwtracing/coresight/c… <https://github.com/torvalds/linux/blob/master/drivers/hwtracing/coresight/c…> : > > static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata, > struct csdev_access *csa) > { > u32 devarch; > > if (!cpu_supports_sysreg_trace()) <- this returns true > return false; > > /* > * ETMs implementing sysreg access must implement TRCDEVARCH. > */ > devarch = read_etm4x_sysreg_const_offset(TRCDEVARCH); <- never see the > message logged immediately after this line, so should abort here. > ... > } > > Do you have any suggestions on how I can debug the problem? Is there any > other place making sysreg interface to ETE not accessible? Or can the > ETE sysreg interface not be implemented? > I tried the memory mapped register interface of ETE, it works fine. The > only change I need is to bypass the devarch version check > in etm4_init_iomem_access, in in > https://github.com/torvalds/linux/blob/master/drivers/hwtracing/coresight/c… <https://github.com/torvalds/linux/blob/master/drivers/hwtracing/coresight/c…> > I wonder if we can allow iomem access for ETE if the sysreg access > doesn't work on some devices. > > Thanks, > Yabin

1 year, 10 months

[PATCH] coresight: tmc: Make etr buffer mode user configurable from sysfs

by Anshuman Khandual

Currently TMC-ETR automatically selects the buffer mode from all available methods in the following sequentially fallback manner - also in that order. 1. FLAT mode with or without IOMMU 2. TMC-ETR-SG (scatter gather) mode when available 3. CATU mode when available But this order might not be ideal for all situations. For example if there is a CATU connected to ETR, it may be better to use TMC-ETR scatter gather method, rather than CATU. But hard coding such order changes will prevent us from testing or using a particular mode. This change provides following new sysfs tunables for the user to control TMC-ETR buffer mode explicitly, if required. /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_modes_available /sys/bus/coresight/devices/tmc_etr<N>/etr_buf_mode_current $ cat etr_buf_modes_available auto flat tmc-sg catu ------------------> Supported TMC-ETR buffer modes $ echo catu > etr_buf_mode_current -------> Explicit buffer mode request But explicit user request has to be within supported ETR buffer modes only. These sysfs interface files are exclussive to ETR, and hence not available for other TMC devices such as ETB or ETF etc. This required separating out new 'coresight_etr_groups' from common 'coresight_tmc_groups'. This adds a new element 'etr_mode' in 'struct tmc_drvdata' which will track such explicit user directives. 'auto' mode has been added to help fallback to the existing default behaviour. ETR_MODE_FLAT mode availability follows existing logic as in tmc_alloc_etr_buf() creating a common helper function i.e etr_supports_flat_mode(). 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: Alexander Shishkin <alexander.shishkin(a)linux.intel.com> Cc: coresight(a)lists.linaro.org Cc: linux-arm-kernel(a)lists.infradead.org Cc: linux-kernel(a)vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual(a)arm.com> --- .../testing/sysfs-bus-coresight-devices-tmc | 16 +++ .../hwtracing/coresight/coresight-tmc-core.c | 103 +++++++++++++++++- .../hwtracing/coresight/coresight-tmc-etr.c | 27 +++-- drivers/hwtracing/coresight/coresight-tmc.h | 10 ++ 4 files changed, 143 insertions(+), 13 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc index 6aa527296c71..956a2f090950 100644 --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tmc @@ -91,3 +91,19 @@ Contact: Mathieu Poirier <mathieu.poirier(a)linaro.org> Description: (RW) Size of the trace buffer for TMC-ETR when used in SYSFS mode. Writable only for TMC-ETR configurations. The value should be aligned to the kernel pagesize. + +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_modes_available +Date: July 2023 +KernelVersion: 6.6 +Contact: Anshuman Khandual <anshuman.khandual(a)arm.com> +Description: (Read) Shows all supported Coresight TMC-ETR buffer modes available + for the users to configure explicitly. This file is avaialble only + for TMC ETR devices. + +What: /sys/bus/coresight/devices/<memory_map>.tmc/etr_buf_mode_current +Date: July 2023 +KernelVersion: 6.6 +Contact: Anshuman Khandual <anshuman.khandual(a)arm.com> +Description: (RW) Current Coresight TMC-ETR buffer mode selected. But user could + only provide a mode which is supported for a given ETR device. This + file is available only for TMC ETR devices. diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c index c106d142e632..ce97ff5e0997 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-core.c +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c @@ -10,6 +10,7 @@ #include <linux/device.h> #include <linux/idr.h> #include <linux/io.h> +#include <linux/iommu.h> #include <linux/err.h> #include <linux/fs.h> #include <linux/miscdevice.h> @@ -329,6 +330,85 @@ static ssize_t buffer_size_store(struct device *dev, static DEVICE_ATTR_RW(buffer_size); +static const char *const buf_modes_str[] = { + [ETR_MODE_FLAT] = "flat", + [ETR_MODE_ETR_SG] = "tmc-sg", + [ETR_MODE_CATU] = "catu", + [ETR_MODE_AUTO] = "auto", +}; + +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + + buf_hw->has_iommu = iommu_get_domain_for_dev(dev->parent); + buf_hw->has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); + buf_hw->has_catu = !!tmc_etr_get_catu_device(drvdata); +} + +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size) +{ + bool has_sg = buf_hw->has_catu || buf_hw->has_etr_sg; + + return !has_sg || buf_hw->has_iommu || etr_buf_size < SZ_1M; +} + +static ssize_t etr_buf_modes_available_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + struct etr_buf_hw buf_hw; + ssize_t size = 0; + + get_etr_buf_hw(dev, &buf_hw); + size += sysfs_emit(buf, "%s ", buf_modes_str[ETR_MODE_AUTO]); + if (etr_supports_flat_mode(&buf_hw, drvdata->size)) + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_FLAT]); + + if (buf_hw.has_etr_sg) + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_ETR_SG]); + + if (buf_hw.has_catu) + size += sysfs_emit_at(buf, size, "%s ", buf_modes_str[ETR_MODE_CATU]); + + size += sysfs_emit_at(buf, size, "\n"); + return size; +} +static DEVICE_ATTR_RO(etr_buf_modes_available); + +static ssize_t etr_buf_mode_current_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + + return sysfs_emit(buf, "%s\n", buf_modes_str[drvdata->etr_mode]); +} + +static ssize_t etr_buf_mode_current_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + struct tmc_drvdata *drvdata = dev_get_drvdata(dev->parent); + struct etr_buf_hw buf_hw; + + get_etr_buf_hw(dev, &buf_hw); + if (sysfs_streq(buf, buf_modes_str[ETR_MODE_FLAT]) && + etr_supports_flat_mode(&buf_hw, drvdata->size)) + drvdata->etr_mode = ETR_MODE_FLAT; + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_ETR_SG]) && buf_hw.has_etr_sg) + drvdata->etr_mode = ETR_MODE_ETR_SG; + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_CATU]) && buf_hw.has_catu) + drvdata->etr_mode = ETR_MODE_CATU; + else if (sysfs_streq(buf, buf_modes_str[ETR_MODE_AUTO])) + drvdata->etr_mode = ETR_MODE_AUTO; + else + return -EINVAL; + + return size; + +} +static DEVICE_ATTR_RW(etr_buf_mode_current); + static struct attribute *coresight_tmc_attrs[] = { &dev_attr_trigger_cntr.attr, &dev_attr_buffer_size.attr, @@ -350,6 +430,24 @@ static const struct attribute_group *coresight_tmc_groups[] = { NULL, }; +static struct attribute *coresight_etr_attrs[] = { + &dev_attr_trigger_cntr.attr, + &dev_attr_buffer_size.attr, + &dev_attr_etr_buf_modes_available.attr, + &dev_attr_etr_buf_mode_current.attr, + NULL, +}; + +static const struct attribute_group coresight_etr_group = { + .attrs = coresight_etr_attrs, +}; + +static const struct attribute_group *coresight_etr_groups[] = { + &coresight_etr_group, + &coresight_tmc_mgmt_group, + NULL, +}; + static inline bool tmc_etr_can_use_sg(struct device *dev) { return fwnode_property_present(dev->fwnode, "arm,scatter-gather"); @@ -465,6 +563,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) drvdata->memwidth = tmc_get_memwidth(devid); /* This device is not associated with a session */ drvdata->pid = -1; + drvdata->etr_mode = ETR_MODE_AUTO; if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) { drvdata->size = tmc_etr_get_default_buffer_size(dev); @@ -474,16 +573,17 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) } desc.dev = dev; - desc.groups = coresight_tmc_groups; switch (drvdata->config_type) { case TMC_CONFIG_TYPE_ETB: + desc.groups = coresight_tmc_groups; desc.type = CORESIGHT_DEV_TYPE_SINK; desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; desc.ops = &tmc_etb_cs_ops; dev_list = &etb_devs; break; case TMC_CONFIG_TYPE_ETR: + desc.groups = coresight_etr_groups; desc.type = CORESIGHT_DEV_TYPE_SINK; desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM; desc.ops = &tmc_etr_cs_ops; @@ -496,6 +596,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) dev_list = &etr_devs; break; case TMC_CONFIG_TYPE_ETF: + desc.groups = coresight_tmc_groups; desc.type = CORESIGHT_DEV_TYPE_LINKSINK; desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER; desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO; diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 766325de0e29..d48455188243 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -841,23 +841,27 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, int node, void **pages) { int rc = -ENOMEM; - bool has_etr_sg, has_iommu; - bool has_sg, has_catu; struct etr_buf *etr_buf; + struct etr_buf_hw buf_hw; struct device *dev = &drvdata->csdev->dev; - has_etr_sg = tmc_etr_has_cap(drvdata, TMC_ETR_SG); - has_iommu = iommu_get_domain_for_dev(dev->parent); - has_catu = !!tmc_etr_get_catu_device(drvdata); - - has_sg = has_catu || has_etr_sg; - + get_etr_buf_hw(dev, &buf_hw); etr_buf = kzalloc(sizeof(*etr_buf), GFP_KERNEL); if (!etr_buf) return ERR_PTR(-ENOMEM); etr_buf->size = size; + /* If there is user directive for buffer mode, try that first */ + if (drvdata->etr_mode != ETR_MODE_AUTO) { + rc = tmc_etr_mode_alloc_buf(drvdata->etr_mode, drvdata, + etr_buf, node, pages); + if (rc) { + kfree(etr_buf); + return ERR_PTR(rc); + } + } + /* * If we have to use an existing list of pages, we cannot reliably * use a contiguous DMA memory (even if we have an IOMMU). Otherwise, @@ -870,14 +874,13 @@ static struct etr_buf *tmc_alloc_etr_buf(struct tmc_drvdata *drvdata, * Fallback to available mechanisms. * */ - if (!pages && - (!has_sg || has_iommu || size < SZ_1M)) + if (!pages && etr_supports_flat_mode(&buf_hw, size)) rc = tmc_etr_mode_alloc_buf(ETR_MODE_FLAT, drvdata, etr_buf, node, pages); - if (rc && has_etr_sg) + if (rc && buf_hw.has_etr_sg) rc = tmc_etr_mode_alloc_buf(ETR_MODE_ETR_SG, drvdata, etr_buf, node, pages); - if (rc && has_catu) + if (rc && buf_hw.has_catu) rc = tmc_etr_mode_alloc_buf(ETR_MODE_CATU, drvdata, etr_buf, node, pages); if (rc) { diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h index b97da39652d2..ca15ccb1d807 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.h +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -135,6 +135,13 @@ enum etr_mode { ETR_MODE_FLAT, /* Uses contiguous flat buffer */ ETR_MODE_ETR_SG, /* Uses in-built TMC ETR SG mechanism */ ETR_MODE_CATU, /* Use SG mechanism in CATU */ + ETR_MODE_AUTO, /* Use the default mechanism */ +}; + +struct etr_buf_hw { + bool has_iommu; + bool has_etr_sg; + bool has_catu; }; struct etr_buf_operations; @@ -207,6 +214,7 @@ struct tmc_drvdata { enum tmc_mem_intf_width memwidth; u32 trigger_cntr; u32 etr_caps; + enum etr_mode etr_mode; struct idr idr; struct mutex idr_mutex; struct etr_buf *sysfs_buf; @@ -334,5 +342,7 @@ void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu); void tmc_etr_remove_catu_ops(void); struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, enum cs_mode mode, void *data); +void get_etr_buf_hw(struct device *dev, struct etr_buf_hw *buf_hw); +bool etr_supports_flat_mode(struct etr_buf_hw *buf_hw, ssize_t etr_buf_size); #endif -- 2.25.1

1 year, 10 months

[PATCH V2] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

by Anshuman Khandual

This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put() in etm4_remove_platform_dev(). The problem was detected using Smatch static checker as reported. 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: coresight(a)lists.linaro.org Cc: linux-arm-kernel(a)lists.infradead.org Cc: linux-kernel(a)vger.kernel.org Reported-by: Dan Carpenter <dan.carpenter(a)linaro.org> Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4… Reviewed-by: James Clark <james.clark(a)arm.com> Reviewed-by: Mike Leach <mike.leach(a)lnaro.org> Signed-off-by: Anshuman Khandual <anshuman.khandual(a)arm.com> --- This applies on coresight-next Changes in V2: - Dropped drvdata->pclk check for non-NULL - Replaced IS_ERR() with IS_ERR_OR_NULL() for drvdata->pclk Changes in V1: https://lore.kernel.org/all/20230811062738.1066787-1-anshuman.khandual@arm.… drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 703b6fcbb6a5..77b0271ce6eb 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -2269,7 +2269,7 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev) etm4_remove_dev(drvdata); pm_runtime_disable(&pdev->dev); - if (drvdata->pclk) + if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) clk_put(drvdata->pclk); return 0; -- 2.25.1

1 year, 10 months

[PATCH 0/2] Fix some issues with TRBE building as a module

by Junhao He

The TRBE driver support is build as a module, we found some driver issues based on the patchset [1] and set CONFIG_CORESIGHT_TRBE=m. 1. TRBE driver potential sleep in atomic context when unregister device 2. Multiple free the platform data resource when rmmod coresight TRBE driver [1] "coresight: trbe: Enable ACPI based devices" https://lore.kernel.org/all/20230808082247.383405-1-anshuman.khandual@arm.c… Junhao He (2): coresight: trbe: Fix TRBE potential sleep in atomic context coresight: core: Fix multiple free TRBE platform data resource drivers/hwtracing/coresight/coresight-core.c | 7 ++-- drivers/hwtracing/coresight/coresight-trbe.c | 35 +++++++++++--------- 2 files changed, 24 insertions(+), 18 deletions(-) -- 2.33.0

1 year, 10 months

[PATCH V5 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-rc5 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 V5: - Detected zeroed parsed GSI as a mismatch but handled all zero scenario - Changed condition check from 'if (ret < 0)' into a 'if (ret)' - Dropped pr_warn() message after platform_device_register() Changes in V4: https://lore.kernel.org/all/20230808082247.383405-1-anshuman.khandual@arm.c… - 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 | 142 ++++++++++++++----- include/linux/perf/arm_pmu.h | 1 + 5 files changed, 132 insertions(+), 42 deletions(-) -- 2.25.1

1 year, 10 months

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

by James Clark

On 15/08/2023 10:40, John Garry wrote: > On 11/08/2023 15:39, James Clark wrote: >> 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 allows calling strcmp_cpuid_str() and comparing CPUIDs in metric >> formulas. >> >> In this case, the commented formula looks like this: >> >> strcmp_cpuid_str(0x410fd493) # greater than or equal to N2 r0p3 >> | strcmp_cpuid_str(0x410fd490) ^ 1 # OR NOT any version of N2 >> >> [1]:https://urldefense.com/v3/__https://gitlab.arm.com/telemetry-solution/te… Signed-off-by: James Clark<james.clark(a)arm.com> >> --- >> tools/perf/arch/arm64/util/pmu.c | 18 +----------------- >> .../arch/arm64/arm/neoverse-n2-v2/metrics.json | 8 ++++---- >> tools/perf/pmu-events/metric.py | 17 +++++++++++++++-- >> 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 + >> 9 files changed, 65 insertions(+), 24 deletions(-) > > This patch looks ok, but I think that it would be better to separate out > the metrics.json change into a separate patch. > Yep I thought about doing that, I can change it in the next version. > Thanks, > John

1 year, 11 months

Re: [PATCH v5 3/6] perf test: Add a test for the new Arm CPU ID comparison behavior

by James Clark

On 15/08/2023 10:47, John Garry wrote: > On 11/08/2023 15:39, James Clark wrote: >> Now that variant and revision fields are taken into account the behavior >> is slightly more complicated so add a test to ensure that this behaves >> as expected. >> >> Signed-off-by: James Clark <james.clark(a)arm.com> >> --- >> 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 ++++++++++++++++++++++ >> 4 files changed, 46 insertions(+) >> create mode 100644 tools/perf/arch/arm64/tests/cpuid-match.c >> >> diff --git a/tools/perf/arch/arm64/include/arch-tests.h >> b/tools/perf/arch/arm64/include/arch-tests.h >> index 452b3d904521..474d7cf5afbd 100644 >> --- a/tools/perf/arch/arm64/include/arch-tests.h >> +++ b/tools/perf/arch/arm64/include/arch-tests.h >> @@ -2,6 +2,9 @@ >> #ifndef ARCH_TESTS_H >> #define ARCH_TESTS_H >> +struct test_suite; >> + >> +int test__cpuid_match(struct test_suite *test, int subtest); >> extern struct test_suite *arch_tests[]; >> #endif >> diff --git a/tools/perf/arch/arm64/tests/Build >> b/tools/perf/arch/arm64/tests/Build >> index a61c06bdb757..e337c09e7f56 100644 >> --- a/tools/perf/arch/arm64/tests/Build >> +++ b/tools/perf/arch/arm64/tests/Build >> @@ -2,3 +2,4 @@ perf-y += regs_load.o >> perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o >> perf-y += arch-tests.o >> +perf-y += cpuid-match.o >> diff --git a/tools/perf/arch/arm64/tests/arch-tests.c >> b/tools/perf/arch/arm64/tests/arch-tests.c >> index ad16b4f8f63e..74932e72c727 100644 >> --- a/tools/perf/arch/arm64/tests/arch-tests.c >> +++ b/tools/perf/arch/arm64/tests/arch-tests.c >> @@ -3,9 +3,13 @@ >> #include "tests/tests.h" >> #include "arch-tests.h" >> + >> +DEFINE_SUITE("arm64 CPUID matching", cpuid_match); >> + >> struct test_suite *arch_tests[] = { >> #ifdef HAVE_DWARF_UNWIND_SUPPORT >> &suite__dwarf_unwind, >> #endif >> + &suite__cpuid_match, >> NULL, >> }; >> diff --git a/tools/perf/arch/arm64/tests/cpuid-match.c >> b/tools/perf/arch/arm64/tests/cpuid-match.c >> new file mode 100644 >> index 000000000000..af0871b54ae7 >> --- /dev/null >> +++ b/tools/perf/arch/arm64/tests/cpuid-match.c >> @@ -0,0 +1,38 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +#include <linux/compiler.h> >> + >> +#include "arch-tests.h" >> +#include "tests/tests.h" >> +#include "util/header.h" >> + >> +int test__cpuid_match(struct test_suite *test __maybe_unused, >> + int subtest __maybe_unused) >> +{ >> + /* midr with no leading zeros matches */ >> + if (strcmp_cpuid_str("0x410fd0c0", "0x00000000410fd0c0")) >> + return -1; >> + /* Upper case matches */ >> + if (strcmp_cpuid_str("0x410fd0c0", "0x00000000410FD0C0")) >> + return -1; >> + /* r0p0 = r0p0 matches */ >> + if (strcmp_cpuid_str("0x00000000410fd480", "0x00000000410fd480")) >> + return -1; >> + /* r0p1 > r0p0 matches */ >> + if (strcmp_cpuid_str("0x00000000410fd480", "0x00000000410fd481")) >> + return -1; >> + /* r1p0 > r0p0 matches*/ >> + if (strcmp_cpuid_str("0x00000000410fd480", "0x00000000411fd480")) >> + return -1; >> + /* r0p0 < r0p1 doesn't match */ >> + if (!strcmp_cpuid_str("0x00000000410fd481", "0x00000000410fd480")) >> + return -1; >> + /* r0p0 < r1p0 doesn't match */ >> + if (!strcmp_cpuid_str("0x00000000411fd480", "0x00000000410fd480")) >> + return -1; >> + /* Different CPU doesn't match */ >> + if (!strcmp_cpuid_str("0x00000000410fd4c0", "0x00000000430f0af0")) >> + return -1; >> + >> + return 0; >> +} >> + > > Would it be possible to put this in core test code, since x86 also > supports strcmp_cpuid_str()? > That's how I started, but Ian suggested to move it to an arch specific folder because that's what it was testing. We could still add test__cpuid_match() in the x86 folder rather than adding it with #ifdefs, but I don't think it needs to be done here because I haven't touched the x86 code. > Maybe we would have an structure per arch of cpuids and expected > results, like > > struct cpuid_match { > char *cpuid1; > char *cpuid1; > int expected_result; > }; > > > #ifdef ARM64 > cpuid_match_array[] = { > {"0x410fd0c0", "0x00000000410FD0C0", -1}, > {"0x00000000410fd480", "0x00000000410fd480", -1}, > ... > {} /* sentinel */ > > }; > #else if defined(X86) > cpuid_match_array[] = { > {....} > ... > {} /* sentinel */ > > }; > #else > /* no support */ > #endif > > Thanks, > John

1 year, 11 months

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

by James Clark

On 15/08/2023 10:35, 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. 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. >> > > This looks ok apart from a couple of comments, below. > > Thanks, > John > >> 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; > > Is there a reason to drop this check? > > As I see, it is still checked in perf_pmu__getcpudid() -> > get_cpuid_str() -> _get_cpuid(), and we don't zero the buf allocated in > _get_cpuid() > Ah yes, now if all the files fail to open or read then buf will be uninitialized. I make it so that it will return EINVAL unless the fgets() succeeds, but I don't think we need to add the strtoul() back in? >> - >> 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. > > And what other values may be returned? If just 0/1, then can we have a > bool return value? > I don't think that's best for consistency. All the other CPU ID comparison functions return the strcmp style return values which is the reverse of booleans. We could change them all to bool, but it would be a big change, and they'd still have strcmp in the name which suggests -1/0/1 return values (although -1 is never used here). I will add to the comment that 1 is returned for a comparison failure thought. That is missing. >> + * >> + * 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) >> +{ >> + 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; >> +} >

1 year, 11 months

[PATCH 1/1] coresight: Fix the ACPI memory leak

by Junhao He

The ACPI buffer memory (buf.pointer) should be freed as the buffer is not used after returning from acpi_get_dsd_graph(), free it to prevent memory leak. Fixes: 76ffa5ab5b79 ("coresight: Support for ACPI bindings") Signed-off-by: Junhao He <hejunhao3(a)huawei.com> --- drivers/hwtracing/coresight/coresight-platform.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 3e2e135cb8f6..c61cac5e058e 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -524,12 +524,15 @@ acpi_get_dsd_graph(struct acpi_device *adev) /* Skip the non-Graph _DSD packages */ if (!is_acpi_dsd_graph_guid(guid)) continue; - if (acpi_validate_dsd_graph(package)) + if (acpi_validate_dsd_graph(package)) { + ACPI_FREE(buf.pointer); return package; + } /* Invalid graph format, continue */ dev_warn(&adev->dev, "Invalid Graph _DSD property\n"); } + ACPI_FREE(buf.pointer); return NULL; } -- 2.33.0

1 year, 11 months

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

by James Clark

On 14/08/2023 15:43, John Garry wrote: > On 14/08/2023 15:15, James Clark wrote: >> >> 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 >> > > ok, fine, so we seems that we would be following x86 on this in terms of > using strcmp_cpuid_str(). It would be good to mention that there is > already a weak version of strcmp_cpuid_str() for !x86 in your commit > message. > Yep I can add this. > Let me check your code again... > > Thanks, > John

1 year, 11 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight