| 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: <d82f1200-c31f-1040-a630-57e132cabf1b@arm.com>
Date: Mon, 7 Dec 2020 11:27:16 +0000
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Qi Liu <liuqi115@...wei.com>,
Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: mike.leach@...aro.org, coresight@...ts.linaro.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linuxarm@...wei.com
Subject: Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid
the overflow of HiSilicon ETM
On 12/7/20 11:21 AM, Qi Liu wrote:
>
> Hi Suzuki,
> On 2020/12/7 18:38, Suzuki K Poulose wrote:
>> On 12/7/20 2:08 AM, Qi Liu wrote:
>>> Hi Mathieu,
>>>
>>> On 2020/12/5 2:55, Mathieu Poirier wrote:
>>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>>>> The ETM device can't keep up with the core pipeline when cpu core
>>>>> is at full speed. This may cause overflow within core and its ETM.
>>>>> This is a common phenomenon on ETM devices.
>>>>>
>>>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>>>> overflow.
>>>>>
>>>>> Signed-off-by: Qi Liu <liuqi115@...wei.com>
>>>>> ---
>>>>> Change since v1:
>>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>>> to keep specific feature off platforms which don't use it.
>>>>> Change since v2:
>>>>> - remove some unused variable.
>>>>> Change since v3:
>>>>> - use read/write_sysreg_s() to access register.
>>>>> Change since v4:
>>>>> - rename the call back function to a more generic name, and fix some
>>>>> compile warnings.
>>>>>
>>>>> drivers/hwtracing/coresight/Kconfig | 9 +++
>>>>> drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>>> drivers/hwtracing/coresight/coresight-etm4x.h | 8 ++
>>>>> 3 files changed, 105 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>>>> index c119824..1cc3601 100644
>>>>> --- a/drivers/hwtracing/coresight/Kconfig
>>>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>>> To compile this driver as a module, choose M here: the
>>>>> module will be called coresight-etm4x.
>>>>>
>>>>> +config ETM4X_IMPDEF_FEATURE
>>>>> + bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>>>> + depends on CORESIGHT_SOURCE_ETM4X
>>>>> + help
>>>>> + This control provides overflow implement define for CoreSight
>>>>> + ETM 4.x tracer module which could not reduce commit race
>>>>> + automatically, and could avoid overflow within ETM tracer module
>>>>> + and its cpu core.
>>>>> +
>>>>> config CORESIGHT_STM
>>>>> tristate "CoreSight System Trace Macrocell driver"
>>>>> depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> index abd706b..fcee27a 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> @@ -3,6 +3,7 @@
>>>>> * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>>> */
>>>>>
>>>>> +#include <linux/bitops.h>
>>>>> #include <linux/kernel.h>
>>>>> #include <linux/moduleparam.h>
>>>>> #include <linux/init.h>
>>>>> @@ -28,7 +29,9 @@
>>>>> #include <linux/perf_event.h>
>>>>> #include <linux/pm_runtime.h>
>>>>> #include <linux/property.h>
>>>>> +
>>>>> #include <asm/sections.h>
>>>>> +#include <asm/sysreg.h>
>>>>> #include <asm/local.h>
>>>>> #include <asm/virt.h>
>>>>>
>>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>>> int rc;
>>>>> };
>>>>>
>>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>>>> +
>>>>> +#define HISI_HIP08_AMBA_ID 0x000b6d01
>>>>> +#define ETM4_AMBA_MASK 0xfffff
>>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR 0x3000
>>>>
>>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>>>> is set - is this intentional? What is bit 13 for?
>>>>
>>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
>>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
>>> reduced to minimum value. So bit 12 and 13 should be cleared together in
>>> etm4_hisi_config_core_commit().
>>
>> Please could you document this in the function.
>>
> of course, thanks.
>>>
>>> Qi
>>>
>>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT 12
>>>>> +#define HISI_HIP08_CORE_COMMIT_REG sys_reg(3, 1, 15, 2, 5)
>>>>> +
>>>>> +struct etm4_arch_features {
>>>>> + void (*arch_callback)(bool enable);
>>>>> +};
>>>>> +
>>>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>>>> +{
>>>>> + return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>>>> +}
>>>>> +
>>>>> +static void etm4_hisi_config_core_commit(bool enable)
>>>>> +{
>>>>> + u64 val;
>>>>> +
>>>>> + val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>>>> + val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>>>> + val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
>>
>> I would use the explicitly masked values when you update
>> a register.
>>
> ok, how about changing these code to this:
> val &= ~GENMASK(12, 13);
I would do :
// Rename the HISI_HIP08_CORE_COMMIT_CLEAR to HISI_HIP08_CORE_COMMIT_MASK
// above.
#define HISI_HIP08_CORE_COMMIT_MASK 0x3000
#define HISI_HIP08_CORE_COMMIT_FULL 0b00
#define HISI_HIP08_CORE_COMMIT_LVL_1 0b01
u8 commit = enable ? HISI_HIP08_CORE_COMMIT_LVL_1 : HISI_HIP08_CORE_COMMIT_FULL;
...
val |= commit << HISI_HIP08_CORE_COMMIT_SHIFT;
..
Suzuki
Powered by blists - more mailing lists