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 X-Spam-Level: X-Spam-Status: No, score=-4.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A162BC4727C for ; Tue, 29 Sep 2020 16:44:43 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 02ABF208B8 for ; Tue, 29 Sep 2020 16:44:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="pct5Zce7"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="IfvPo64W" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 02ABF208B8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=KPPO2JF91HmmunQQAIL64j2q8dwoK+HS3W0bLkj8YQY=; b=pct5Zce7enHDfd8KO0qASdXK1 TFicrikUgMNal8mDfAg24s7Z2AZnYqJtBLqRAbQPlNx+tQc1d8vGRk9anXg7QyKzxPQ83i7ZiQeMH jZ3FTzzNjjmIRBQQyORuWw407UkwjW2ZdAL52MH1q1G/gPTdbp9+/9OCUZRi3ivDkK6vG8pWhE9eU WMZ3NCPETukthhgDSUNy6QCOl2y0VSTZiBbfzYxDI4XJeNo5lUITwkp0AyX3eV+uXAVZK065t/8IT ws6itCaP/+UmId+Xe/C0ycXscfdu9p72itJQuNGaxUYLOtn2rwCj5iFVhlTkGXeeM3vCmDITq0c9Z +QgZPEXwg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kNIiY-0003xD-6B; Tue, 29 Sep 2020 16:43:06 +0000 Received: from mail-ej1-x62a.google.com ([2a00:1450:4864:20::62a]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kNIiU-0003wW-Ps for linux-arm-kernel@lists.infradead.org; Tue, 29 Sep 2020 16:43:04 +0000 Received: by mail-ej1-x62a.google.com with SMTP id lo4so15959867ejb.8 for ; Tue, 29 Sep 2020 09:42:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SfetMa/LM1Yw2+/8MoG+vDPhqblsPRdlMhtq9e85uCk=; b=IfvPo64WvhpferyTJoLYdlkNoexeZWzoL3vQOIZbBJOr7shJbzqCcuso/ysDmvI3wT 0m3piv2mOmzsi0bGpUNOqDNpSMMRexUNU2dUMuXbO5tSMWdfmH6HSjKw9UR7HLkdmali UImgEm3yw2+ntms4jNdlQyIgfNJhLATCWAnXpBrk85X8p2JJou7fJU8e8bd1MRC0ZG6M 4C4JPfEJ5qRcnOpCZI/tkC6TCDCJfmHLKMykA5i8mb0VepLTd6RoVfija4n72tOvUp0P oradnXi3Bz+ZO/qsF0X2v/jtFcI5B90+sxyKQLxmElrf9MrGi0vo0i0Cu+Hz3KvbByDv yp9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SfetMa/LM1Yw2+/8MoG+vDPhqblsPRdlMhtq9e85uCk=; b=hdc+6E84y7rFZijynrTZ7m/Ri6Q6ADwZxsI+xMI37LP2U9mUVDltxi1ZBqZIqfi//m NQHuAz9Xd0mFZn1FumDvcZAaFWr2p2nrRlp7WrWEriGf5mkz1JaFrDSfkIIjDgLr7NQa Bqgn1RX1IXdtc6nJTn/bvmgwiCJwFCrg9UiieNC5f6+tzVkW8VjZUC5fdmRmbO20fCgC 4RkBFk1wBdmt/+t8sNjay2b6EPyYnio83VehYM4mbgPIT1FWp5Aw4rX92sZA6z8Kbj3P z7/KHrNz0PxWa+e24LFh7I6K98fdwVXnLTrb9iCaZ/oBUmJZv1cM2NbJaUF7MJ0Nashh zEDg== X-Gm-Message-State: AOAM530MxWpRIGmAOybb+2QQewCOgqyeR9EXsCHThjPvQS5WzKTiatwp esOuJDHkn0Vbz8m3opEhNRDWS/ass7FRIsrZMI5VpA== X-Google-Smtp-Source: ABdhPJwOiRvK5gP/flSTzNYlXOjB9r3vZWoj/y5er/3ZfiepAirN92wqtKlFu+9cnkfcY+qs7ncaYyjkwM0CzIKsQus= X-Received: by 2002:a17:906:4a53:: with SMTP id a19mr4993668ejv.56.1601397778178; Tue, 29 Sep 2020 09:42:58 -0700 (PDT) MIME-Version: 1.0 References: <20200911084119.1080694-1-suzuki.poulose@arm.com> <76651662-f06d-26fc-8f3d-6fed3702eb5c@arm.com> In-Reply-To: <76651662-f06d-26fc-8f3d-6fed3702eb5c@arm.com> From: Mike Leach Date: Tue, 29 Sep 2020 17:42:47 +0100 Message-ID: Subject: Re: [PATCH 00/19] coresight: Support for ETMv4.4 system instructions To: Suzuki K Poulose X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200929_124303_126722_0AFF5902 X-CRM114-Status: GOOD ( 68.77 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Coresight ML , anshuman.khandual@arm.com, Mathieu Poirier , linux-arm-kernel , Leo Yan 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 Hi Suzuki, On Fri, 25 Sep 2020 at 10:50, Suzuki K Poulose wrote: > > Hi Mike, > > > First of all, thank you so much for your in depth review. > Please find my comments inline. > > On 09/18/2020 04:33 PM, Mike Leach wrote: > > Hi Suzuki, > > > > I've looked at the set and have only one real gripe - the > > implementation and timing of component detection on the sysreg path. > > I've summarised my thoughts here, but as the changes are found across > > multiple patches I may well have repeated myself a little in the > > individual places. > > > > On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose wrote: > >> > >> CoreSight ETMv4.4 introduced system instructions for accessing > >> the ETM. This also implies that they may not be on the amba bus. > > > > System instructions have always been an option - but we have never > > supported them up to now. In fact both memory access and system > > instructions can live side by side - but the driver really needs to > > choose just one! > > What did happen is that a PE that supports Arm Trace 8.4 mandates ETM > > 4.4, and ETM 4.4 mandates system instruction access for PEs with Arm > > Trace 8.4, and deprecates memory access. > > > > But there is nothing to stop other variants having the system > > instruction interface. So there is no need to describe this as a > > purely 4.4. onwards support - it will support any version of the ETM > > that has sysreg access. > > Correct, I agree, and will change this. > > > > > The spec permits aarch32 / armv7 register access via CP14 - but I > > assume this is omitted deliberately & not intended to be supported at > > this time. > > Yes, because the ETMv4 driver doesn't support arm32 bit at all. We could > definitely add this in the future. I will add it to the commit description. > > > > >> Right now all the CoreSight components are accessed via memory > >> map. Also, we have some common routines in coresight generic > >> code driver (e.g, CS_LOCK, claim/disclaim), which assume the > >> mmio. In order to preserve the generic algorithms at a single > >> place and to allow dynamic switch for ETMs, this series introduces > >> an abstraction layer for accessing a coresight device. It is > >> designed such that the mmio access are fast tracked (i.e, without > >> an indirect function call). > >> > >> This will also help us to get rid of the driver+attribute specific > >> sysfs show/store routines and replace them with a single routine > >> to access a given register offset (which can be embedded in the > >> dev_ext_attribute). This is not currently implemented in the series, > >> but can be achieved. > >> > >> Further we switch the generic routines to work with the abstraction. > >> With this in place, we refactor the etm4x code a bit to allow for > >> supporting the system instructions with very little new code. The > >> changes also switch to using the system instructions by default > >> even when we may have an MMIO. > >> > >> We use TRCDEVARCH for the detection of the ETM component, which > >> is a standard register as per CoreSight architecture, rather than > >> the etm specific id register TRCIDR3. This is for making sure > >> that we are able to detect the ETM via system instructions accurately, > >> when the the trace unit could be anything (etm or a custom trace unit). > >> > > > > I'm assuming you mean TRCIDR1 here -- which in part, defines the etm > > architecture version. TRCIDR3 does something else entirely. > > Yes, definitely, it is a mistake on my side, generated from staring at > both of them TRCIDR1 and TRCIDR3 (for the masks). > > > Not sure I agree with this though - the driver is designed to match > > the ETM spec so there is no problem with using TRCIDR1 to spot > > functional variants according to ETM version, > > Correct. But for a system instruction based trace unit, we can't trust > just the bindings and must use a CoreSight architected register to > do the basic detection. Also, I am planning to add support the Future > Architectures for the processor trace [0] with the ETM driver, which > mandates the use of TRCDEVARCH for the trace version. So, this is more > of > Agreed - sysreg must do its own validation of the connected component. > > > > The etm4_init_arch_data() function is not about detecting the presence > > of an ETMv4 component, but about exploring the capabilities it has. We > > check 4 bits of the version as a sanity check, but at this point we > > should be pretty sure we are dealing with an ETM of some kind. > > > > TRCDEVARCH is already used for detection in the AMBA matching code - > > assuming the table includes the optional CoreSIght UCI. I would > > imagine that similar detection needs to go on for instruction access - > > but once we have detected an ETM, then ETM architected registers are > > sufficient. If the device is not an ETM then it should be detected and > > rejected early - and the bindings examined to determine why this > > driver was attached! > > I agree with the fact that we should check the device for a supported > type at the earliest and must not trust the bindings. With the AMBA > based devices we have the early check as mentioned above via the PIDs > and the UCI (where available). But where the UCI is not listed, these > will be caught by the additional checks on the TRCIDR1 fields. e.g, > CTI could have the same PID as an ETM4 and without the UCI field, > the driver could assume that an ETM is CTI if the firmware was > incorrect The TRCDEVARCH register was introduced as part of CoreSight Architecture 2.0, the UCI concept as part of CoreSight Architecture 3.0. When we added the UCI checks to the AMBA matching path, this was for newer components that we knew were following the Coresight 3.0 concept of having the same part number for components in the same common function - i.e. ETM, CTI, PMU, with a given CPU. There was no way of knowing if 100% of the older components that matched the driver using only part number would also pass the UCI check, so this was not added for them (and certainly early CTIs never had a TRCDEVARCH). We did not want to break compatibility, given that some of those components may not be ARM designed. I am not sure what has changed such that we are now prepared to risk making the driver incompatible with older devices. > > > > The act of adding in a check against TRCDEVARCH as part of the > > etm4_init_arch_data() function adds new and hidden checks to AMBA > > devices where it was sufficient to have an entry in the probe match > > table before. Most recent additions include the UCI matching, but > > older ones don't. I am concerned that this changes may trip up older > > existing implementations which for some reason may not have > > TRCDEVCARCH, or have set it to not present. > > For the records, ETMv4.0 revision A, says, the PRESENT bit is always > Read As One (RAO). So, if they don't implement it or have set to 0, > that means that they are broken. But, we could gracefully handle it > if the PRESENT bit is 0 and fold back to TRCIDR1. > > > > > For this reason, I beleive that the TRCDEVARCH check for the sys reg > > access should occur on the sysreg specific probe - balancing what > > happens on the AMBA side. That way the common code remains common. > > To make the current situation clear, for those who have not looked > at the series, here is the summary : > > 1) AMBA driver checks the PIDs to match a device to known ETMv4. > Note that CTIs could share the same PIDs and thus we added additional > check on the UCI (which is TRCDEVARCH) field for some of the ETMs. > > 2) The ETM4 driver assumes that the component is ETMv4 and calls > etmv4_init_arch_data() and probes the ETM4 for features, filling > in drvdata, including the TRCIDR1. Please note that, at this point > there is no guarantee that the unit is indeed ETMv4, if the UCI > check (TRCDEVARCH) has not been performed. So, we are possibly > treading into wild land here (at least on bring up). > > 3) The etm4_probe() confirms that the ETM4 architecture is supported > by checking the TRCIDR1 fields (stored in drvdata->arch from step (2)). > This check is important (at least for bring up), because if the > UCI check is not added for the component, a CTI could be mistaken > for an ETM with AMBA devices. > > Fact : TRCDEVARCH must be implemented and represent that the component is > ETMv4 (this is the basis of UCI check) since ETMv4.0 specification. > Agreed - but unfortunately in the real world not every component correctly follows the spec - hence my concern about tightening up checks on existing devices. > > For the system instructions based devices, we have : > 1) Device tree compatible to advertise the presence of a trace unit on > a CPU. (no PID checks, this is good, because you don't have to > add an entry for a new CPU to be supported upstream, as long as > it is compliant with the ETMv4). > > 2) To confirm that the CPU tracing unit is ETMv4 compatible, we need > to use CoreSight architected register, TRCDEVARCH (the same as UCI). > This is because TRCIDR1 may not be what is expected if the Trace unit > is not ETMv4, since the encoding is ETM specific. And this must be > performed on the host CPU (just like etm4_init_arch_data). > > With this series: > > * AMBA devices pass through the PID check as usual. But the sysreg > devices jumps straight to etm4_init_arch_data() via common etm4_probe. > > * etm4_init_arch_data() will verify that the component is ETMv4 > by verifying the TRCDEVARCH for all ETMs (both AMBA and system > instructions), before poking the features. This will avoid having > to do another round of smp_func_call() in etm4 sysreg probe code. OK - so using the existing function avoids having two smp_func_call() instances - at the cost of increased complexity in the overall code. If you think this is worth it then fair enough. > - Note: As per Mike's suggestion, this can be further relaxed to check > TRCIDR1 for AMBA devices, iff TRCDEVARCH is marked absent for > supporting any wild broken implementations out there (I prefer to add a > pr_warn_once() for such cases, so that we know such units). > > > > Further the setup of the CSA for the device can happen immediately in > > the common etm_probe() function, based on *base being NULL or not, > > For now, yes. But with the additional changes for supporting [0], this > may not. As we need to really see if we have an ETMv4.x or a future > unit which has slightly different register list. > Not sure I understand here - are you saying that there may be a case where sysreg access could have a base memory address as well? After ETM4.4 the two have to be mutually exclusive, and even where they are not, there is no way the driver should be using both memory and system register access. The case in [0] may indicate an extension to the programming model, e.g. we currently check if the ETM version is >= 4.3 as this will affect the interpretation of resource selector numbers, and future trace devices may well have more programming model alterations - but this should not affect the access model. Regards Mike > > rather than as a side effect of the etm4_init_arch_data() call. > > [0] https://developer.arm.com/docs/ddi0601/latest , See TRCIDR1 > > Kind regards > Suzuki > > > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel