| 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: <fa21ee02-a000-9d39-2e0c-850f63354272@arm.com>
Date: Thu, 5 Nov 2020 22:47:41 +0000
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: linux-arm-kernel@...ts.infradead.org, mike.leach@...aro.org,
coresight@...ts.linaro.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 14/26] coresight: etm4x: Add sysreg access helpers
Hi Mathieu,
On 11/5/20 8:52 PM, Mathieu Poirier wrote:
> On Wed, Oct 28, 2020 at 10:09:33PM +0000, Suzuki K Poulose wrote:
>> ETMv4.4 architecture defines the system instructions for accessing
>> ETM via register accesses. Add basic support for accessing a given
>> register via system instructions.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
>> Cc: Mike Leach <mike.leach@...aro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>> ---
>> .../coresight/coresight-etm4x-core.c | 39 ++
>> drivers/hwtracing/coresight/coresight-etm4x.h | 348 ++++++++++++++++--
>> 2 files changed, 365 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 4af7d45dfe63..90b80982c615 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -56,6 +56,45 @@ static u64 etm4_get_access_type(struct etmv4_config *config);
>>
>> static enum cpuhp_state hp_online;
>>
>> +u64 etm4x_sysreg_read(struct csdev_access *csa,
>> + u32 offset,
>> + bool _relaxed,
>> + bool _64bit)
>
> Please fix the stacking.
>
Sure.
>> +
>> +void etm4x_sysreg_write(struct csdev_access *csa,
>> + u64 val,
>> + u32 offset,
>> + bool _relaxed,
>> + bool _64bit)
>
> Here too.
Sure.
>> /* Writing 0 to TRCOSLAR unlocks the trace registers */
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index 510828c73db6..5cf71b30a652 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.
>> +
>> +#define ETM4x_READ_CASES(res) CASE_LIST(READ, (res))
>> +#define ETM4x_WRITE_CASES(val) CASE_LIST(WRITE, (val))
>> +
>> +#define read_etm4x_sysreg_offset(csa, offset, _64bit) \
>> + ({ \
>> + u64 __val; \
>> + \
>> + if (__builtin_constant_p((offset))) \
>
> Neat trick - I wonder how you stumbled on that one.
>
:-). There are plenty of uses in the kernel and glibc.
>
>> + __val = read_etm4x_sysreg_const_offset((offset)); \
>> + else \
>> + __val = etm4x_sysreg_read((csa), (offset), \
>> + true, _64bit); \
>> + __val; \
>> + })
>> +
>> +#define write_etm4x_sysreg_offset(csa, val, offset, _64bit) \
>> + do { \
>> + if (__builtin_constant_p((offset))) \
>> + write_etm4x_sysreg_const_offset((val), \
>> + (offset)); \
>> + else \
>> + etm4x_sysreg_write((csa), (val), (offset), \
>> + true, _64bit); \
>> + } while (0)
>> +
>> +
>> +#define etm4x_relaxed_read32(csa, offset) \
>> + ((u32)((csa)->io_mem ? \
>> + readl_relaxed((csa)->base + (offset)) : \
>> + read_etm4x_sysreg_offset((csa), (offset), false)))
>
> Please add an extra new line - otherwise it is very hard to read.
>
Sure
>> +#define etm4x_relaxed_read64(csa, offset) \
>> + ((u64)((csa)->io_mem ? \
>> + readq_relaxed((csa)->base + (offset)) : \
>> + read_etm4x_sysreg_offset((csa), (offset), true)))
>
> Here too.
>
sure
>> +#define etm4x_read32(csa, offset) \
>> + ({ \
>> + u32 __val = etm4x_relaxed_read32((csa), (offset)); \
>> + __iormb(__val); \
>> + __val; \
>> + })
>> +
>> +#define etm4x_read64(csa, offset) \
>> + ({ \
>> + u64 __val = etm4x_relaxed_read64((csa), (offset)); \
>> + __iormb(__val); \
>> + __val; \
>> + })
>> +
>> +#define etm4x_relaxed_write32(csa, val, offset) \
>> + do { \
>> + if ((csa)->io_mem) \
>> + writel_relaxed((val), (csa)->base + (offset)); \
>> + else \
>> + write_etm4x_sysreg_offset((csa), (val), \
>> + (offset), false); \
>
> Why using an if/else statement and above the '?' condition marker? I would
> really like a uniform approach. Otherwise the reader keeps looking for
> something subtle when there isn't.
The write variants do not return a result, unlike the read.
So, we cant use the '?'
>
>> + } while (0)
>> +
>> +#define etm4x_relaxed_write64(csa, val, offset) \
>> + do { \
>> + if ((csa)->io_mem) \
>> + writeq_relaxed((val), (csa)->base + (offset)); \
>> + else \
>> + write_etm4x_sysreg_offset((csa), (val), \
>> + (offset), true); \
>> + } while (0)
>> +
>> +#define etm4x_write32(csa, val, offset) \
>> + do { \
>> + __iowmb(); \
>> + etm4x_relaxed_write32((csa), (val), (offset)); \
>> + } while (0)
>> +
>> +#define etm4x_write64(csa, val, offset) \
>> + do { \
>> + __iowmb(); \
>> + etm4x_relaxed_write64((csa), (val), (offset)); \
>> + } while (0)
>>
>> -#define etm4x_write64(csa, val, offset) \
>> - writeq((val), (csa)->base + (offset))
>>
>> /* ETMv4 resources */
>> #define ETM_MAX_NR_PE 8
>> @@ -512,4 +806,14 @@ enum etm_addr_ctxtype {
>>
>> extern const struct attribute_group *coresight_etmv4_groups[];
>> void etm4_config_trace_mode(struct etmv4_config *config);
>> +
>> +u64 etm4x_sysreg_read(struct csdev_access *csa,
>> + u32 offset,
>> + bool _relaxed,
>> + bool _64bit);
>> +void etm4x_sysreg_write(struct csdev_access *csa,
>> + u64 val,
>> + u32 offset,
>> + bool _relaxed,
>> + bool _64bit);
>
> With the above:
>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@...aro.org>
Thanks !
>
> This patch holds together well. I commend you on rendering something that is
> quite complex into a manageable implementation. That being said it will impact
> Mike's complex configuration patchset (or Mike's complex configuration patchset
> will have an impact on this).
I understand. Will see when we get to it.
Cheers
Suzuki
Powered by blists - more mailing lists