Re: [PATCH RFC] perf/x86/amd/lbr: Don't freeze the LBR on PMI for user-only filters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf/x86/amd/lbr: freeze format exposed and accepted without feature check
- [Medium] perf/x86/amd/lbr: wrong freeze state in amd_pmu_lbr_enable_all() for multi-event LBR
--

commit 4ee339b2b2fdce2f2f85b09e93b75d05b24049b1
Author: Puranjay Mohan <puranjay@kernel.org>

perf/x86/amd/lbr: Don't freeze the LBR on PMI for user-only filters

This commit skips setting DEBUGCTLMSR_FREEZE_LBRS_ON_PMI on PMI when kernel
branches are suppressed in LBR_SELECT, while adding a freeze format attribute
(config2:0) to allow user space to explicitly request freezing the LBR.

> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 49b6b8fce5664..d63649c756a58 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -1404,8 +1404,22 @@ static struct attribute_group group_events_amd_brs = {
>  
>  #endif	/* CONFIG_PERF_EVENTS_AMD_BRS */
>  
> +PMU_FORMAT_ATTR(freeze, "config2:0");
> +
> +static struct attribute *amd_pmu_lbr_format_attrs[] = {
> +	&format_attr_freeze.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group group_format_amd_lbr = {
> +	.name       = "format",
> +	.attrs      = amd_pmu_lbr_format_attrs,
> +	.is_visible = amd_branches_is_visible,
> +};
> +
>  static const struct attribute_group *amd_attr_update[] = {
>  	&group_caps_amd_branches,
> +	&group_format_amd_lbr,
>  #ifdef CONFIG_PERF_EVENTS_AMD_BRS
>  	&group_events_amd_brs,
>  #endif
> diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c
> index a55646fcb8465..f45e2c53566af 100644
> --- a/arch/x86/events/amd/lbr.c
> +++ b/arch/x86/events/amd/lbr.c
> @@ -10,6 +10,12 @@
>  /* LBR Branch Select valid bits */
>  #define LBR_SELECT_MASK		0x1ff
>  
> +/* attr.config2: freeze the LBR on PMI */
> +#define LBR_CFG2_FREEZE		BIT_ULL(0)
> +
> +/* Software only, masked out before writing LBR_SELECT */
> +#define LBR_FREEZE		BIT_ULL(63)
> +
>  /*
>   * LBR Branch Select filter bits which when set, ensures that the
>   * corresponding type of branches are not recorded
> @@ -307,6 +313,9 @@ static int amd_pmu_lbr_setup_filter(struct perf_event *event)
>  	/* Filter bits operate in suppress mode */
>  	reg->config = mask ^ LBR_SELECT_MASK;
>  
> +	if (event->attr.config2 & LBR_CFG2_FREEZE)
> +		reg->config |= LBR_FREEZE;
> +
>  	return 0;
>  }

[Severity: Medium]
Should group_format_amd_lbr and amd_pmu_lbr_setup_filter() check for
X86_FEATURE_AMD_LBR_PMC_FREEZE?

In arch/x86/events/amd/core.c, group_format_amd_lbr uses
amd_branches_is_visible():

static umode_t
amd_branches_is_visible(struct kobject *kobj, struct attribute *attr, int i)
{
	return x86_pmu.lbr_nr ? attr->mode : 0;
}

Because x86_pmu.lbr_nr is non-zero both on Zen 3 BRS systems (where
amd_brs_detect() sets x86_pmu.lbr_nr = 16) and on LBRv2 systems or VMs that do
not advertise X86_FEATURE_AMD_LBR_PMC_FREEZE, the sysfs format attribute
/sys/bus/event_source/devices/cpu/format/freeze is exposed on systems that
cannot freeze the LBR on PMI.

In addition, when a user opens an event with LBR_CFG2_FREEZE set in
event->attr.config2 on those systems, amd_pmu_lbr_setup_filter() accepts it
and sets LBR_FREEZE in reg->config, while amd_pmu_lbr_enable_all() silently
ignores it because the MSR_IA32_DEBUGCTLMSR update is guarded by
cpu_feature_enabled(X86_FEATURE_AMD_LBR_PMC_FREEZE).

Should the visibility callback and amd_pmu_lbr_setup_filter() verify
cpu_feature_enabled(X86_FEATURE_AMD_LBR_PMC_FREEZE) and return -EOPNOTSUPP if
freeze is requested when unsupported?

> @@ -392,20 +401,26 @@ void amd_pmu_lbr_sched_task(struct perf_event_pmu_context *pmu_ctx,
>  void amd_pmu_lbr_enable_all(void)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> -	u64 lbr_select, dbg_ctl, dbg_extn_cfg;
> +	u64 lbr_cfg = 0, lbr_select = 0, dbg_ctl, dbg_extn_cfg;
>  
>  	if (!cpuc->lbr_users || !x86_pmu.lbr_nr)
>  		return;
>  
>  	/* Set hardware branch filter */
>  	if (cpuc->lbr_select) {
> -		lbr_select = cpuc->lbr_sel->config & LBR_SELECT_MASK;
> +		lbr_cfg = cpuc->lbr_sel->config;
> +		lbr_select = lbr_cfg & LBR_SELECT_MASK;
>  		wrmsrq(MSR_AMD64_LBR_SELECT, lbr_select);
>  	}
>  
>  	if (cpu_feature_enabled(X86_FEATURE_AMD_LBR_PMC_FREEZE)) {
>  		rdmsrq(MSR_IA32_DEBUGCTLMSR, dbg_ctl);
> -		wrmsrq(MSR_IA32_DEBUGCTLMSR, dbg_ctl | DEBUGCTLMSR_FREEZE_LBRS_ON_PMI);
> +		/* Skip the freeze if kernel branches are suppressed, unless asked */
> +		if ((lbr_select & LBR_KERNEL) && !(lbr_cfg & LBR_FREEZE))
> +			dbg_ctl &= ~DEBUGCTLMSR_FREEZE_LBRS_ON_PMI;
> +		else
> +			dbg_ctl |= DEBUGCTLMSR_FREEZE_LBRS_ON_PMI;
> +		wrmsrq(MSR_IA32_DEBUGCTLMSR, dbg_ctl);
>  	}

[Severity: Medium]
Can this re-enable DEBUGCTLMSR_FREEZE_LBRS_ON_PMI for an active user-only LBR
event when multiple branch-stack events are scheduled on the same CPU?

In arch/x86/events/amd/lbr.c, amd_pmu_lbr_add() sets cpuc->lbr_select = 1 and
increments cpuc->lbr_users, but amd_pmu_lbr_del() unconditionally clears
cpuc->lbr_select to 0 whenever any branch-stack event is removed:

void amd_pmu_lbr_del(struct perf_event *event)
{
	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);

	if (!x86_pmu.lbr_nr)
		return;

	if (has_branch_stack(event))
		cpuc->lbr_select = 0;

	cpuc->lbr_users--;
...
}

If two user-only branch-stack events are active on a CPU (cpuc->lbr_users == 2)
and one of them is removed via amd_pmu_lbr_del(), cpuc->lbr_users remains 1
 while cpuc->lbr_select becomes 0.

When amd_pmu_v2_enable_all() then calls amd_pmu_lbr_enable_all(), the
if (cpuc->lbr_select) block is skipped, leaving lbr_cfg and lbr_select as 0.
As a result, (lbr_select & LBR_KERNEL) evaluates to false and
amd_pmu_lbr_enable_all() sets DEBUGCTLMSR_FREEZE_LBRS_ON_PMI in
MSR_IA32_DEBUGCTLMSR even though the remaining active event suppresses kernel
branches and did not request a freeze.

Additionally, if a user-only event with freeze=0 is added alongside an event
that requires freezing (either recording kernel branches or with freeze=1),
amd_pmu_lbr_add() overwrites cpuc->lbr_sel->config with the new event's config,
causing amd_pmu_lbr_enable_all() to clear DEBUGCTLMSR_FREEZE_LBRS_ON_PMI for
both events.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105170340.165507-1-puranjay@kernel.org?part=1