Re: [PATCH] perf/amd/ibs: Clear stale IBS_{FETCH|OP}_CTL in perf_ibs_start() with CTL2[Dis]

[Jim Mattson]

On Fri, Oct 09, 2026 at 09:19:49AM +0530, Ravi Bangoria wrote:
> Since the additional disable bit in the independent control register
> eliminates the RMW write race, we can simplify the logic by removing
> the IBS_STARTED and IBS_STOPPED software states. However, we may still
> need to retain IBS_STOPPING, since ->stop() can race with NMIs, and
> NMIs also arrive with a delay.
>
> Can you please review this. I'll send a formal patch after testing
> it thoroughly.

Your approach also fixes the race, but I'm not convinced that it's an
improvement. There's a lot more complexity here.

[...]

> +	if (config & perf_ibs->valid_mask) {
> +		ctl = config;
> +		while (delay-- && ctl & perf_ibs->valid_mask) {
> +			udelay(1);
> +			rdmsrq(hwc->config_base, ctl);
> +		}
> +	}

This loop will time out when running in NMI context:

    perf_ibs_handle_irq() ->
      perf_event_overflow() ->
        __perf_event_overflow() ->
          __perf_event_account_interrupt() ->
            perf_event_throttle_group() ->
              perf_event_throttle() ->
                perf_ibs_stop() ->
                  perf_ibs_stop_dis() 

Should the loop just be skipped when in_nmi()?

[...]

> +	/* Catch delayed NMIs which arrives after disabling IBS PMUs. */
> +	if (ibs_caps & IBS_CAPS_DIS) {
> +		rdmsrq(perf_ibs->msr2, ctl2);
> +		if (ctl2 & perf_ibs->disable_mask) {
> +			rdmsrq(perf_ibs->msr, ctl);
> +			if (ctl & perf_ibs->valid_mask) {
> +				ctl &= ~(perf_ibs->enable_mask &
> +					 perf_ibs->valid_mask);

enable_mask and valid_mask are disjoint. Should '&' be '|'?

> +				wrmsrq(perf_ibs->msr, ctl);
> +				return 1;
> +			}
> +			return 0;
> +		}
> +

[...]

>       if (WARN_ON_ONCE(!event))

With IBS_CAPS_DIS, we are now treating CTL2[Dis] as authoritative on the
question of "is IBS idle." However, on the BSP after S3 resume or on any
LPU after an AE #VMEXIT from an SEV-ES/SNP guest with IBS virtualization
enabled, CTL2[Dis] will be reset to 0 and may no longer be authoritative.

> I think the intention is, host IBS should be disabled (could be from
> CTL2[Dis]) before running VMRUN. But I'll confirm with the HW team.

Another question for the HW team: can the hardware set CTL[Val], or change
CTL in any other way, after CTL2[Dis] has been set? The answer may
obviate the need for the 50us poll.

--jim