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