Re: [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf/amd/ibs: Unhandled NMI panic due to 50us timeout
--

commit be4cf40ba3750348ffc77933b0cfe40c48e335e7
Author: Ravi Bangoria <ravi.bangoria@amd.com>
Subject: perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS

This commit simplifies event state handling on systems supporting
IBS_CAPS_DIS. It introduces a separate stop path that preserves the
Val bit while disabling the event to catch delayed NMIs.

Link: https://lore.kernel.org/r/20261007234815.4026028-1-jmattson@google.com

> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c

[ ... ]

> @@ -575,23 +583,87 @@ static void perf_ibs_start(struct perf_event *event, int flags)

[ ... ]

> +	if (ibs_caps & IBS_CAPS_DIS) {
> +		/*
> +		 *      IBS HW                     SW
> +		 *
> +		 *  o Capture sample       o ->stop()
> +		 *  o Set VAL bit            CTL2[Dis] = 1
> +		 *  o Raise NMI --.        o ...
> +		 *                 \       o ->start()
> +		 *                  \        wrmsr(IBS_{FETCH|OP}_CTL, new value);
> +		 *                   \       (wrmsr clears the VAL bit)
> +		 *                    `--> o Receives delayed NMI
> +		 *                           (Unknown NMI since VAL bit is cleared)
> +		 *
> +		 * Handle this with induced delay.
> +		 */
> +		rdmsrq(hwc->config_base, ctl);
> +		while ((ctl & perf_ibs->valid_mask) && delay--) {
> +			udelay(1);
> +			rdmsrq(hwc->config_base, ctl);
> +		}

[Severity: High]
What happens if the delayed NMI takes longer than 50us to arrive, for
example due to hypervisor scheduling, nested virtualization, or SMIs?

If this delay loop expires, the code proceeds to call
perf_ibs_enable_event() which clears the VAL and CTL2[Dis] bits. When the
delayed NMI eventually arrives, it appears it will be treated as an
unhandled NMI.

[ ... ]

> @@ -1419,19 +1494,43 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)

[ ... ]

> +	} else {
> +		if (!test_bit(IBS_STARTED, pcpu->state)) {
> +fail:
> +			/*
> +			 * Catch spurious interrupts after stopping IBS: After
> +			 * disabling IBS there could be still incoming NMIs
> +			 * with samples that even have the valid bit cleared.
> +			 * Mark all this NMIs as handled.
> +			 */
> +			if (test_and_clear_bit(IBS_STOPPED, pcpu->state))
> +				return 1;
>  
> -		return 0;
> +			return 0;
> +		}
> +	}

[Severity: High]
If the NMI is delayed past the 50us timeout in perf_ibs_start(), won't
perf_ibs_handle_irq() return 0 here?

Since IBS_STOPPED is intentionally not set for systems with IBS_CAPS_DIS,
when the delayed NMI arrives, CTL2[Dis] == 0 and VAL == 0. If the code
jumps to the 'fail' label due to the cleared VAL bit, IBS_STOPPED will be
clear, causing it to return 0.

Could this result in an unhandled NMI and a subsequent kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015060430.5877-1-ravi.bangoria@amd.com?part=3