| 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: <919ef281-6ef2-8557-c8a3-e4eea452694f@arm.com>
Date: Tue, 7 Sep 2021 10:04:37 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Anshuman Khandual <anshuman.khandual@....com>,
linux-arm-kernel@...ts.infradead.org
Cc: linux-kernel@...r.kernel.org, coresight@...ts.linaro.org,
will@...nel.org, catalin.marinas@....com, james.morse@....com,
mathieu.poirier@...aro.org, mike.leach@...aro.org,
leo.yan@...aro.org, maz@...nel.org, mark.rutland@....com
Subject: Re: [PATCH 01/10] coresight: trbe: Add infrastructure for Errata
handling
On 02/08/2021 07:43, Anshuman Khandual wrote:
>
>
> On 7/28/21 7:22 PM, Suzuki K Poulose wrote:
>> Add a minimal infrastructure to keep track of the errata
>> affecting the given TRBE instance. Given that we have
>> heterogeneous CPUs, we have to manage the list per-TRBE
>> instance to be able to apply the work around as needed.
>>
>> We rely on the arm64 errata framework for the actual
>> description and the discovery of a given erratum, to
>> keep the Erratum work around at a central place and
>> benefit from the code and the advertisement from the
>> kernel. We use a local mapping of the erratum to
>> avoid bloating up the individual TRBE structures.
>
> I guess there is no other way around apart from each TRBE instance
> tracking applicable erratas locally per CPU, even though it sounds
> bit redundant.
>
>> i.e, each arm64 TRBE erratum bit is assigned a new number
>> within the driver to track. Each trbe instance updates
>> the list of affected erratum at probe time on the CPU.
>> This makes sure that we can easily access the list of
>> errata on a given TRBE instance without much overhead.
>
> It also ensures that the generic errata framework is queried just
> once during individual CPU probe.
>
>>
>> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
>> Cc: Mike Leach <mike.leach@...aro.org>
>> Cc: Leo Yan <leo.yan@...aro.org>
>> Cc: Anshuman Khandual <anshuman.khandual@....com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>> ---
>> drivers/hwtracing/coresight/coresight-trbe.c | 48 ++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
>> index b8586c170889..0368bf405e35 100644
>> --- a/drivers/hwtracing/coresight/coresight-trbe.c
>> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
>> @@ -16,6 +16,8 @@
>> #define pr_fmt(fmt) DRVNAME ": " fmt
>>
>> #include <asm/barrier.h>
>> +#include <asm/cputype.h>
>> +
>> #include "coresight-self-hosted-trace.h"
>> #include "coresight-trbe.h"
>>
>> @@ -65,6 +67,35 @@ struct trbe_buf {
>> struct trbe_cpudata *cpudata;
>> };
>>
>> +/*
>> + * TRBE erratum list
>> + *
>> + * We rely on the corresponding cpucaps to be defined for a given
>> + * TRBE erratum. We map the given cpucap into a TRBE internal number
>> + * to make the tracking of the errata lean.
>> + *
>> + * This helps in :
>> + * - Not duplicating the detection logic
>> + * - Streamlined detection of erratum across the system
>> + *
>> + * Since the erratum work arounds could be applied individually
>> + * per TRBE instance, we keep track of the list of errata that
>> + * affects the given instance of the TRBE.
>> + */
>> +#define TRBE_ERRATA_MAX 0
>> +
>> +static unsigned long trbe_errata_cpucaps[TRBE_ERRATA_MAX] = {
>> +};
>
> This needs to be tighten up. There should be build time guard rails in
> arm64 errata cpucaps, so that only TRBE specific ones could be assigned
> here as trbe_errata_cpucaps[].
I don't get your point. The actual arm64 erratum caps are not linear
and as such we don't have to force it. This approach gives us a hand
picked exact list of errata that apply to the TRBE driver by mapping
it linearly here. The only reason why we have that TRBE_ERRATA_MAX,
is such that we can track it per TRBE instance and ...
>
>> +
>> +/*
>> + * struct trbe_cpudata: TRBE instance specific data
>> + * @trbe_flag - TRBE dirty/access flag support
>> + * @tbre_align - Actual TRBE alignment required for TRBPTR_EL1.
>> + * @cpu - CPU this TRBE belongs to.
>> + * @mode - Mode of current operation. (perf/disabled)
>> + * @drvdata - TRBE specific drvdata
>> + * @errata - Bit map for the errata on this TRBE.
>> + */
>> struct trbe_cpudata {
>> bool trbe_flag;
>> u64 trbe_align;
>> @@ -72,6 +103,7 @@ struct trbe_cpudata {
>> enum cs_mode mode;
>> struct trbe_buf *buf;
>> struct trbe_drvdata *drvdata;
>> + DECLARE_BITMAP(errata, TRBE_ERRATA_MAX);
>> };
>>
>> struct trbe_drvdata {
>> @@ -84,6 +116,21 @@ struct trbe_drvdata {
>> struct platform_device *pdev;
>> };
>>
>> +static void trbe_check_errata(struct trbe_cpudata *cpudata)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(trbe_errata_cpucaps); i++) {
>
> BUILD_BUG_ON() - if trbe_errata_cpucaps[i] is not inside TRBE specific
> errata cpucap range ?
... also run these detection tests.
>
>> + if (this_cpu_has_cap(trbe_errata_cpucaps[i]))
>> + set_bit(i, cpudata->errata);
>> + }
>> +}
>> +
>> +static inline bool trbe_has_erratum(int i, struct trbe_cpudata *cpudata)
>
> Switch the argument positions here ? 'int i' should be the second one.
>
ok.
>> +{
>> + return (i < TRBE_ERRATA_MAX) && test_bit(i, cpudata->errata);
>> +}
>> +
>> static int trbe_alloc_node(struct perf_event *event)
>> {
>> if (event->cpu == -1)
>> @@ -925,6 +972,7 @@ static void arm_trbe_probe_cpu(void *info)
>> goto cpu_clear;
>> }
>>
>> + trbe_check_errata(cpudata);
>
> This should be called right at the end before arm_trbe_probe_cpu() exits
> on the success path. Errata should not be evaluated if TRBE on the CPU
> wont be used for some reason i.e cpumask_clear_cpu() path.
ok
>
>> cpudata->trbe_align = 1ULL << get_trbe_address_align(trbidr);
>> if (cpudata->trbe_align > SZ_2K) {
>> pr_err("Unsupported alignment on cpu %d\n", cpu);
>>
>
> This patch should be moved after [PATCH 5/10] i.e just before adding the
> first TRBE errata.
>
I will take a look.
Thanks for the review
Suzuki
Powered by blists - more mailing lists