On 04/01/2024 16:27, James Clark wrote:
> The guest value for TRFCR requested by the Coresight driver is saved in
> kvm_host_global_state. On guest switch this value needs to be written to
> the register. Currently TRFCR is only modified when we want to disable
> trace completely in guests due to an issue with TRBE. Expand the
> __debug_save_trace() function to always write to the register if a
> different value for guests is required, but also keep the existing TRBE
> disable behavior if that's required.
>
> The TRFCR restore function remains functionally the same, except a value
> of 0 doesn't mean "don't restore" anymore. Now that we save both guest
> and host values the register is restored any time the guest and host
> values differ.
>
> Signed-off-by: James Clark <james.clark(a)arm.com>
> ---
> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 55 ++++++++++++++++++------------
> 1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index 4558c02eb352..7fd876d4f034 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -51,32 +51,45 @@ static void __debug_restore_spe(u64 pmscr_el1)
> write_sysreg_s(pmscr_el1, SYS_PMSCR_EL1);
> }
>
> -static void __debug_save_trace(u64 *trfcr_el1)
> +/*
> + * Save TRFCR and disable trace completely if TRBE is being used, otherwise
> + * apply required guest TRFCR value.
> + */
> +static void __debug_save_trace(struct kvm_vcpu *vcpu)
> {
> - *trfcr_el1 = 0;
> + u64 host_trfcr_el1 = read_sysreg_s(SYS_TRFCR_EL1);
> + u64 guest_trfcr_el1;
> +
> + vcpu->arch.host_debug_state.trfcr_el1 = host_trfcr_el1;
>
> /* Check if the TRBE is enabled */
> - if (!(read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_EL1_E))
> - return;
> - /*
> - * Prohibit trace generation while we are in guest.
> - * Since access to TRFCR_EL1 is trapped, the guest can't
> - * modify the filtering set by the host.
> - */
> - *trfcr_el1 = read_sysreg_s(SYS_TRFCR_EL1);
> - write_sysreg_s(0, SYS_TRFCR_EL1);
> - isb();
> - /* Drain the trace buffer to memory */
> - tsb_csync();
> + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE) &&
> + (read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_EL1_E)) {
> + /*
> + * Prohibit trace generation while we are in guest. Since access
> + * to TRFCR_EL1 is trapped, the guest can't modify the filtering
> + * set by the host.
> + */
> + write_sysreg_s(0, SYS_TRFCR_EL1);
> + isb();
> + /* Drain the trace buffer to memory */
> + tsb_csync();
> + } else {
> + /*
> + * Not using TRBE, so guest trace works. Apply the guest filters
> + * provided by the Coresight driver, if different.
> + */
> + guest_trfcr_el1 = kvm_host_global_state[vcpu->cpu].guest_trfcr_el1;
> + if (host_trfcr_el1 != guest_trfcr_el1)
> + write_sysreg_s(guest_trfcr_el1, SYS_TRFCR_EL1);
> + }
> }
>
> static void __debug_restore_trace(u64 trfcr_el1)
> {
> - if (!trfcr_el1)
> - return;
> -
> /* Restore trace filter controls */
> - write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
> + if (trfcr_el1 != read_sysreg_s(SYS_TRFCR_EL1))
> + write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
Could we not write it unconditionally here ? In the saving step, we have
to save the host setting. But while restoring, we could skip the check.
A read and write is probably the same cost, as the value is implicitly
synchronized by a later ISB.
Eitherways,
Reviewed-by: Suzuki K Poulose <suzuki.poulose(a)arm.com>
> }
>
> void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
> @@ -85,8 +98,8 @@ void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
> __debug_save_spe(&vcpu->arch.host_debug_state.pmscr_el1);
> /* Disable and flush Self-Hosted Trace generation */
> - if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
> - __debug_save_trace(&vcpu->arch.host_debug_state.trfcr_el1);
> + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR))
> + __debug_save_trace(vcpu);
> }
>
> void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
> @@ -98,7 +111,7 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
> {
> if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
> __debug_restore_spe(vcpu->arch.host_debug_state.pmscr_el1);
> - if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
> + if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR))
> __debug_restore_trace(vcpu->arch.host_debug_state.trfcr_el1);
> }
>
Currently the userspace and kernel filters for guests are never set, so
no trace will be generated for them. Add support for tracing guests by
passing the desired TRFCR value to KVM so it can be applied to the
guest.
By writing either E1TRE or E0TRE, filtering on either guest kernel or
guest userspace is also supported. And if both E1TRE and E0TRE are
cleared when exclude_guest is set, that option is supported too. This
change also brings exclude_host support which is difficult to add as a
separate commit without excess churn and resulting in no trace at all.
Testing
=======
The addresses were counted with the following:
$ perf report -D | grep -Eo 'EL2|EL1|EL0' | sort | uniq -c
Guest kernel only:
$ perf record -e cs_etm//Gk -a -- true
535 EL1
1 EL2
Guest user only (only 5 addresses because the guest runs slowly in the
model):
$ perf record -e cs_etm//Gu -a -- true
5 EL0
Host kernel only:
$ perf record -e cs_etm//Hk -a -- true
3501 EL2
Host userspace only:
$ perf record -e cs_etm//Hu -a -- true
408 EL0
1 EL2
Signed-off-by: James Clark <james.clark(a)arm.com>
---
.../coresight/coresight-etm4x-core.c | 42 ++++++++++++++++---
drivers/hwtracing/coresight/coresight-etm4x.h | 2 +-
drivers/hwtracing/coresight/coresight-priv.h | 3 ++
3 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 34aee59dd147..885d70fd6f40 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -6,6 +6,7 @@
#include <linux/acpi.h>
#include <linux/bitops.h>
#include <linux/kernel.h>
+#include <linux/kvm_host.h>
#include <linux/moduleparam.h>
#include <linux/init.h>
#include <linux/types.h>
@@ -271,9 +272,22 @@ static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata)
/* If the CPU doesn't support FEAT_TRF, nothing to do */
if (!drvdata->trfcr)
return;
+ kvm_etm_set_guest_trfcr(0);
cpu_prohibit_trace();
}
+static u64 etm4x_get_kern_user_filter(struct etmv4_drvdata *drvdata)
+{
+ u64 trfcr = drvdata->trfcr;
+
+ if (drvdata->config.mode & ETM_MODE_EXCL_KERN)
+ trfcr &= ~TRFCR_ELx_ExTRE;
+ if (drvdata->config.mode & ETM_MODE_EXCL_USER)
+ trfcr &= ~TRFCR_ELx_E0TRE;
+
+ return trfcr;
+}
+
/*
* etm4x_allow_trace - Allow CPU tracing in the respective ELs,
* as configured by the drvdata->config.mode for the current
@@ -286,18 +300,28 @@ static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata)
*/
static void etm4x_allow_trace(struct etmv4_drvdata *drvdata)
{
- u64 trfcr = drvdata->trfcr;
+ u64 trfcr;
/* If the CPU doesn't support FEAT_TRF, nothing to do */
- if (!trfcr)
+ if (!drvdata->trfcr)
return;
- if (drvdata->config.mode & ETM_MODE_EXCL_KERN)
- trfcr &= ~TRFCR_ELx_ExTRE;
- if (drvdata->config.mode & ETM_MODE_EXCL_USER)
- trfcr &= ~TRFCR_ELx_E0TRE;
+ if (drvdata->config.mode & ETM_MODE_EXCL_HOST)
+ trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE);
+ else
+ trfcr = etm4x_get_kern_user_filter(drvdata);
write_trfcr(trfcr);
+
+ /* Set filters for guests and pass to KVM */
+ if (drvdata->config.mode & ETM_MODE_EXCL_GUEST)
+ trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE);
+ else
+ trfcr = etm4x_get_kern_user_filter(drvdata);
+
+ /* TRFCR_EL1 doesn't have CX so mask it out. */
+ trfcr &= ~TRFCR_EL2_CX;
+ kvm_etm_set_guest_trfcr(trfcr);
}
#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
@@ -655,6 +679,12 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
if (attr->exclude_user)
config->mode = ETM_MODE_EXCL_USER;
+ if (attr->exclude_host)
+ config->mode |= ETM_MODE_EXCL_HOST;
+
+ if (attr->exclude_guest)
+ config->mode |= ETM_MODE_EXCL_GUEST;
+
/* Always start from the default config */
etm4_set_default_config(config);
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 20e2e4cb7614..3f170599822f 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -841,7 +841,7 @@ enum etm_impdef_type {
* @s_ex_level: Secure ELs where tracing is supported.
*/
struct etmv4_config {
- u32 mode;
+ u64 mode;
u32 pe_sel;
u32 cfg;
u32 eventctrl0;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 767076e07970..727dd27ba800 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -39,6 +39,9 @@
#define ETM_MODE_EXCL_KERN BIT(30)
#define ETM_MODE_EXCL_USER BIT(31)
+#define ETM_MODE_EXCL_HOST BIT(32)
+#define ETM_MODE_EXCL_GUEST BIT(33)
+
struct cs_pair_attribute {
struct device_attribute attr;
u32 lo_off;
--
2.34.1
On 04/01/2024 16:27, James Clark wrote:
> Add an interface for the Coresight driver to use to set the value of the
> TRFCR register for the guest. This register controls the exclude
> settings for trace at different exception levels, and is used to honor
> the exclude_host and exclude_guest parameters from the Perf session.
> This will be used to later write TRFCR_EL1 on nVHE at guest switch. For
> VHE, the host trace is controlled by TRFCR_EL2 and thus we can write to
> the TRFCR_EL1 immediately. Because guest writes to the register are
> trapped, the value will persist and can't be modified.
>
> Signed-off-by: James Clark <james.clark(a)arm.com>
> ---
> arch/arm64/include/asm/kvm_host.h | 3 +++
> arch/arm64/kvm/debug.c | 24 ++++++++++++++++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 4864a1fcdf89..ee6cba7ee6ee 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -471,6 +471,7 @@ struct kvm_host_global_state {
> u32 events_host;
> u32 events_guest;
> } pmu_events;
> + u64 guest_trfcr_el1;
> } ____cacheline_aligned;
> extern struct kvm_host_global_state kvm_host_global_state[NR_CPUS];
>
> @@ -1145,6 +1146,7 @@ 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_etm_set_guest_trfcr(u64 trfcr_guest);
> #else
> static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {}
> static inline void kvm_clr_pmu_events(u32 clr) {}
> @@ -1152,6 +1154,7 @@ static inline bool kvm_set_pmuserenr(u64 val)
> {
> return false;
> }
> +static inline void kvm_etm_set_guest_trfcr(u64 trfcr_guest) {}
> #endif
>
> void kvm_vcpu_load_vhe(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index f86cbfae60f3..d69a0b9d9575 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -358,3 +358,27 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
> }
> +
> +/*
> + * Interface for the Coresight driver to use to set the value of the TRFCR
> + * register for the guest. This register controls the exclude settings for trace
> + * at different exception levels, and is used to honor the exclude_host and
> + * exclude_guest parameters from the Perf session.
> + *
> + * This will be used to later write TRFCR_EL1 on nVHE at guest switch. For VHE,
> + * the host trace is controlled by TRFCR_EL2 and thus we can write to the
> + * TRFCR_EL1 immediately. Because guest writes to the register are trapped, the
> + * value will persist and can't be modified.
> + */
> +void kvm_etm_set_guest_trfcr(u64 trfcr_guest)
> +{
> + if (!cpuid_feature_extract_unsigned_field(read_sysreg(id_aa64dfr0_el1),
> + ID_AA64DFR0_EL1_TraceFilt_SHIFT))
Perhaps WARN_ON_ONCE() ?
Otherwise,
Reviewed-by: Suzuki K Poulose <suzuki.poulose(a)arm.com>
> + return;
> +
> + if (has_vhe())
> + write_sysreg_s(trfcr_guest, SYS_TRFCR_EL12);
> + else
> + kvm_host_global_state[smp_processor_id()].guest_trfcr_el1 = trfcr_guest;
> +}
> +EXPORT_SYMBOL_GPL(kvm_etm_set_guest_trfcr);
finalise_host_mappings() became fix_host_ownership() in
commit 0d16d12eb26e ("KVM: arm64: Fix-up hyp stage-1 refcounts for all
pages mapped at EL2") so update the comment.
Signed-off-by: James Clark <james.clark(a)arm.com>
---
arch/arm64/kvm/hyp/nvhe/setup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index cd2f39388251..b5452e58c49a 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -150,7 +150,7 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
* can't be donated or shared with another entity.
*
* The ownership transition requires matching changes in the host
- * stage-2. This will be done later (see finalize_host_mappings()) once
+ * stage-2. This will be done later (see fix_host_ownership()) once
* the hyp_vmemmap is addressable.
*/
prot = pkvm_mkstate(PAGE_HYP_RO, PKVM_PAGE_SHARED_OWNED);
--
2.34.1
On 26/12/2023 09:36, Krzysztof Kozlowski wrote:
> On 26/12/2023 02:50, Jinlong Mao wrote:
>>
>>
>> On 12/21/2023 4:44 PM, Krzysztof Kozlowski wrote:
>>> On 21/12/2023 09:36, Jinlong Mao wrote:
>>>>
>>>>
>>>> On 12/21/2023 4:17 PM, Krzysztof Kozlowski wrote:
>>>>> On 21/12/2023 09:15, Jinlong Mao wrote:
>>>>>>
>>>>>>
>>>>>> On 12/21/2023 4:12 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 21/12/2023 04:28, Jinlong Mao wrote:
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml b/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>>>>>> index f725e6940993..cbf583d34029 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>>>>>> @@ -23,7 +23,7 @@ description: |
>>>>>>>>>>
>>>>>>>>>> properties:
>>>>>>>>>> $nodename:
>>>>>>>>>> - pattern: "^ete([0-9a-f]+)$"
>>>>>>>>>> + pattern: "^ete-([0-9a-f]+)$"
>>>>>>>>>
>>>>>>>>> My concerns are not resolved. Why is it here in the first place?
>>>>>>>>
>>>>>>>> Hi Krzysztof,
>>>>>>>>
>>>>>>>> ETE is acronym of embedded trace extension. The number of the name is
>>>>>>>> the same as the number of the CPU it belongs to.
>>>>>>>
>>>>>>> This is obvious and was not my question.
>
> You already said it here...
>
>>>>>>
>>>>>> Do you mean why the pattern match of the node name is added here ?
>>>>>
>>>>> Yes, especially that it is requiring a non-generic name.
>>>>>
>>>>>>
>>>>>> This node should not have the node name match, right ?
>>>>>
>>>>> Usually. For sure shouldn't be for non-generic names.
>>>>>
>>>> Hi Suzuki,
>>>>
>>>> Can we remove the pattern match of the node name and use a generic name
>>>> "ete" for the ete DT nodes ?
>>>
>>> "ete" is not a generic name. What is generic here? It's an acronym of
>>> some specific device name.
>>>
>>
>> The device full name is embedded trace extension. So use ETE as the name
>> here.
>
> That's obvious and my comment was not about it. Second time... This is
> my unlucky day... I said, why do you even want to enforce name which is
> not generic, since the names should be generic?
>
I think we can just drop the enforced name if it's getting in the way.
It doesn't really do anything and other Coresight bindings don't have it
anyway.
> I assume you read the DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetr…
>
>
> Best regards,
> Krzysztof
>
I couldn't find anything in that list that would be a good fit for a
name, and it seems like all of the Coresight devices have already been
added with non generic names (like funnel and replicator etc), so it
might be a bit late now.
But if we drop the enforced name then it's probably fine.
James
On 12/21/2023 4:44 PM, Krzysztof Kozlowski wrote:
> On 21/12/2023 09:36, Jinlong Mao wrote:
>>
>>
>> On 12/21/2023 4:17 PM, Krzysztof Kozlowski wrote:
>>> On 21/12/2023 09:15, Jinlong Mao wrote:
>>>>
>>>>
>>>> On 12/21/2023 4:12 PM, Krzysztof Kozlowski wrote:
>>>>> On 21/12/2023 04:28, Jinlong Mao wrote:
>>>>>>>> diff --git a/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml b/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>>>> index f725e6940993..cbf583d34029 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>>>> @@ -23,7 +23,7 @@ description: |
>>>>>>>>
>>>>>>>> properties:
>>>>>>>> $nodename:
>>>>>>>> - pattern: "^ete([0-9a-f]+)$"
>>>>>>>> + pattern: "^ete-([0-9a-f]+)$"
>>>>>>>
>>>>>>> My concerns are not resolved. Why is it here in the first place?
>>>>>>
>>>>>> Hi Krzysztof,
>>>>>>
>>>>>> ETE is acronym of embedded trace extension. The number of the name is
>>>>>> the same as the number of the CPU it belongs to.
>>>>>
>>>>> This is obvious and was not my question.
>>>>
>>>> Do you mean why the pattern match of the node name is added here ?
>>>
>>> Yes, especially that it is requiring a non-generic name.
>>>
>>>>
>>>> This node should not have the node name match, right ?
>>>
>>> Usually. For sure shouldn't be for non-generic names.
>>>
>> Hi Suzuki,
>>
>> Can we remove the pattern match of the node name and use a generic name
>> "ete" for the ete DT nodes ?
>
> "ete" is not a generic name. What is generic here? It's an acronym of
> some specific device name.
>
The device full name is embedded trace extension. So use ETE as the name
here.
Thanks
Jinlong Mao
>
On 20/12/2023 16:16, Adrian Hunter wrote:
> On 20/12/23 17:54, James Clark wrote:
>>
>>
>> On 08/12/2023 17:24, Adrian Hunter wrote:
>>> Hardware traces, such as instruction traces, can produce a vast amount of
>>> trace data, so being able to reduce tracing to more specific circumstances
>>> can be useful.
>>>
>>> The ability to pause or resume tracing when another event happens, can do
>>> that.
>>>
>>> Add ability for an event to "pause" or "resume" AUX area tracing.
>>>
>>> Add aux_pause bit to perf_event_attr to indicate that, if the event
>>> happens, the associated AUX area tracing should be paused. Ditto
>>> aux_resume. Do not allow aux_pause and aux_resume to be set together.
>>>
>>> Add aux_start_paused bit to perf_event_attr to indicate to an AUX area
>>> event that it should start in a "paused" state.
>>>
>>> Add aux_paused to struct perf_event for AUX area events to keep track of
>>> the "paused" state. aux_paused is initialized to aux_start_paused.
>>>
>>> Add PERF_EF_PAUSE and PERF_EF_RESUME modes for ->stop() and ->start()
>>> callbacks. Call as needed, during __perf_event_output(). Add
>>> aux_in_pause_resume to struct perf_buffer to prevent races with the NMI
>>> handler. Pause/resume in NMI context will miss out if it coincides with
>>> another pause/resume.
>>>
>>> To use aux_pause or aux_resume, an event must be in a group with the AUX
>>> area event as the group leader.
>>>
>>> Example (requires Intel PT and tools patches also):
>>>
>>> $ perf record --kcore -e '{intel_pt/aux-start-paused/k,syscalls:sys_enter_newuname/aux-resume/,syscalls:sys_exit_newuname/aux-pause/}' uname
>>
>> I think it might be useful to have an aux-toggle option as well, and
>> then you could do sampling if you put it on a PMU counter with an
>> interval. Unless you can make two events for the same counter with
>> different intervals, and one does resume and the other does pause? I'm
>> not sure if that would work?
>
> There is already ->snapshot_aux() for sampling. Is that what you mean?
>
I suppose that mostly handles that use case yes. Although there are some
slight differences. It looks like for SAMPLE_AUX, the buffer size for
each sample is fixed and limited to 16 bits in size, whereas between
pause and resume you could potentially have multiple buffers delivered
to userspace of any size.
And it looks like SAMPLE_AUX would leave the trace running even when no
samples were being collected. I suppose you might not want to consume
the memory bandwidth and turn the trace off between samples, which
pause/resume does. Especially if you intend to have long periods between
the samples.
I think if it did turn out to be useful the toggle function can easily
be added later, so I don't intend this comment to be a blocking one.
>>
>> Other than that it looks ok. I got Coresight working with a couple of
>> changes to what you posted on here, but that can always be done more
>> thoroughly later if we leave PERF_PMU_CAP_AUX_PAUSE off Coresight for now.
>
> Thanks a lot for looking at this!
>
On 12/21/2023 4:17 PM, Krzysztof Kozlowski wrote:
> On 21/12/2023 09:15, Jinlong Mao wrote:
>>
>>
>> On 12/21/2023 4:12 PM, Krzysztof Kozlowski wrote:
>>> On 21/12/2023 04:28, Jinlong Mao wrote:
>>>>>> diff --git a/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml b/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>> index f725e6940993..cbf583d34029 100644
>>>>>> --- a/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/arm/arm,embedded-trace-extension.yaml
>>>>>> @@ -23,7 +23,7 @@ description: |
>>>>>>
>>>>>> properties:
>>>>>> $nodename:
>>>>>> - pattern: "^ete([0-9a-f]+)$"
>>>>>> + pattern: "^ete-([0-9a-f]+)$"
>>>>>
>>>>> My concerns are not resolved. Why is it here in the first place?
>>>>
>>>> Hi Krzysztof,
>>>>
>>>> ETE is acronym of embedded trace extension. The number of the name is
>>>> the same as the number of the CPU it belongs to.
>>>
>>> This is obvious and was not my question.
>>
>> Do you mean why the pattern match of the node name is added here ?
>
> Yes, especially that it is requiring a non-generic name.
>
>>
>> This node should not have the node name match, right ?
>
> Usually. For sure shouldn't be for non-generic names.
>
Hi Suzuki,
Can we remove the pattern match of the node name and use a generic name
"ete" for the ete DT nodes ?
Thanks
Jinlong Mao
> Best regards,
> Krzysztof
>