lkml.org 
[lkml]   [2020]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH v2] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
    From
    Date
    On 11/13/20 10:18 AM, Qi Liu wrote:
    >
    >
    > On 2020/11/12 22:03, Suzuki K Poulose wrote:
    >> On 11/12/20 1:09 PM, Qi Liu wrote:
    >>>
    >>>
    >>> On 2020/11/12 1:03, Mathieu Poirier wrote:
    >>>> On Wed, Nov 11, 2020 at 04:58:23PM +0800, Qi Liu wrote:
    >>>>> Hi Mathieu,
    >>>>>
    >>>>> On 2020/9/10 0:26, Mathieu Poirier wrote:
    >>>>>> On Wed, Sep 09, 2020 at 12:30:02PM +0100, Mike Leach wrote:
    >>>>>>> Hi,
    >>>>>>>
    >>>>>>> On Wed, 2 Sep 2020 at 11:36, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
    >>>>>>>> On 08/27/2020 09:44 PM, Mathieu Poirier wrote:
    >>>>>>>>> Hi Liu,
    >>>>>>>>>
    >>>>>>>>> On Wed, Aug 19, 2020 at 04:06:37PM +0800, Qi Liu wrote:
    >>>>>>>>>> When too much trace information is generated on-chip, the ETM will
    >>>>>>>>>> overflow, and cause data loss. This is a common phenomenon on ETM
    >>>>>>>>>> devices.
    >>>>>>>>>>
    >>>>>>>>>> But sometimes we do not want to lose performance trace data, so we
    >>>>>>>>>> suppress the speed of instructions sent from CPU core to ETM to
    >>>>>>>>>> avoid the overflow of ETM.
    >>>>>>>>>>
    >>>>>>>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
    >>>>>>>>>> ---
    >>>>>>>>>>
    >>>>>>>>>> Changes since v1:
    >>>>>>>>>> - ETM on HiSilicon Hip09 platform supports backpressure, so does
    >>>>>>>>>> not need to modify core commit.
    >>>>>>>>>>
    >>>>>>>>>> drivers/hwtracing/coresight/coresight-etm4x.c | 43 +++++++++++++++++++++++++++
    >>>>>>>>>> 1 file changed, 43 insertions(+)
    >>>>>>>>>>
    >>>>>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
    >>>>>>>>>> index 7797a57..7641f89 100644
    >>>>>>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
    >>>>>>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
    >>>>>>>>>> @@ -43,6 +43,10 @@ MODULE_PARM_DESC(boot_enable, "Enable tracing on boot");
    >>>>>>>>>> #define PARAM_PM_SAVE_NEVER 1 /* never save any state */
    >>>>>>>>>> #define PARAM_PM_SAVE_SELF_HOSTED 2 /* save self-hosted state only */
    >>>>>>>>>>
    >>>>>>>>>> +#define CORE_COMMIT_CLEAR 0x3000
    >>>>>>>>>> +#define CORE_COMMIT_SHIFT 12
    >>>>>>>>>> +#define HISI_ETM_AMBA_ID_V1 0x000b6d01
    >>>>>>>>>> +
    >>>>>>>>>> static int pm_save_enable = PARAM_PM_SAVE_FIRMWARE;
    >>>>>>>>>> module_param(pm_save_enable, int, 0444);
    >>>>>>>>>> MODULE_PARM_DESC(pm_save_enable,
    >>>>>>>>>> @@ -104,11 +108,40 @@ struct etm4_enable_arg {
    >>>>>>>>>> int rc;
    >>>>>>>>>> };
    >>>>>>>>>>
    >>>>>>>>>> +static void etm4_cpu_actlr1_cfg(void *info)
    >>>>>>>>>> +{
    >>>>>>>>>> + struct etm4_enable_arg *arg = (struct etm4_enable_arg *)info;
    >>>>>>>>>> + u64 val;
    >>>>>>>>>> +
    >>>>>>>>>> + asm volatile("mrs %0,s3_1_c15_c2_5" : "=r"(val));
    >>>>>>>>
    >>>>>>>> The ID register (S3_1_C15_c2_5) falls into implementation defined space.
    >>>>>>>> See Arm ARM DDI 0487F.a, section "D12.3.2 Reserved encodings for
    >>>>>>>> IMPLEMENTATION DEFINED registers".
    >>>>>>>>
    >>>>>>>> So, please stop calling this "etm4_cpu_actlr1_cfg". Since,
    >>>>>>>> 1) actlr1 is not an architected name for the said encoding
    >>>>>>>> 2) The id register could mean something else on another CPU.
    >>>>>>>>
    >>>>>>>> Rather this should indicate platform/CPU specific. e.g,
    >>>>>>>>
    >>>>>>>> etm4_cpu_hisilicon_config_core_commit()
    >>>>>>>>
    >>>>>>>>
    >>>>>>>>>> + val &= ~CORE_COMMIT_CLEAR;
    >>>>>>>>>> + val |= arg->rc << CORE_COMMIT_SHIFT;
    >>>>>>>>>> + asm volatile("msr s3_1_c15_c2_5,%0" : : "r"(val));
    >>>>>>>>>> +}
    >>>>>>>>>> +
    >>>>>>>>>> +static void etm4_config_core_commit(int cpu, int val)
    >>>>>>>>>> +{
    >>>>>>>>>> + struct etm4_enable_arg arg = {0};
    >>>>>>>>>> +
    >>>>>>>>>> + arg.rc = val;
    >>>>>>>>>> + smp_call_function_single(cpu, etm4_cpu_actlr1_cfg, &arg, 1);
    >>>>>>>>> Function etm4_enable/disable_hw() are already running on the CPU they are
    >>>>>>>>> supposed to so no need to call smp_call_function_single().
    >>>>>>>>>
    >>>>>>>>>> +}
    >>>>>>>>>> +
    >>>>>>>>>> static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
    >>>>>>>>>> {
    >>>>>>>>>> int i, rc;
    >>>>>>>>>> + struct amba_device *adev;
    >>>>>>>>>> struct etmv4_config *config = &drvdata->config;
    >>>>>>>>>> struct device *etm_dev = &drvdata->csdev->dev;
    >>>>>>>>>> + struct device *dev = drvdata->csdev->dev.parent;
    >>>>>>>>>> +
    >>>>>>>>>> + adev = container_of(dev, struct amba_device, dev);
    >>>>>>>>>> + /*
    >>>>>>>>>> + * If ETM device is HiSilicon ETM device, reduce the
    >>>>>>>>>> + * core-commit to avoid ETM overflow.
    >>>>>>>>>> + */
    >>>>>>>>>> + if (adev->periphid == HISI_ETM_AMBA_ID_V1)
    >>>>>>>> Please could you move this check to the function above ?
    >>>>>>>> Ideally, it would be good to have something like :
    >>>>>>>>
    >>>>>>>> etm4_config_impdef_features();
    >>>>>>>>
    >>>>>>>> And :
    >>>>>>>>
    >>>>>>>> etm4_config_impdef_features(struct etm4_drvdata *drvdata)
    >>>>>>>> {
    >>>>>>>> etm4_cpu_hisilicon_config_core_commit(drvdata);
    >>>>>>>> }
    >>>>>>>>
    >>>>>>> In addition to the above, Is it worth having this implementation
    >>>>>>> defined code gated in the kernel configuration - like we do for core
    >>>>>>> features sometimes?
    >>>>>>> i,.e.
    >>>>>>> CONFIG_ETM4X_IMPDEF_FEATURE (controls overall impdef support in the driver)
    >>>>>>> and
    >>>>>>> CONFIG_ETM4X_IMPDEF_HISILICON (depends on CONFIG_ETM4X_IMPDEF_FEATURE )
    >>>>>>>
    >>>>>>> This way we keep non ETM architectural code off platforms that cannot
    >>>>>>> use it / test it.
    >>>>>>>
    >>>>>> That's a good idea - they do the same for CPU erratas.
    >>>>>>
    >>>>> Considering that users sometimes use the same set of code on different platforms, how about
    >>>>> using both CONFIG andperiphid to keep core-commit code off the platforms that do not
    >>>>> need it?
    >>>>> i, .e.
    >>>>> CONFIG_ETM4X_IMPDEF_FEATURE ( controls overall impdef support in the driver )
    >>>>> periphid ( match impdef code with the target platform )
    >>>>>
    >>>>> This way we could keep the same set of code working on different platforms, and it could help to
    >>>>> ensure compatibility.
    >>>>
    >>>> I'm not 100% sure of what you mean by "same set of code working on different
    >>>> platforms"... Up to know the way we have been handling peripheral IDs has
    >>>> worked quite well and I don't intend on changing it unless there is a really
    >>>> good reason.
    >>>>
    >>> Hi Mathieu,
    >>>
    >>> Perhaps I didn't make it clear and here is the code to express what I mean:
    >>>
    >>> #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
    >>>
    >>> #define HISI_HIP08_AMBA_ID 0x000b6d01
    >>> #define HISI_HIP08_AMBA_MASK 0xfffff
    >>> static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
    >>> {
    >>> struct device *dev = drvdata->csdev->dev.parent;
    >>> struct amba_device *adev = container_of(dev, struct amba_device, dev);
    >>
    >> There is no guarantee that this is always an "AMBA" device, with this
    >> patchset (which is still under review). Also, doing this check at
    >> every time we enable/disable the ETM is not idea..
    >>
    >> May be we should add a concept of "features" and use a bit mask instead,
    >> which can be set at probe time, where we do have this information.
    >>
    >> #define ETM4x_IMPDEF_HISILICON_CORE_COMMIT 0
    >> #define ETM4x_IMPDEF_ARCH_N_FEATS 1
    >>
    >> struct etmv4_drvdata {
    >>
    >> ...
    >> DECALRE_BITMAP(arch_features, ETM4x_IMPDEF_ARCH_FEAT_MAX);
    >> }
    >>
    >> struct etm4x_arch_feature {
    >> void (*callback)(struct etmv4_drdvata *, bool enable);
    >> };
    >>
    >> static struct etm4x_arch_features[] = {
    >> [ETM4x_IMPDEF_HISILICON_CORE_COMMIT] = {
    >> .callback = etm4x_hisilicon_core_commit,
    >> },
    >> {}
    >> };
    >>
    >> static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
    >> {
    >> for_each_set_bit(i, &drvdata->arch_features) {
    >> struct etm4x_arch_feature *ftr = &etm4x_arch_features[i];
    >>
    >> if (ftr->callback)
    >> ftr->callback(drvdata, true);
    >> }
    >> }
    >>
    >> etm4x_check_arch_features() {
    >> if (hisilicon_etm4x_match_pid)
    >> set_bit(drvdata->arch_features, ETM4x_IMPDEF_HISILICON_CORE_COMMIT);
    >> }
    >>
    >> etm4x_probe() {
    >>
    >> etm4x_check_arch_features();
    >> }
    >>
    >> Suzuki
    >
    > Hi Suzuki,
    >
    > Add a check in probe is a good idea, and perhaps we could also add CONFIG_ETM4X_IMPDEF_FEATURE
    > here, like:
    > etm4x_probe() {
    > ...
    > #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
    > etm4x_check_arch_features();
    > #endif

    Please make this unconditional and define the function conditionally, as below.


    etm4x_check_arch_features();
    > }

    and :

    #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
    static void etm4x_check_arch_features(struct etmv4_drvdata *drvdata, struct device *dev)
    {

    }

    static void etm4x_enable_arch_specific(...)
    {
    .... do arch specific setup ...
    }

    #else
    static inline void etm4x_check_arch_features(..)
    {}
    static void etm4x_enable_arch_specific(...)
    {}
    #endif


    >
    > Thanks
    > Qi
    >>
    >> .
    >>
    >

    \
     
     \ /
      Last update: 2020-11-13 11:23    [W:3.168 / U:2.360 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site