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=-10.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 9BF62C433E0 for ; Sun, 17 Jan 2021 12:11:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5DB6F22573 for ; Sun, 17 Jan 2021 12:11:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728823AbhAQMLL (ORCPT ); Sun, 17 Jan 2021 07:11:11 -0500 Received: from foss.arm.com ([217.140.110.172]:43208 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728499AbhAQMKz (ORCPT ); Sun, 17 Jan 2021 07:10:55 -0500 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 203701FB; Sun, 17 Jan 2021 04:10:09 -0800 (PST) Received: from [192.168.0.130] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 56F123F719; Sun, 17 Jan 2021 04:10:06 -0800 (PST) Subject: Re: [PATCH V2 10/11] coresight: sink: Add TRBE driver To: Suzuki K Poulose , linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org Cc: mathieu.poirier@linaro.org, mike.leach@linaro.org, Linu Cherian , linux-kernel@vger.kernel.org References: <1610511498-4058-1-git-send-email-anshuman.khandual@arm.com> <1610511498-4058-11-git-send-email-anshuman.khandual@arm.com> <43bc1738-040c-1e48-b8f1-d337dcfcff2e@arm.com> From: Anshuman Khandual Message-ID: <2fb48d88-f195-ca48-5d87-f402ac4e4381@arm.com> Date: Sun, 17 Jan 2021 17:40:23 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 1/15/21 6:13 PM, Suzuki K Poulose wrote: > On 1/15/21 5:29 AM, Anshuman Khandual wrote: >> >> >> On 1/13/21 8:58 PM, Suzuki K Poulose wrote: >>> Hi Anshuman, >>> >>> The driver looks overall good to me. Please find some minor comments below Sure. >>> >>> On 1/13/21 4:18 AM, Anshuman Khandual wrote: >>>> Trace Buffer Extension (TRBE) implements a trace buffer per CPU which is >>>> accessible via the system registers. The TRBE supports different addressing >>>> modes including CPU virtual address and buffer modes including the circular >>>> buffer mode. The TRBE buffer is addressed by a base pointer (TRBBASER_EL1), >>>> an write pointer (TRBPTR_EL1) and a limit pointer (TRBLIMITR_EL1). But the >>>> access to the trace buffer could be prohibited by a higher exception level >>>> (EL3 or EL2), indicated by TRBIDR_EL1.P. The TRBE can also generate a CPU >>>> private interrupt (PPI) on address translation errors and when the buffer >>>> is full. Overall implementation here is inspired from the Arm SPE driver. >>>> >>>> Cc: Mathieu Poirier >>>> Cc: Mike Leach >>>> Cc: Suzuki K Poulose >>>> Signed-off-by: Anshuman Khandual > > ... > >>>> + >>>> +/* >>>> + * TRBE Buffer Management >>>> + * >>>> + * The TRBE buffer spans from the base pointer till the limit pointer. When enabled, >>>> + * it starts writing trace data from the write pointer onward till the limit pointer. >>> >>> >>>> + * When the write pointer reaches the address just before the limit pointer, it gets >>>> + * wrapped around again to the base pointer. This is called a TRBE wrap event, which >>>> + * generates a maintenance interrupt when operated in WRAP or STOP mode. >>> >>> According to the TRM, it is FILL mode, instead of STOP. So please change the above to: >>> >>> "operated in WRAP or FILL mode". Changed. >> >> Updated. >> >>> >>> >>>>      The write >>>> + * pointer again starts writing trace data from the base pointer until just before >>>> + * the limit pointer before getting wrapped again with an IRQ and this process just >>>> + * goes on as long as the TRBE is enabled. >>> >>> This could be dropped as it applies to WRAP/CIRCULAR buffer mode, which we don't use. >> >> Probably this could be changed a bit to match the FILL mode. Because it is essential >> to describe the continuous nature of the buffer operation, even in the FILL mode. >> >>   * After TRBE >>   * IRQ gets handled and enabled again, write pointer again starts writing trace data >>   * from the base pointer until just before the limit pointer before getting wrapped >>   * again with an IRQ and this process just goes on as long as the TRBE is enabled. >> > > The above doesn't parse well and kind of repeats the operation of TRBE which is > already explained above. How about : > >>>> + * When the write pointer reaches the address just before the limit pointer, it gets >>>> + * wrapped around again to the base pointer. This is called a TRBE wrap event, which >>>> + * generates a maintenance interrupt when operated in WRAP or STOP mode. > > This driver uses FILL mode, where the TRBE stops the trace collection at wrap event. > The IRQ handler updates the AUX buffer and re-enables the TRBE with updated WRITE and > LIMIT pointers. Updated. > > >>>> + >>>> +static void *arm_trbe_alloc_buffer(struct coresight_device *csdev, >>>> +                   struct perf_event *event, void **pages, >>>> +                   int nr_pages, bool snapshot) >>>> +{ >>>> +    struct trbe_buf *buf; >>>> +    struct page **pglist; >>>> +    int i; >>>> + >>>> +    if ((nr_pages < 2) || (snapshot && (nr_pages & 1))) >>> >>> This restriction on snapshot could be removed now, since we use the >>> full buffer. >> >> Dropped only the second condition here i.e (snapshot && (nr_pages & 1). >> Just wondering if the aux buffer could work with a single page so that >> the first condition can also be dropped. > > I think it is good to keep the restriction of 2 pages, as the WRITE_PTR > and the LIMIT_PTR must be page aligned. With a single page, you can't do > much, with writing into a partially filled buffer. This may be added > as a comment to explain the restriction. Added the above comment. > >>>> + >>>> +static enum trbe_fault_action trbe_get_fault_act(struct perf_output_handle *handle) >>>> +{ >>>> +    int ec = get_trbe_ec(); >>>> +    int bsc = get_trbe_bsc(); >>>> + >>>> +    WARN_ON(is_trbe_running()); >>>> +    if (is_trbe_trg() || is_trbe_abort()) >>> >>> We seem to be reading the TRBSR every single in these helpers. Could we optimise them >>> by passing the register value in ? >> >> The same goes for get_trbe_ec() and get_trbe_bsc() as well. Probably all >> TRBSR field probing helpers should be modified to accept a TRBSR register >> value instead. >> >>> >>> i.e >>>      u64 trbsr = get_trbe_status(); >>> >>>      WARN_ON(is_trbe_runnign(trbsr)) >>>      if (is_trbe_trg(trbsr) || is_trbe_abort(trbsr)) >>> >>> For is_trbe_wrap() too >> >> Yes. >> >>> > > >>> >>> We should skip the driver init, if the kernel is unmapped at EL0, >>> as the TRBE can't safely write to the kernel virtual addressed >>> buffer when the CPU is running at EL0. This is unlikely, but we >>> should cover that case. >> >> This should be sufficient or it needs a pr_err() as well ? >> > > Please add a pr_err() message to indicate why this failed. Otherwise > the user could be left with no clue. Sure, will add the following before exiting the TRBE init. pr_err("TRBE wouldn't work if kernel gets unmapped at EL0\n")