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.
Apart from compilation and checking that the exclude guest settings
are correctly programmed on guest switch, this is untested by me.
Applies to kvmarm/next (3b4e3afb2032)
[1]: https://lore.kernel.org/linux-arm-kernel/20230804085219.260790-3-james.clar…
James Clark (3):
arm64: KVM: Add support for exclude_guest and exclude_host for ETM
arm64: KVM: Support exclude_guest for Coresight trace in nVHE
coresight: Support exclude_guest with Feat_TRF and nVHE
arch/arm64/include/asm/kvm_host.h | 10 +++-
arch/arm64/kvm/Makefile | 1 +
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/debug.c | 7 +++
arch/arm64/kvm/etm.c | 48 ++++++++++++++++
arch/arm64/kvm/hyp/nvhe/debug-sr.c | 56 +++++++++++++++++--
.../hwtracing/coresight/coresight-etm-perf.c | 4 ++
include/kvm/etm.h | 43 ++++++++++++++
8 files changed, 165 insertions(+), 5 deletions(-)
create mode 100644 arch/arm64/kvm/etm.c
create mode 100644 include/kvm/etm.h
--
2.34.1
On 04/08/2023 13:10, Mark Brown wrote:
> On Fri, Aug 04, 2023 at 09:52:16AM +0100, James Clark wrote:
>
>> TRFCR_EL2_CX needs to become TRFCR_ELx_CX to avoid unnecessary
>> duplication and make the SysregFields block re-usable.
>
> That field is only present in the EL2 version. I would tend to leave
> the registers split for that reason, there's some minor potential for
> confusion if people refer to the sysreg file rather than the docs, or
> potentially confuse some future automation. However that's not a super
> strongly held opinion.
>
True, the potential for confusion is a good reason to not try to avoid
duplication. Probably helps if it is ever auto generated or validated as
well.
I could update it on the next version. But do I leave all the existing
_ELx usages in the code, or change them all to _EL1 (Except CX_EL2)? To
leave them as _ELx sysreg would look like this, even though _EL1 would
probably be more accurate:
SysregFields TRFCR_EL2
Res0 63:7
UnsignedEnum 6:5 TS
0b0001 VIRTUAL
0b0010 GUEST_PHYSICAL
0b0011 PHYSICAL
EndEnum
Res0 4
Field 3 CX
Res0 2
Field 1 E2TRE
Field 0 E0TRE
EndSysregFields
SysregFields TRFCR_ELx
Res0 63:7
UnsignedEnum 6:5 TS
0b0001 VIRTUAL
0b0010 GUEST_PHYSICAL
0b0011 PHYSICAL
EndEnum
Res0 4:2
Field 1 ExTRE
Field 0 E0TRE
EndSysregFields
Sysreg TRFCR_EL1 3 0 1 2 1
Fields TRFCR_ELx
EndSysreg
Sysreg TRFCR_EL2 3 4 1 2 1
Fields TRFCR_EL2
EndSysreg
Sysreg TRFCR_EL12 3 5 1 2 1
Fields TRFCR_ELx
EndSysreg
> Otherwise this checks out against DDI0601 2023-06:
>
> Reviewed-by: Mark Brown <broonie(a)kernel.org>
Thanks for the review
On Fri, 4 Aug 2023 17:27:09 +0800, Yang Yingliang wrote:
> The init/exit() of driver only calls platform_driver_register/unregister,
> it can be simpilfied with module_platform_driver.
>
>
Applied, thanks!
[1/1] coresight: dummy: simplify the code with module_platform_driver
https://git.kernel.org/coresight/c/28a03fae6e524d39fec14fea1ee67b86f4870658
Best regards,
--
Suzuki K Poulose <suzuki.poulose(a)arm.com>
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>
---
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..b88d83b59531 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_treshold
+ - Cycle count treshhold 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..9a2766f68416 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_treshold = attr->config3 & ETM_CYC_THRESHOLD_MASK;
+ if (cc_treshold) {
+ if (cc_treshold < 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
On 03/08/2023 07:22, Tian Tao wrote:
> Each cpu will print the following log when initializing
> ETM "coresight etm1: CPU1: etm v4.5 initialized", if there are a lot
> of cpus, e.g. 128. there will be a screen full of this log. replace
> dev_info with dev_dbg prints only when needed.
>
> Signed-off-by: Tian Tao <tiantao6(a)hisilicon.com>
> ---
> 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 7e307022303a..7b51e8594fd5 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -2033,7 +2033,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
>
> etmdrvdata[drvdata->cpu] = drvdata;
>
> - dev_info(&drvdata->csdev->dev, "CPU%d: %s v%d.%d initialized\n",
> + dev_dbg(&drvdata->csdev->dev, "CPU%d: %s v%d.%d initialized\n",
> drvdata->cpu, type_name, major, minor);
>
> if (boot_enable) {
Hi Tian,
Do you think it's worth keeping a single info printout as well as the
per-cpu dbg one? I'm thinking that the only way to get these messages
now is to boot with debug printing on, and that is quite noisy. Because
it's too late to use debugfs to enable just coresight debug messages
after booting. So something like this:
dev_info_once(&drvdata->csdev->dev, "%s v%d.%d initialized\n",
Also I still get this printed out per-cpu, which is from
coresight-syscfg.c. Do you not see this? Might be worth doing the same
change there.
CSCFG registered etm0
CSCFG registered etm1
CSCFG registered etm2
CSCFG registered etm3
Thanks
James
On 8/3/23 14:44, Yicong Yang wrote:
> On 2023/8/3 13:56, Anshuman Khandual wrote:
>> ACPI TRBE does not have a HID for identification which could create and add
>> a platform device into the platform bus. Also without a platform device, it
>> cannot be probed and bound to a platform driver.
>>
>> This creates a dummy platform device for TRBE after ascertaining that ACPI
>> provides required interrupts uniformly across all cpus on the system. This
>> device gets created inside drivers/perf/arm_pmu_acpi.c to accommodate TRBE
>> being built as a module.
>>
>> 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
>> Signed-off-by: Anshuman Khandual <anshuman.khandual(a)arm.com>
>> ---
>> arch/arm64/include/asm/acpi.h | 3 +++
>> drivers/perf/arm_pmu_acpi.c | 37 ++++++++++++++++++++++++++++++++++-
>> include/linux/perf/arm_pmu.h | 1 +
>> 3 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>> index bd68e1b7f29f..4d537d56eb84 100644
>> --- a/arch/arm64/include/asm/acpi.h
>> +++ b/arch/arm64/include/asm/acpi.h
>> @@ -42,6 +42,9 @@
>> #define ACPI_MADT_GICC_SPE (offsetof(struct acpi_madt_generic_interrupt, \
>> spe_interrupt) + sizeof(u16))
>>
>> +#define ACPI_MADT_GICC_TRBE (offsetof(struct acpi_madt_generic_interrupt, \
>> + trbe_interrupt) + sizeof(u16))
>> +
>> /* Basic configuration for ACPI */
>> #ifdef CONFIG_ACPI
>> pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>> index 235c14766a36..79feea548e6e 100644
>> --- a/drivers/perf/arm_pmu_acpi.c
>> +++ b/drivers/perf/arm_pmu_acpi.c
>> @@ -69,7 +69,7 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>> acpi_unregister_gsi(gsi);
>> }
>>
>> -#if IS_ENABLED(CONFIG_ARM_SPE_PMU)
>> +#if IS_ENABLED(CONFIG_ARM_SPE_PMU) || IS_ENABLED(CONFIG_CORESIGHT_TRBE)
>> static int
>> arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
>> u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
>> @@ -166,6 +166,40 @@ static inline void arm_spe_acpi_register_device(void)
>> }
>> #endif /* CONFIG_ARM_SPE_PMU */
>>
>> +#ifdef CONFIG_CORESIGHT_TRBE
> Maybe we should use #if IS_ENABLED(CONFIG_CORESIGHT_TRBE) here and other places?
>
> As trbe can be configured as a module, when CONFIG_CORESIGHT_TRBE=m this block
> won't be compiled. Referred to
>
> https://github.com/torvalds/linux/blob/c7c90e121e992eefdf07945e5a6e9cf097b2…
You are right, just making CONFIG_CORESIGHT_TRBE=m make it fall back on the
stub arm_trbe_acpi_register_device() definition, which does not create the
required dummy platform device thus preventing TRBE probe on the platform.
Will change the above #ifdef as IS_ENABLED() for CONFIG_CORESIGHT_TRBE and
revert back using IS_ENABLED() in the previous patch for CONFIG_ARM_SPE_PMU
as well. Thanks for noticing this problem.
The first commit moves the register to sysreg because I add the EL12
version in the second commit. I couldn't really think of the best way
to make it fit in sysreg without any changes, because all the fields are
shared between EL1 and EL2 except for the CX bit which was named
differently for that reason in the original definition. The CX bit only
exists in TRFCR_EL2.
The test results have some single spurious EL2 addresses, but I don't
think this is an issue with this patchset because it happens in the
host-userspace case which maintains the existing programming of
TRFCR. It's likely an issue with the model but I will follow it up
separately.
James Clark (2):
arm64/sysreg: Move TRFCR definitions to sysreg
coresight: Allow guests to be traced when FEAT_TRF and VHE are present
arch/arm64/include/asm/sysreg.h | 12 -----
arch/arm64/tools/sysreg | 26 ++++++++++
.../coresight/coresight-etm4x-core.c | 47 ++++++++++++++++---
drivers/hwtracing/coresight/coresight-etm4x.h | 2 +-
drivers/hwtracing/coresight/coresight-priv.h | 3 ++
5 files changed, 70 insertions(+), 20 deletions(-)
--
2.34.1
is_trbe_available() checks for the TRBE support via extracting TraceBuffer
field value from ID_AA64DFR0_EL1, and ensures that it is implemented. This
replaces the open encoding '0b0001' with 'ID_AA64DFR0_EL1_TraceBuffer_IMP'
which is now available via sysreg tools. Functional change is not intended.
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-kernel(a)vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual(a)arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-trbe.h b/drivers/hwtracing/coresight/coresight-trbe.h
index 94e67009848a..ebb9108d8e24 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.h
+++ b/drivers/hwtracing/coresight/coresight-trbe.h
@@ -24,7 +24,7 @@ static inline bool is_trbe_available(void)
unsigned int trbe = cpuid_feature_extract_unsigned_field(aa64dfr0,
ID_AA64DFR0_EL1_TraceBuffer_SHIFT);
- return trbe >= 0b0001;
+ return trbe >= ID_AA64DFR0_EL1_TraceBuffer_IMP;
}
static inline bool is_trbe_enabled(void)
--
2.25.1
On 8/1/23 20:22, Will Deacon wrote:
> On Tue, Aug 01, 2023 at 03:10:48PM +0530, Anshuman Khandual wrote:
>> 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 V2:
>>
>> - 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
>
> FYI: if you pass '--cc-cover' to git send-email, it will CC all these
> folks on the series, which I think is better when you're reviewing stuff
> (I didn't receive patches 3 and 4).
My bad, forgot --cc-cover on the command line.