From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 99D2FC4167D for ; Thu, 9 Nov 2023 10:15:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ZOQkZRrsxRxN8B8MaD9Gp65mxk7zQ7v+/ELnjXwBNvs=; b=Q98KRU1q/WMdjU kd94qlRL93Ut1gk3buZU8Cf7qkUn96+O6iaggoE5ZcQNwVkN0QMghCKfRefOh4gz28o/yI12eCvzc Z/ZENdFyqCewFiUmqzNWdmEKpv2cZkQrB5OM7lmIQ+WIO4F41DPj/ITbrOdr3/EE5JGnc9CJ2u865 NKOXFr2dPLw9xdWINCZrpuyJ/Dxe2n6SX232C7y2S7m0A4anbatGQE2D8mquhM82URBJSHQ1eeh0G rpUjQRtQDQ5JOBlarYRrqS9D+OVUrdS+6hJtiAZe9Zjb79A+u3ADWbY/LIrOB38x8h/sTzA08oZeT dNmp31sVawpq5/qlsX9Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r124L-005uEq-1e; Thu, 09 Nov 2023 10:15:25 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r124I-005uAN-12 for linux-arm-kernel@lists.infradead.org; Thu, 09 Nov 2023 10:15:24 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9D6F91063; Thu, 9 Nov 2023 02:16:02 -0800 (PST) Received: from [10.1.36.59] (e127643.arm.com [10.1.36.59]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 74F2E3F703; Thu, 9 Nov 2023 02:15:15 -0800 (PST) Message-ID: <1196a091-0af1-1658-338e-dba981d94ec3@arm.com> Date: Thu, 9 Nov 2023 10:15:13 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [EXT] Re: [PATCH 5/7] coresight: tmc: Add support for reading tracedata from previous boot To: Linu Cherian , "suzuki.poulose@arm.com" , "mike.leach@linaro.org" , "leo.yan@linaro.org" Cc: "linux-arm-kernel@lists.infradead.org" , "coresight@lists.linaro.org" , "linux-kernel@vger.kernel.org" , "robh+dt@kernel.org" , "krzysztof.kozlowski+dt@linaro.org" , "conor+dt@kernel.org" , "devicetree@vger.kernel.org" , Sunil Kovvuri Goutham , George Cherian , Anil Kumar Reddy H References: <20230929133754.857678-1-lcherian@marvell.com> <20230929133754.857678-6-lcherian@marvell.com> <9b519a19-548c-dec0-ed69-52695ff12a69@arm.com> Content-Language: en-US From: James Clark In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231109_021522_460824_FD2EED67 X-CRM114-Status: GOOD ( 36.78 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 09/11/2023 01:08, Linu Cherian wrote: > Hi James, > >> -----Original Message----- >> From: Linu Cherian >> Sent: Tuesday, October 10, 2023 6:53 PM >> To: James Clark ; suzuki.poulose@arm.com; >> mike.leach@linaro.org; leo.yan@linaro.org >> Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux- >> kernel@vger.kernel.org; robh+dt@kernel.org; >> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; >> devicetree@vger.kernel.org; Sunil Kovvuri Goutham >> ; George Cherian ; Anil >> Kumar Reddy H >> Subject: RE: [EXT] Re: [PATCH 5/7] coresight: tmc: Add support for reading >> tracedata from previous boot >> >> Hi James, >> >>> -----Original Message----- >>> From: James Clark >>> Sent: Wednesday, October 4, 2023 7:18 PM >>> To: Linu Cherian ; suzuki.poulose@arm.com; >>> mike.leach@linaro.org; leo.yan@linaro.org >>> Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; >>> linux- kernel@vger.kernel.org; robh+dt@kernel.org; >>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; >>> devicetree@vger.kernel.org; Sunil Kovvuri Goutham >>> ; George Cherian ; Anil >>> Kumar Reddy H ; Tanmay Jagdale >>> >>> Subject: [EXT] Re: [PATCH 5/7] coresight: tmc: Add support for reading >>> tracedata from previous boot >>> >>> External Email >>> >>> ---------------------------------------------------------------------- >>> >>> >>> On 03/10/2023 17:43, James Clark wrote: >>>> >>>> >>>> On 29/09/2023 14:37, Linu Cherian wrote: >>>>> * Introduce a new mode CS_MODE_READ_PREVBOOT for reading >>> tracedata >>>>> captured in previous boot. >>>>> >>>>> * Add special handlers for preparing ETR/ETF for this special mode >>>>> >>>>> * User can read the trace data as below >>>>> >>>>> For example, for reading trace data from tmc_etf sink >>>>> >>>>> 1. cd /sys/bus/coresight/devices/tmc_etfXX/ >>>>> >>>>> 2. Change mode to READ_PREVBOOT >>>>> >>>>> #echo 1 > read_prevboot >>>>> >>>>> 3. Dump trace buffer data to a file, >>>>> >>>>> #dd if=/dev/tmc_etrXX of=~/cstrace.bin >>>>> >>>>> 4. Reset back to normal mode >>>>> >>>>> #echo 0 > read_prevboot >>>>> >>>>> Signed-off-by: Anil Kumar Reddy >>>>> Signed-off-by: Tanmay Jagdale >>>>> Signed-off-by: Linu Cherian >>>>> --- >>>>> .../coresight/coresight-etm4x-core.c | 1 + >>>>> .../hwtracing/coresight/coresight-tmc-core.c | 81 +++++++++- >>>>> .../hwtracing/coresight/coresight-tmc-etf.c | 62 ++++++++ >>>>> .../hwtracing/coresight/coresight-tmc-etr.c | 145 >> +++++++++++++++++- >>>>> drivers/hwtracing/coresight/coresight-tmc.h | 6 + >>>>> include/linux/coresight.h | 13 ++ >>>>> 6 files changed, 306 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>>> b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>>> index 77b0271ce6eb..513baf681280 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>>>> @@ -1010,6 +1010,7 @@ static void etm4_disable(struct >>>>> coresight_device *csdev, >>>>> >>>>> switch (mode) { >>>>> case CS_MODE_DISABLED: >>>>> + case CS_MODE_READ_PREVBOOT: >>>>> break; >>>>> case CS_MODE_SYSFS: >>>>> etm4_disable_sysfs(csdev); >>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>> b/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>> index 6658ce76777b..65c15c9f821b 100644 >>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c >>>>> @@ -103,6 +103,45 @@ u32 tmc_get_memwidth_mask(struct >>> tmc_drvdata *drvdata) >>>>> return mask; >>>>> } >>>>> >>>>> +int tmc_read_prepare_prevboot(struct tmc_drvdata *drvdata) { >>>>> + int ret = 0; >>>>> + struct tmc_register_snapshot *reg_ptr; >>>>> + struct coresight_device *csdev = drvdata->csdev; >>>>> + >>>>> + if (!drvdata->metadata.vaddr) { >>>>> + ret = -ENOMEM; >>>>> + goto out; >>>>> + } >>>>> + >>>>> + reg_ptr = drvdata->metadata.vaddr; >>>>> + if (!reg_ptr->valid) { >>>>> + dev_err(&drvdata->csdev->dev, >>>>> + "Invalid metadata captured from previous boot\n"); >>>>> + ret = -EINVAL; >>>>> + goto out; >>>>> + } >>>> >>>> I'm wondering if a more robust check is needed than the valid flag, >>>> like a checksum or something. I didn't debug it yet but I ended up >>>> with an invalid set of metadata after a panic reboot, see below. I'm >>>> not sure if it's just a logic bug or something got lost during the >>>> reboot, I didn't debug it yet. But I suppose unless you assume the >>>> panic didn't affect writing the metadata, then it could be partially >>>> written and shouldn't be trusted? >>>> >>>> [...] >>>>> + >>>>> +static int tmc_etr_sync_prevboot_buf(struct tmc_drvdata *drvdata) { >>>>> + u32 status; >>>>> + u64 rrp, rwp, dba; >>>>> + struct tmc_register_snapshot *reg_ptr; >>>>> + struct etr_buf *etr_buf = drvdata->prevboot_buf; >>>>> + >>>>> + reg_ptr = drvdata->metadata.vaddr; >>>>> + >>>>> + rrp = reg_ptr->rrp; >>>>> + rwp = reg_ptr->rwp; >>>>> + dba = reg_ptr->dba; >>>>> + status = reg_ptr->sts; >>>>> + >>>>> + etr_buf->full = !!(status & TMC_STS_FULL); >>>>> + >>>>> + /* Sync the buffer pointers */ >>>>> + etr_buf->offset = rrp - dba; >>>>> + if (etr_buf->full) >>>>> + etr_buf->len = etr_buf->size; >>>>> + else >>>>> + etr_buf->len = rwp - rrp; >>>>> + >>>>> + /* Sanity checks for validating metadata */ >>>>> + if ((etr_buf->offset > etr_buf->size) || >>>>> + (etr_buf->len > etr_buf->size)) >>>>> + return -EINVAL; >>>> >>>> The values I got here are 0x781b67182aa346f9 0x8000000 0x8000000 for >>>> offset, size and len respectively. This fails the first check. It >>>> would also be nice to have a dev_dbg here as well, it's basically >>>> the same as the valid check above which does have one. >>>> >>> >>> So I debugged it and the issue is that after the panic I was doing a >>> cold boot rather than a warm boot and the memory was being randomised. >>> >>> The reason that 0x8000000 seemed to be initialised is because they are >>> based on the reserved region size, rather than anything from the >>> metadata. When I examined the metadata it was all randomised. >>> >>> That leads me to think that the single bit for 'valid' is insufficient. >>> There is a simple hashing function in include/linux/stringhash.h that >>> we could use on the whole metadata struct, but that specifically says: >>> >>> * These hash functions are NOT GUARANTEED STABLE between kernel >>> * versions, architectures, or even repeated boots of the same kernel. >>> * (E.g. they may depend on boot-time hardware detection or be >>> * deliberately randomized.) >>> >>> Although I'm not sure how true the repeated boots of the same kernel >>> part is. >>> >>> Maybe something in include/crypto/hash.h could be used instead, or >>> make our own simple hash. >> >> Thanks for the pointers. Will take a look at it. > > Since the purpose is to identify any data corruption, crc32(using crc32_le API) looks okay to me. Any thoughts on this ? > May be we could add crc32 checks for trace data as well ? > > Thanks. > Seems fine to me. Maybe doing it on the trace data is overkill if you already know the metadata is fine, but at the same time it might not do any harm either. It might catch some edge case where the firmware or device is doing something strange. > > > >> >> >> >> >> >> >> >> _______________________________________________ >> CoreSight mailing list -- coresight@lists.linaro.org To unsubscribe send an >> email to coresight-leave@lists.linaro.org _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel