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 1E08FC83F23 for ; Thu, 29 Aug 2024 13:37:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type: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=JSsM/lO+4YpCWxFffAqXniQP8CFl6WhkdaXd93xgMe4=; b=FFuDMVuWgQ9wjyiJZqlQtTvojc zIUTI/zhYdnrTL0AZ9OReL0RTCsAplVAx5cc6wX2j57ALeWxQ7iwPU0qel5fZ2qallthcIcj/58oO gb4tEs/aYhLpssTszTFMyGbMTDgxaaCYMlnFTlMmCLY1OpJhg4sC5BGOdeYpx0vIN1do6qc0UviUf OwDAZIeJO94ZH5rkdwyw/nWe5a3SpD+22o7mHsRJ4i/7UsN/qgUauUXNvc+jnwSTfzlez5MFX49kW TiiAJMGtLGiDX1CTvR5Gqi0Eh64saTH+umv7Xk/EW5NoR0WqKv6dh2Co+5+LSCNbT7JnfdkCTtj9e CevT9nKw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sjfKS-00000002DbI-2PVB; Thu, 29 Aug 2024 13:36:48 +0000 Received: from mail-wm1-x335.google.com ([2a00:1450:4864:20::335]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sjfJE-00000002DL7-26lb for linux-arm-kernel@lists.infradead.org; Thu, 29 Aug 2024 13:35:56 +0000 Received: by mail-wm1-x335.google.com with SMTP id 5b1f17b1804b1-427fc97a88cso5995935e9.0 for ; Thu, 29 Aug 2024 06:35:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1724938530; x=1725543330; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=JSsM/lO+4YpCWxFffAqXniQP8CFl6WhkdaXd93xgMe4=; b=dpaTFrSrBr+9m2gfgmRSuV7i1bU8wqbos59cN8hCM3QvFm0jl0uAio7me7FfeHqQuC 3h70ZiyxBbe4UsXqpFFvzJQnURTZwJKJ+y2nV9xUPxk8NGMg8wcEvmHPHckYmVsUfXKY 6m2zGkPqGdoUcKoXNVZBJGr9BaPGd/GYlnJ4ZPPzvKCxt+sz3nXPJ0jsKETpchmK3UKU 5VhyUZb1uGaAcu7RhFQ5NUCWY4ZDiIt/QeIM2rWmbYgbWk+k9JkkuBP9J+QdpxwQ6yjG 57+j1MJ80QRIy1se+wBXH7i1tti+q5hLml9NzHQVuWCtfpyGXk6blyScsT0WNUKo66xI nM3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724938530; x=1725543330; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=JSsM/lO+4YpCWxFffAqXniQP8CFl6WhkdaXd93xgMe4=; b=MJ2m+x+HzyNJufKYu1qI4sTuf5Fzczdg0r2UBGKwuZ//EaYAsmykUXipVXbcFQ0tZn xxev2bAySeTKRro3LBc+3dGyfgmhAqQMlm0H8jPcfNE2QV8TebBSIlh455eO2Vf/MGq3 4qK/6huelWpC3Auk11Pa5HYcA1z37AVfBPSc+Kr4Xt0pLpJmZlteBRmgYjhsFsHgEc9+ ZspdMJdl2ArgpTfVVhUKcN70/jIcqXFPdbBjskhCuR+YbLajem3agNmG3eX4KFPprpHp v787JQrbP+tE081NLKWdcUyMFPXmL6hrt0W54vJsVlCE1YoyE6P7VFIvR9iGPPAXO2tl tF9g== X-Forwarded-Encrypted: i=1; AJvYcCVm1JS0W1r0b8FEqr8JG3i5q44K2xb4i2SlJCpugfwvA5gwVQsKAWFWJ2tQdBwfboQCwWE/JMmVVtxP7gAzETwT@lists.infradead.org X-Gm-Message-State: AOJu0YwglH+0Z4fpVph1JhneCIxT3CVBcw9DCPegbBZkQkvZar5BkRFP rz+QR+37reCnGKZtelFgOi9frgMeEvVvzNq64FSVV4F3r5J0OUlbZ95gb4dKh5M= X-Google-Smtp-Source: AGHT+IER8vGT6ediwCAyqx6aCQUpbqLJByepL+K8SdstMmaMLlSkGocwPSpSEcoR8m3UxDC31+hi0g== X-Received: by 2002:a05:600c:4f42:b0:426:6220:cb57 with SMTP id 5b1f17b1804b1-42bb02d8cfemr22362385e9.25.1724938529737; Thu, 29 Aug 2024 06:35:29 -0700 (PDT) Received: from [192.168.1.3] ([89.47.253.130]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3749ee70bf4sm1453805f8f.34.2024.08.29.06.35.28 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 29 Aug 2024 06:35:29 -0700 (PDT) Message-ID: Date: Thu, 29 Aug 2024 14:35:28 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken To: Mike Leach , Ganapatrao Kulkarni Cc: Leo Yan , scclevenger@os.amperecomputing.com, acme@redhat.com, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, darren@os.amperecomputing.com, james.clark@arm.com, suzuki.poulose@arm.com, Al.Grant@arm.com References: <20240719092619.274730-1-gankulkarni@os.amperecomputing.com> <00fac24c-d664-4ebb-8c60-f4697b7f76c1@linaro.org> <8b53a424-19f7-4042-a2db-e1c5d051f9cc@os.amperecomputing.com> <6adf84fa-b755-4d7a-957a-9bf01e442238@linaro.org> <6f535bb6-2cee-48e6-93f1-ea19887bae74@os.amperecomputing.com> <027c76a9-9bd4-43e9-a170-8391a0037291@linaro.org> <3d7a6f93-0555-48fa-99cb-bf26b53c2da5@os.amperecomputing.com> <4dd7f210-c03e-4203-b8e9-1c26a7f8fe79@arm.com> <27912fc6-8419-4828-82a7-dacde5b4a759@linaro.org> <36f947ef-c2a7-486a-b905-f0529308b06e@linaro.org> Content-Language: en-US From: James Clark In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240829_063532_600072_5ACB3A33 X-CRM114-Status: GOOD ( 65.98 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 28/08/2024 10:33 am, Mike Leach wrote: > Hi James, > > On Fri, 23 Aug 2024 at 10:03, James Clark wrote: >> >> >> >> On 19/08/2024 11:59 am, Mike Leach wrote: >>> Hi, >>> >>> A new branch of OpenCSD is available - ocsd-consistency-checks-1.5.4-rc1 >>> >>> Testing I managed to do confirms the N atom on unconditional branches >>> appear to work. I do not have a test case for the range >>> discontinuities. >>> >>> The checks are enabled using operation flags on decoder creation. See >>> the docs for details. >>> >>> Mike >>> >> >> Hi Mike, >> >> I tested the new OpenCSD and I don't see the error anymore in the >> disassembly script. I'm not sure if we need to go any further and add >> the backwards check, it looks like just a later symptom and the checks >> that you've added already prevent it. >> > > The OCSD_OPFLG_CHK_RANGE_CONTINUE is the backwards address check - at > least as so far as is possible in OpenCSD. > What it checks is if the next range after a not taken branch starts at > the end address of the previous range. However this check is cancelled > if there are other packets that intervene - e.g. trace on / exceptions > / anything that might imply a discontinuity. > > The other caveat is that I did not have an example to see if the code > could actually get triggered - though I will go back and manually > trigger it in the debugger just to test functional correctness. > > If you are still seeing backwards addresses after these changes then I > am not sure where they are coming from. It may be there is a missing > discontinuity somewhere that is not being flagged. I tracked down this issue, so there are two issues now: #1 Using vmlinux which is a bad image, but is fixed by your OpenCSD bad image detection changes. #2 With Ganapat's kcore which should be the correct image, the issue is in Perf. There is a bug in the handling of a full packet_queue resulting in it setting the previous branch destination rather than the next one for the last sample in the queue. I should be able to send a patch for this. I'll also try to add a test because this decode script seems like a good place to catch bugs. > > The other alternative that does occur to me now - thinking about > incorrect images, is if we incorrectly associate an atom with a direct > branch rather than an indirect branch. > For a direct branch the decoder will calculate the target and carry on > - not looking for an address update as it is not needed. Then when the > address update does arrive, it is used as a latest address and the > range address will be updated. Yeah this is the backwards address issue I was thinking of, but with the other OpenCSD changes I don't think there is an example of it, so we can probably hold off for now. Especially if it's difficult to test for. > > Unfortunately this would be difficult to test for - the decoder is > written to assume good trace and correct images - adding in code to > try to remember previous state and judge if something is wrong, > without getting false positives is difficult. It adds code complexity > that is not necessary for well behaved clients! > >> If you release a new version I can send the perf patch. I was going to >> use these flags if that looks right to you? As far as I know that's the >> set that can be always on and won't fail on bad hardware? >> > > That set of flags is fine - > >> I also assumed that ETM4_OPFLG_PKTDEC_AA64_OPCODE_CHK can be given even >> for etmv3 and it's just a nop? >> > > It is safe - as there is no flag for ETMv3 in the same slot. > Effectively decode flags have a common set of bits and decoder > specific set of bits that overlap for each decoder. We have not > really needed anything for ETMv3 to date, and I don't expect that to > change. > > > I'll get the new version released by the end of the week. I am off on > sabbatical for a month after that so any further investigation / > changes will have to wait > > Regards > > Mike > > >> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >> index e917985bbbe6..90967fd807e6 100644 >> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c >> @@ -685,9 +685,14 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params, >> return 0; >> >> if (d_params->operation == CS_ETM_OPERATION_DECODE) { >> + int decode_flags = OCSD_CREATE_FLG_FULL_DECODER; >> +#ifdef OCSD_OPFLG_N_UNCOND_DIR_BR_CHK >> + decode_flags |= OCSD_OPFLG_N_UNCOND_DIR_BR_CHK | OCSD_OPFLG_CHK_RANGE_CONTINUE | >> + ETM4_OPFLG_PKTDEC_AA64_OPCODE_CHK; >> +#endif >> if (ocsd_dt_create_decoder(decoder->dcd_tree, >> decoder->decoder_name, >> - OCSD_CREATE_FLG_FULL_DECODER, >> + decode_flags, >> trace_config, &csid)) >> return -1; >> >>> On Fri, 9 Aug 2024 at 16:20, James Clark wrote: >>>> >>>> >>>> >>>> On 09/08/2024 3:13 pm, Mike Leach wrote: >>>>> Hi James >>>>> >>>>> On Thu, 8 Aug 2024 at 10:32, James Clark wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 07/08/2024 5:48 pm, Leo Yan wrote: >>>>>>> Hi all, >>>>>>> >>>>>>> On 8/7/2024 3:53 PM, James Clark wrote: >>>>>>> >>>>>>> A minor suggestion: if the discussion is too long, please delete the >>>>>>> irrelevant message ;) >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py >>>>>>>>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py >>>>>>>>> @@ -257,6 +257,11 @@ def process_event(param_dict): >>>>>>>>> print("Stop address 0x%x is out of range [ 0x%x .. 0x%x >>>>>>>>> ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso)) >>>>>>>>> return >>>>>>>>> >>>>>>>>> + if (stop_addr < start_addr): >>>>>>>>> + if (options.verbose == True): >>>>>>>>> + print("Packet Dropped, Discontinuity detected >>>>>>>>> [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, start_addr, >>>>>>>>> dso)) >>>>>>>>> + return >>>>>>>>> + >>>>>>>> >>>>>>>> I suppose my only concern with this is that it hides real errors and >>>>>>>> Perf shouldn't be outputting samples that go backwards. Considering that >>>>>>>> fixing this in OpenCSD and Perf has a much wider benefit I think that >>>>>>>> should be the ultimate goal. I'm putting this on my todo list for now >>>>>>>> (including Steve's merging idea). >>>>>>> >>>>>>> In the perf's util/cs-etm.c file, it handles DISCONTINUITY with: >>>>>>> >>>>>>> case CS_ETM_DISCONTINUITY: >>>>>>> /* >>>>>>> * The trace is discontinuous, if the previous packet is >>>>>>> * instruction packet, set flag PERF_IP_FLAG_TRACE_END >>>>>>> * for previous packet. >>>>>>> */ >>>>>>> if (prev_packet->sample_type == CS_ETM_RANGE) >>>>>>> prev_packet->flags |= PERF_IP_FLAG_BRANCH | >>>>>>> PERF_IP_FLAG_TRACE_END; >>>>>>> >>>>>>> I am wandering if OpenCSD has passed the correct info so Perf decoder can >>>>>>> detect the discontinuity. If yes, then the flag 'PERF_IP_FLAG_TRACE_END' will >>>>>>> be set (it is a general flag in branch sample), then we can consider use it in >>>>>>> the python script to handle discontinuous data. >>>>>> >>>>>> No OpenCSD isn't passing the correct info here. Higher up in the thread >>>>>> I suggested an OpenCSD patch that makes it detect the error earlier and >>>>>> fixes the issue. It also needs to output a discontinuity when the >>>>>> address goes backwards. So two fixes and then the script works without >>>>>> modifications. >>>>>> >>>>> >>>>> Which address is going backwards here? - OpenCSD generates trace >>>>> ranges only by walking forwards from the last known address till it >>>>> hits a branch. Unless this wraps round 0x000000 this will never result >>>>> in a backwards address as far as I can see. >>>>> Do you have an example dump with OpenCSD outputting a range packet >>>>> with backwards addresses? >>>>> >>>>> Mike >>>>> >>>> The example I have I think is something like this: >>>> >>>> 1. Start address / trace on >>>> 2. E >>>> 3. Output range >>>> ... >>>> 4. Periodic address update >>>> ... >>>> 5. E >>>> 6. Output range >>>> >>>> If decode has gone wrong (but undetectably) between steps 1 and 3. Then >>>> the next steps still output a second range based on the last periodic >>>> address received. (I think it might not necessarily have to be a >>>> periodic address but could also be indirect address packet?). Perf >>>> converts the ranges into branch samples by taking the end of the first >>>> range and beginning of the second range. Then the disassembly script >>>> converts those samples into ranges again by taking the source and >>>> destination of the last two branch samples. >>>> >>>> The original issue that Ganapat saw was that the periodic address causes >>>> OpenCSD to put the source address of the second range somewhere before >>>> the first one, even though it didn't output a branch or discontinuity >>>> that would explain how it got there. >>>> >>>> But yes you're right the ranges themselves always go forwards from the >>>> point of view of their own start and end addresses. >>>> >>>> I thought it might be possible for OpenCSD to check against the last >>>> range output? Although I wasn't sure if maybe it's actually valid to do >>>> a backwards jump like that without the trace on/off packets with address >>>> filtering or something? >>>> >>>> The root cause is still the incorrect image, but I think this check >>>> along with the other direct branch check should make it pretty difficult >>>> for people to make the mistake. >>> >>> >>> > > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK