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

[Jim Mattson]

On Sun, Oct 18, 2026 at 4:32 AM Ravi Bangoria <ravi.bangoria@xxxxxxx> wrote:
>
> Jim,
>
> Thanks for the detailed review.
>
> >> The IBS_{FETCH|OP}_CTL2[Dis] bit allows IBS sampling to be stopped without
> >> a RMW of IBS_{FETCH,OP}_CTL. Systems providing this bit therefore do not
> >> require the IBS_STARTED and IBS_STOPPED states used to coordinate control
> >> MSR updates with late NMIs.
> >
> > CTL2[Dis] doesn't make late NMIs go away. Hence, this patch needs a
> > 50us poll. With the two-line fix in my original patch, the existing
> > scheme handles late NMIs indefinitely: IBS_STOPPED remains set until a
> > late NMI consumes it, even if the event has been restarted in the
> > meantime. This patch trades that for a 50us window, after which a late
> > NMI is unknown (which *is* fatal with unknown_nmi_panic). Moreover,
> > since we still have to handle hardware without IBS_CAPS_DIS, we now
> > have two state machines instead of one.
>
> IBS_STOPPED only records that "a late NMI might exist". It can't tell
> whether the disable actually completed or whether that late NMI has
> already arrived. Trying to infer from software state is fundamentally
> broken, since the WRMSR and a software state transition can't be made
> atomic.
>
> Given that Zen6 provides a clear hardware state machine, I believe using
> it, rather than a software state machine, is the right approach.
>
> I ran a fuzzer for several hours with my patch (+Manali's patch [1]) and
> saw no Unknown NMIs on Zen6.
>
> However, With your patch[2] (+Manali's patch), the same fuzzer continued
> to produce quite a few Unknown NMIs on Zen6. One race I was able to
> identify (with your patch applied) is described below.
>
> ---
>   o FETCH and OP both are enabled.
>
>   perf_ibs_stop() /* For OP */
>   {
>        set IBS_STOPPED
>        op state: ENABLED | STARTED |
>                  STOPPING | STOPPED
>
>        OpVal is 0 in hardware
>
>                                                NMI due to FETCH
>                                                {
>                                                     FETCH: Val=1, sample handled,
>                                                     fetch re-armed
>
>                                                     OP: STARTED is set, OpVal is 0 so
>                                                     test_and_clear(IBS_STOPPED)
>                                                     clears the STOPPED state
>
>                                                     OP continues to run, it overflows,
>                                                     sets OpVal=1 and a new NMI is
>                                                     latched by the OP hardware
>
>                                                     return handled=2
>                                                }
>
>        op state: ENABLED | STARTED |
>                  STOPPING
>
>        WRMSR OpCtl2.Dis=1
>        leaves the valid sample in OpCtl
>
>        clear IBS_STARTED
>
>        op state: ENABLED | STOPPING
>
>                                                NMI (late) due to OP
>                                                {
>                                                     FETCH: Val=0 -> return 0
>
>                                                     OP: STARTED is clear and
>                                                     STOPPED is already clear.
>
>                                                     return 0 without reading
>                                                     IbsOpCtl
>
>                                                     return handled=0
>
>                                                     Unknown NMI
>                                                }
> ---
>
> [1]: https://lore.kernel.org/r/20260310060022.15120-2-manali.shukla@xxxxxxx
> [2]: https://lore.kernel.org/r/20261007234815.4026028-1-jmattson@xxxxxxxxxx
>
> [...]
>
> >> +/* ->stop() for platforms with additional CTL2[Dis] bit. */
> >> +static void perf_ibs_stop_dis(struct perf_event *event, int flags)
> >> +{
> >> +       struct hw_perf_event *hwc = &event->hw;
> >> +       struct perf_ibs *perf_ibs = container_of(event->pmu, struct perf_ibs, pmu);
> >> +       struct cpu_perf_ibs *pcpu = this_cpu_ptr(perf_ibs->pcpu);
> >> +       u64 config;
> >> +       u64 ctl2;
> >> +
> >> +       WARN_ON_ONCE(test_bit(IBS_STOPPING, pcpu->state));
> >> +
> >> +       set_bit(IBS_STOPPING, pcpu->state);
> >
> > This function needs to be reentrant. If an IBS event is a sibling in a
> > group led by a core PMU event, and the leader is throttled,
> > perf_event_throttle_group() will stop the IBS sibling from the core
> > PMU's NMI handler:
> >
> >     amd_pmu_v2_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()
>
> Core and IBS, both are of hardware PMU type and thus can not be
> grouped together.

Ah. In that case, perf_ibs_stop() can only be reached in NMI context
from perf_ibs_handle_irq(), so I retract my objection.

> [...]
>
> >> +       if (in_nmi())
> >> +               wrmsrq(hwc->config_base, config);
> >
> > As we saw above, in_nmi() doesn't mean that we're in
> > perf_ibs_handle_irq() for this sample. If IBS Val=1 and the IBS NMI is
> > still pending, this clears Val. When the IBS NMI arrives,
> > perf_ibs_handle_irq() finds CTL2[Dis]=1 and Val=0, and therefore
> > returns 0, leading to an unknown NMI.
>
> The back-to-back NMI handling logic in the x86 generic NMI code should
> already handle this. No?

Given the skid that the 50us poll is designed to accommodate, I'm not
sure that the back-to-back code always handles it, but since
perf_ibs_stop() can only be reached in NMI context from
perf_ibs_handle_irq(), I retract this objection, too.

There are still problems for older hardware (e.g. the race you
identified above predates my change), but that's a different
discussion.

Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx>