| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1c1092e6-aa5d-4bb1-84e5-81151a02cc6f@linaro.org>
Date: Tue, 9 Dec 2025 13:17:50 +0000
From: James Clark <james.clark@...aro.org>
To: Leo Yan <leo.yan@....com>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
Mike Leach <mike.leach@...aro.org>,
Anshuman Khandual <anshuman.khandual@....com>,
Yeoreum Yun <yeoreum.yun@....com>, Will Deacon <will@...nel.org>,
Mark Rutland <mark.rutland@....com>, Tamas Petz <tamas.petz@....com>,
Tamas Zsoldos <tamas.zsoldos@....com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>,
Suzuki K Poulose <suzuki.poulose@....com>
Subject: Re: [PATCH 06/19] coresight: trbe: Refactor syndrome decoding
On 02/12/2025 11:06 am, Suzuki K Poulose wrote:
> Hi Leo
>
> A couple of minor nits below. Otherwise looks like a nice cleanup
>
>
> On 01/12/2025 11:21, Leo Yan wrote:
>> It gives priority to TRBSR_EL1.EA (external abort); an external abort
>> will immediately bail out and return an error.
>>
>> Next, the syndrome decoding is refactored based on two levels of
>> information: the EC (Event Class) bits and the BSC (Trace Buffer Status
>> Code) bits.
>>
>> If TRBSR_EL1.EC==0b000000, the driver continues parsing TRBSR_EL1.BSC to
>> identify the specific trace buffer event. Otherwise, any non-zero
>> TRBSR_EL1.EC is treated as an error.
>>
>> For error cases, the driver prints an error string and dumps registers
>> for debugging.
>
> minor nit: Please avoid describing the code, which cod does well.
> Briefly mention what you are doing, giving any additional contexts
> that may not be evident from the code.
>
> e.g:
>
> "Add support for decoding the syndrome for TRBE, providing a verbose
> description of the code for Fatal/unhandled events.
> While at it add the new definitions from the latest Arm ARM for
> EC and BSC"
>
>
>
>
>>
>> No additional checks are required for wrap mode beyond verifying the
>> TRBSR_EL1.WRAP bit, even on units with overwrite errata, as this bit
>
>
>> reliably indicates a buffer wrap.
>>
>> Signed-off-by: Leo Yan <leo.yan@....com>
>> ---
>> drivers/hwtracing/coresight/coresight-trbe.c | 60 ++++++++++++++++++
>> +++-------
>> drivers/hwtracing/coresight/coresight-trbe.h | 8 ++--
>> 2 files changed, 51 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/
>> hwtracing/coresight/coresight-trbe.c
>> index
>> 9e565122816949b37b8ff5e4ba04cfbc317c6f25..28e2bfa68074f19ccaa4a737d00af577aea818fe 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -643,29 +643,61 @@ static void trbe_enable_hw(struct trbe_buf *buf)
>> static enum trbe_fault_action trbe_get_fault_act(struct
>> perf_output_handle *handle,
>> u64 trbsr)
>> {
>> + const char *err_str;
>> int ec = get_trbe_ec(trbsr);
>> int bsc = get_trbe_bsc(trbsr);
>> - struct trbe_buf *buf = etm_perf_sink_config(handle);
>> - struct trbe_cpudata *cpudata = buf->cpudata;
>> WARN_ON(is_trbe_running(trbsr));
>> - if (is_trbe_trg(trbsr) || is_trbe_abort(trbsr))
>> - return TRBE_FAULT_ACT_FATAL;
>> - if ((ec == TRBE_EC_STAGE1_ABORT) || (ec == TRBE_EC_STAGE2_ABORT))
>> - return TRBE_FAULT_ACT_FATAL;
>> + if (is_trbe_abort(trbsr)) {
>> + err_str = "External abort";
>> + goto out_fatal;
>> + }
>> - /*
>> - * If the trbe is affected by TRBE_WORKAROUND_OVERWRITE_FILL_MODE,
>> - * it might write data after a WRAP event in the fill mode.
>> - * Thus the check TRBPTR == TRBBASER will not be honored.
>> - */
>> - if ((is_trbe_wrap(trbsr) && (ec == TRBE_EC_OTHERS) && (bsc ==
>> TRBE_BSC_FILLED)) &&
>> - (trbe_may_overwrite_in_fill_mode(cpudata) ||
>> - get_trbe_write_pointer() == get_trbe_base_pointer()))
>> + switch (ec) {
>> + case TRBE_EC_OTHERS:
>> + break;
>> + case TRBE_EC_BUF_MGMT_IMPL:
>> + err_str = "Unexpected implemented management";
>
> minor nit: "Unexpected IMPDEF buffer management event"
>
>
> Suzuki
>
>> + goto out_fatal;
>> + case TRBE_EC_GP_CHECK_FAULT:
>> + err_str = "Granule Protection Check fault";
>> + goto out_fatal;
>> + case TRBE_EC_STAGE1_ABORT:
>> + err_str = "Stage 1 data abort";
>> + goto out_fatal;
>> + case TRBE_EC_STAGE2_ABORT:
>> + err_str = "Stage 2 data abort";
>> + goto out_fatal;
>> + default:
>> + err_str = "Unknown error code";
>> + goto out_fatal;
>> + }
>> +
>> + switch (bsc) {
>> + case TRBE_BSC_NOT_STOPPED:
>> + break;
>> + case TRBE_BSC_FILLED:
>> + break;
>> + case TRBE_BSC_TRIGGERED:
>> + err_str = "Unexpected trigger status";
>> + goto out_fatal;
>> + default:
>> + err_str = "Unexpected buffer status code";
>> + goto out_fatal;
>> + }
>> +
>> + if (is_trbe_wrap(trbsr))
>> return TRBE_FAULT_ACT_WRAP;
>> return TRBE_FAULT_ACT_SPURIOUS;
>> +
>> +out_fatal:
>> + pr_err_ratelimited("%s on CPU %d [TRBSR=0x%016llx,
>> TRBPTR=0x%016llx, TRBLIMITR=0x%016llx]\n",
>> + err_str, smp_processor_id(), trbsr,
>> + read_sysreg_s(SYS_TRBPTR_EL1),
>> + read_sysreg_s(SYS_TRBLIMITR_EL1));
Don't you need to use %p to not leak pointers?
>> + return TRBE_FAULT_ACT_FATAL;
>> }
>> static unsigned long trbe_get_trace_size(struct perf_output_handle
>> *handle,
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.h b/drivers/
>> hwtracing/coresight/coresight-trbe.h
>> index
>> 45202c48accec7c86ba56130e2737bc2d1830fae..d7f7cd763c0c7139cf322b7336ee563073e3bea0 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.h
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.h
>> @@ -35,9 +35,11 @@ static inline bool is_trbe_enabled(void)
>> return trblimitr & TRBLIMITR_EL1_E;
>> }
>> -#define TRBE_EC_OTHERS 0
>> -#define TRBE_EC_STAGE1_ABORT 36
>> -#define TRBE_EC_STAGE2_ABORT 37
>> +#define TRBE_EC_OTHERS 0x0
>> +#define TRBE_EC_GP_CHECK_FAULT 0X1e
>> +#define TRBE_EC_BUF_MGMT_IMPL 0x1f
>> +#define TRBE_EC_STAGE1_ABORT 0x24
>> +#define TRBE_EC_STAGE2_ABORT 0x25
I know some were already defined here, but this should probably be an
enum in sysreg.
>> static inline int get_trbe_ec(u64 trbsr)
>> {
>>
>
Powered by blists - more mailing lists