linux-kernel - Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a199db90-a344-40e0-8628-b4c3b56fc92c@linaro.org>
Date: Wed, 19 Nov 2025 13:55:15 +0000
From: James Clark <james.clark@...aro.org>
To: Leo Yan <leo.yan@....com>
Cc: Mike Leach <mike.leach@...aro.org>,
 Suzuki K Poulose <suzuki.poulose@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jonathan Corbet <corbet@....net>, Randy Dunlap <rdunlap@...radead.org>,
 coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v5 08/13] coresight: Interpret perf config with
 ATTR_CFG_GET_FLD()



On 19/11/2025 12:36 pm, Leo Yan wrote:
> On Wed, Nov 19, 2025 at 12:00:30PM +0000, James Clark wrote:
> 
> [...]
> 
>> ...  But then to make
>> the code match the warning it might also make sense to change CONFIG_ARM64
>> back to CONFIG_CORESIGHT_SOURCE_ETM4X, which Leo suggested to change. Maybe
>> I can just delete the warning text, practically this warning can never be
>> hit.
> 
> Armv8 CPUs can runs in aarch32 mode, strictly speaking, we should also
> can run ETMv4 driver in aarch32 mode as well.  Then CONFIG_ARM64 is the
> right choice, this can remind us that `is_kernel_in_hyp_mode()` is
> always stick to aarch64 mode.
> 

For the avoidance of confusion, in this case CONFIG_ARM64 and 
CONFIG_CORESIGHT_SOURCE_ETM4X are 100% equivalent and there's no 
functional difference. Yes maybe 32 bit userspace can be traced from 
ETMv4, but that's not really related with how this code is compiled.

>    static ssize_t format_attr_contextid_show(struct device *dev,
>                                              struct device_attribute *attr,
>                                              char *page)
>    {
>    #if IS_ENABLED(CONFIG_ARM64)
>         if (is_kernel_in_hyp_mode())
>                 return contextid2_show(dev, attr, page);
>    #endif
> 
>         return contextid1_show(dev, attr, page);

Not having an #else implies that the contextid1_show() part is valid 
when !CONFIG_ARM64, but that isn't right. That's why I had the WARN_ON 
because it's dead code.

Personally I would drop the is_visible(). It makes sense for dynamically 
hidden things, but these are all compile time. IMO it's cleaner to just 
not include them to begin with, rather than include and then hide them. 
Then the extra condition in format_attr_contextid_show() isn't needed 
because the function doesn't exist:

GEN_PMU_FORMAT_ATTR(cycacc);
GEN_PMU_FORMAT_ATTR(timestamp);
GEN_PMU_FORMAT_ATTR(retstack);
GEN_PMU_FORMAT_ATTR(sinkid);

#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)

/* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */
GEN_PMU_FORMAT_ATTR(contextid1);
/* contextid2 enables tracing CONTEXTIDR_EL2 for ETMv4 */
GEN_PMU_FORMAT_ATTR(contextid2);
GEN_PMU_FORMAT_ATTR(branch_broadcast);
/* preset - if sink ID is used as a configuration selector */
GEN_PMU_FORMAT_ATTR(preset);
/* config ID - set if a system configuration is selected */
GEN_PMU_FORMAT_ATTR(configid);
GEN_PMU_FORMAT_ATTR(cc_threshold);

/*
  * contextid always traces the "PID".  The PID is in CONTEXTIDR_EL1
  * when the kernel is running at EL1; when the kernel is at EL2,
  * the PID is in CONTEXTIDR_EL2.
  */
static ssize_t format_attr_contextid_show(struct device *dev,
					  struct device_attribute *attr,
					  char *page)
{
	if (is_kernel_in_hyp_mode())
		return contextid2_show(dev, attr, page);
	return contextid1_show(dev, attr, page);
}

static struct device_attribute format_attr_contextid =
	__ATTR(contextid, 0444, format_attr_contextid_show, NULL);
#endif

/*
  * ETMv3 only uses the first 3 attributes for programming itself (see
  * ETM3X_SUPPORTED_OPTIONS). Sink ID is also supported for selecting a
  * sink in both, but not used for configuring the ETM. The remaining
  * attributes are ETMv4 specific.
  */
static struct attribute *etm_config_formats_attr[] = {
	&format_attr_cycacc.attr,
	&format_attr_timestamp.attr,
	&format_attr_retstack.attr,
	&format_attr_sinkid.attr,
#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
	&format_attr_contextid.attr,
	&format_attr_contextid1.attr,
	&format_attr_contextid2.attr,
	&format_attr_preset.attr,
	&format_attr_configid.attr,
	&format_attr_branch_broadcast.attr,
	&format_attr_cc_threshold.attr,
#endif
	NULL,
};
>    }
> 
> Thanks,
> Leo