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=-5.2 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,USER_AGENT_SANE_1 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 95111C2D0A3 for ; Thu, 12 Nov 2020 12:24:44 +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 01EA720872 for ; Thu, 12 Nov 2020 12:24:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="gsNbpkJn"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="BONItrIc" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 01EA720872 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:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=ilfGUz/81NO4EHzNZRQe+fhCdRoGcUqQd9M+VApkocA=; b=gsNbpkJnq13cE4k1UDIDhBcH/ 1KIWt5wgBwy6fuaH4CfvTcPcuhBNA0NM8B5BUheDgQU5f6zsRwc3lFvauHi7JkeXi7SYV5jk2c5GN nU006UxLN8Qhw23+I8nO6IChPHi9vdmH8RhXbxqycW9Cdan2uSJg3VqJKuhJyCjJ6gsMwv6b6tthz GzyyiQNi2QuRgawQWZqauwehTyUMHMI/0FzHz/qGvioMoQFv2mO1ZM1lWroxNQJ5L/GPwPmkPoEnw vc9Nl9pieqVbXqLDmqboO6cQ0RY3bWw6Iqdw95z9XhJgkJRL4U7Z0thZLzh221nPNMoH4AXA5rfaE 0eQD5ijvw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdBeF-0002gW-GY; Thu, 12 Nov 2020 12:24:19 +0000 Received: from mail-pg1-x542.google.com ([2607:f8b0:4864:20::542]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdBeD-0002gC-Gp for linux-arm-kernel@lists.infradead.org; Thu, 12 Nov 2020 12:24:18 +0000 Received: by mail-pg1-x542.google.com with SMTP id i26so4045008pgl.5 for ; Thu, 12 Nov 2020 04:24:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=P5B7L9L4uKM78fX9FhT7K2dQzK0orJjDYwx8HNJuzZM=; b=BONItrIcviq9Bo73NyQGwogAPVRL9KOgGF7+8tujzdR01GhYk3Cypj2ZqPE2q0qqCs N3ZrX/ujOHA9ohOiMrb/wernCqXTjnxupysIGzCWxxIUkWopUmwmGhHNZgXPwCMqLzm8 5e159ebDoPy07evy/R9/hNP7/qxM4rHA5ln3nSezfP+w6qKg6p6BOa/eKBhHJr8lQmJP qESQiiBKTS7PJBPTPmJNaLL5m2w30DG7vpN5bQRVydgmnZHNHAC+DIXRzSC/2fdT4cp+ OsThmHUeCB6n2PpusYSvYxo80JuzCVvadUWqfxBzDExBx7DEqb7ohSBwInoMaFD3sL/G WKvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=P5B7L9L4uKM78fX9FhT7K2dQzK0orJjDYwx8HNJuzZM=; b=PRgT1T4bxmhk+06ilChiSJ+wfT+dWVYg7a/khhyNZWG/PULgWX/gmIWMbukerDWjOF rCkw4M4nFvVc44Ojbduo5MIAK7tvmpiEbt+yofYxSs09pfb6o7c+mjPIP4YyJakI3iQW PFAyiuJELPvGvMG8OJKimCsLD6vq0oZgK/gd3/y7/XiE7LIsCNtr0C6PW7Eywp4MXFfk kWrtKpkEzh1+MRJ/ic0kEj8yCUuMS2Y8A9HMaHnTmkQp/1RkRCs2S0vWNojRdSeJPwSQ M88AnbyAQ+69jcqhtDT/JL+DM5FnBVzuFw2QJIvUIaiuNgx7a+f4gdptnfwzU5e6sHjZ sE8Q== X-Gm-Message-State: AOAM530b+g4urt2l0J7dTlN1MFs0y02BxqZ+mky132L6j8PWI6vyY+sB okXM4qrlbCrrZaKW8KVSPgjzmA== X-Google-Smtp-Source: ABdhPJzmqxhlXi986r+Bd3DzcLwoc+mBB2B3y6w2/2lMoHc0ODtC7gvjKfmZb+3/0pTEepEWEs7nWg== X-Received: by 2002:a63:1a1d:: with SMTP id a29mr5465482pga.19.1605183854941; Thu, 12 Nov 2020 04:24:14 -0800 (PST) Received: from leoy-ThinkPad-X240s ([103.127.239.100]) by smtp.gmail.com with ESMTPSA id s11sm6864661pjm.4.2020.11.12.04.24.12 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Thu, 12 Nov 2020 04:24:14 -0800 (PST) Date: Thu, 12 Nov 2020 20:24:10 +0800 From: Leo Yan To: Suzuki K Poulose Subject: Re: [PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid Message-ID: <20201112122410.GD17274@leoy-ThinkPad-X240s> References: <20201110183313.1823760-1-suzuki.poulose@arm.com> <20201110183313.1823760-3-suzuki.poulose@arm.com> <20201112100010.GB17274@leoy-ThinkPad-X240s> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201112_072417_651910_3F121A2C X-CRM114-Status: GOOD ( 27.79 ) 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: al.grant@arm.com, mathieu.poirier@linaro.org, anshuman.khandual@arm.com, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org 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 Thu, Nov 12, 2020 at 10:54:23AM +0000, Suzuki Kuruppassery Poulose wrote: > Hi Leo, > > Thanks for looking at the patch. Welcome! [...] > > > + switch (pid_fmt) { > > > + case (1ULL << ETM_OPT_CTXTID): > > > + /* > > > + * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing > > > + * is supported: > > > + * 0b00000 Context ID tracing is not supported. > > > + * 0b00100 Maximum of 32-bit Context ID size. > > > + * All other values are reserved. > > > + */ > > > + val = BMVAL(val, 5, 9); > > > + if (!val || val != 0x4) { > > > + err = -EINVAL; > > > + goto out; > > > + } > > > + break; > > > + case (1ULL << ETM_OPT_CTXTID_IN_VMID): > > > + /* > > > + * TRCIDR2.VMIDOPT[30:29] != 0 and > > > + * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual context id size) > > > + */ > > > + if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) { > > > > The comment is not alignment with the code. Based on the comment, the > > code should be: > > > > if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) != 4) { > > > > The reasoning is any value > 4, would imply a size > 32. This is really making > it future proof. I could restrict this to 4 now if you insist. So here you want to filter out the cases for 16-bit and 8-bit virtual context ID size, but want to leave the possibility for support size > 32-bit in future. It's fine to keep current code, but it's better to add description in the comment, otherwise, this might be difficult for maintenance. > > > +#define ETM_SET_OPT_PID (1 << 0) > > > +#define ETM_SET_OPT_TS (1 << 1) > > > +#define ETM_SET_OPT_MASK (ETM_SET_OPT_PID | ETM_SET_OPT_TS) > > > + > > > static int cs_etm_set_option(struct auxtrace_record *itr, > > > struct evsel *evsel, u32 option) > > > { > > > @@ -169,17 +196,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr, > > > !cpu_map__has(online_cpus, i)) > > > continue; > > > - if (option & ETM_OPT_CTXTID) { > > > - err = cs_etm_set_context_id(itr, evsel, i); > > > + if (option & ETM_SET_OPT_PID) { > > > + err = cs_etm_set_pid(itr, evsel, i); > > > > I don't understand what's the reason for introducing the new macros > > "ETM_SET_OPT_XXX", seems to me the old macros still can be used at > > here. Could you help explian for this? > > Sure, with the change now, we either set ETM_OPT_CTXTID or ETM_OPT_CTXTID_IN_VMID > in the config. So, using the flag ETM_OPT_CTXTID to indicate the same is going to > be confusing. This makes it explicit and uses a separate flag. Or the in otherways > it is really making clear that we want to set PID tracing and de-coupling it from > "CONTEXTID" as it is not the correct config anymore and has to be determined at > runtime. I could make it explicit in a comment. Makes sense for me; thanks for the explanation. Leo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel