linux-kernel - Re: [PATCH] coresight: always use stashed trace id value in etm4_trace

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57697A36.3010105@arm.com>
Date:	Tue, 21 Jun 2016 18:32:38 +0100
From:	Sudeep Holla <sudeep.holla@....com>
To:	Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:	Sudeep Holla <sudeep.holla@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Suzuki Poulose <Suzuki.Poulose@....com>
Subject: Re: [PATCH] coresight: always use stashed trace id value in
 etm4_trace_id



On 21/06/16 18:05, Mathieu Poirier wrote:
> On 20 June 2016 at 08:25, Sudeep Holla <sudeep.holla@....com> wrote:
>> etm4_trace_id is not guaranteed to be executed on the CPU whose ETM is
>> being accessed. This leads to exception similar to below one if the
>> CPU whose ETM is being accessed is in deeper idle states. So it must
>> be executed on the CPU whose ETM is being accessed.
>>
>> Unhandled fault: synchronous external abort (0x96000210) at 0xffff000008db4040
>> Internal error: : 96000210 [#1] PREEMPT SMP
>> Modules linked in:
>> CPU: 5 PID: 5979 Comm: etm.sh Not tainted 4.7.0-rc3 #159
>> Hardware name: ARM Juno development board (r2) (DT)
>> task: ffff80096dd34b00 ti: ffff80096dfe4000 task.ti: ffff80096dfe4000
>> PC is at etm4_trace_id+0x5c/0x90
>> LR is at etm4_trace_id+0x3c/0x90
>> Call trace:
>>   etm4_trace_id+0x5c/0x90
>>   coresight_id_match+0x78/0xa8
>>   bus_for_each_dev+0x60/0xa0
>>   coresight_enable+0xc0/0x1b8
>>   enable_source_store+0x3c/0x70
>>   dev_attr_store+0x18/0x28
>>   sysfs_kf_write+0x48/0x58
>>   kernfs_fop_write+0x14c/0x1e0
>>   __vfs_write+0x1c/0x100
>>   vfs_write+0xa0/0x1b8
>>   SyS_write+0x44/0xa0
>>   el0_svc_naked+0x24/0x28
>>
>> However, TRCTRACEIDR is not guaranteed to hold the previous programmed
>> trace id if it enters deeper idle states. Further, the trace id that is
>> computed in etm4_init_trace_id is programmed into TRCTRACEIDR only in
>> etm4_enable_hw which happens much later in the sequence after
>> coresight_id_match is executed from enable_source_store.
>>
>> This patch simplifies etm4_trace_id by returning the stashed trace id
>> value similar to etm4_cpu_id.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
>> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm4x.c | 16 +---------------
>>   1 file changed, 1 insertion(+), 15 deletions(-)
>>
>> Hi Mathieu,
>>
>> While trying to support ETM with CPUIdle active, I faced this issue.
>>
>> Regards,
>> Sudeep
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index 43fa3beaa0df..d6f1d6d874eb 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -77,22 +77,8 @@ static int etm4_cpu_id(struct coresight_device *csdev)
>>   static int etm4_trace_id(struct coresight_device *csdev)
>>   {
>>          struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> -       unsigned long flags;
>> -       int trace_id = -1;
>>
>> -       if (!local_read(&drvdata->mode))
>> -               return drvdata->trcid;
>> -
>> -       spin_lock_irqsave(&drvdata->spinlock, flags);
>> -
>> -       CS_UNLOCK(drvdata->base);
>> -       trace_id = readl_relaxed(drvdata->base + TRCTRACEIDR);
>> -       trace_id &= ETM_TRACEID_MASK;
>> -       CS_LOCK(drvdata->base);
>> -
>> -       spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> -
>> -       return trace_id;
>> +       return drvdata->trcid;
>>   }
>
> This code was written prior to the integration with the Perf core.  To
> make sure the correct active value was return to users it goes to the
> HW if the IP is in use.  With the integration with Perf the code was
> moved around and the traceID is no longer called on the CPU (as you
> noticed) and has lost the required PM runtime operation.
>

Is the perf integration already queued ? If not, it good to have this
until then. I see crashes if I disable idle on one core and enable ETM
source on that.

With this I don't see any crashes even if we have CPUIdle enabled.
However the trace collection halts once the core suspends and resumes
back. I have a simple solution for handling CPUIdle too, will post it soon.

-- 
Regards,
Sudeep