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

[Ravi Bangoria <ravi.bangoria@amd.com>]

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@amd.com
[2]: https://lore.kernel.org/r/20261007234815.4026028-1-jmattson@google.com

[...]

>> +/* ->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.

[...]

>> +       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?

Thanks,
Ravi