* [PATCH 0/3] perf/amd/ibs: Assorted fixes
@ 2026-10-15 6:04 Ravi Bangoria
2026-10-15 6:04 ` [PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume Ravi Bangoria
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ravi Bangoria @ 2026-10-15 6:04 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Jim Mattson
Cc: Ravi Bangoria, Jayapal Ashwin Nair, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, x86, linux-perf-users, linux-kernel,
Santosh Shukla, Ananth Narayan, Sandipan Das, Manali Shukla
#1 Set IBS MSRs in syscore->resume() callback
#2 Fix kernel address leaking through REGS_USER
#3 Fix races by reducing SW states when IBS_CAPS_DIS is present
Ravi Bangoria (3):
perf/amd/ibs: Reset CPU0 IBS state on syscore resume
perf/amd/ibs: Fix kernel address leak via REGS_USER
perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS
arch/x86/events/amd/ibs.c | 156 +++++++++++++++++++++++++++++++-------
1 file changed, 129 insertions(+), 27 deletions(-)
--
2.55.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume 2026-10-15 6:04 [PATCH 0/3] perf/amd/ibs: Assorted fixes Ravi Bangoria @ 2026-10-15 6:04 ` Ravi Bangoria 2026-10-15 20:01 ` Jim Mattson 2026-10-15 6:04 ` [PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER Ravi Bangoria 2026-10-15 6:04 ` [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS Ravi Bangoria 2 siblings, 1 reply; 8+ messages in thread From: Ravi Bangoria @ 2026-10-15 6:04 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Jim Mattson Cc: Ravi Bangoria, Jayapal Ashwin Nair, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers, x86, linux-perf-users, linux-kernel, Santosh Shukla, Ananth Narayan, Sandipan Das, Manali Shukla During a suspend/resume cycle, all CPUs except CPU0 go through hotplug callbacks, which sets up IBS state. CPU0 bypasses this path, leaving its IBS state stale after resume. Fix this by explicitly setting CPU0's IBS state in the syscore ->resume() callback. Fixes: efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race") Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> --- arch/x86/events/amd/ibs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c index 3531f9c23b8c..bfb51f8e184f 100644 --- a/arch/x86/events/amd/ibs.c +++ b/arch/x86/events/amd/ibs.c @@ -1948,7 +1948,7 @@ static int perf_ibs_suspend(void *data) static void perf_ibs_resume(void *data) { ibs_eilvt_setup(); - setup_APIC_ibs(); + x86_pmu_amd_ibs_starting_cpu(smp_processor_id()); } static const struct syscore_ops perf_ibs_syscore_ops = { -- 2.55.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume 2026-10-15 6:04 ` [PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume Ravi Bangoria @ 2026-10-15 20:01 ` Jim Mattson 0 siblings, 0 replies; 8+ messages in thread From: Jim Mattson @ 2026-10-15 20:01 UTC (permalink / raw) To: Ravi Bangoria Cc: Peter Zijlstra, Ingo Molnar, Jayapal Ashwin Nair, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers, x86, linux-perf-users, linux-kernel, Santosh Shukla, Ananth Narayan, Sandipan Das, Manali Shukla On Wed, Oct 14, 2026 at 11:04 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > During a suspend/resume cycle, all CPUs except CPU0 go through hotplug > callbacks, which sets up IBS state. CPU0 bypasses this path, leaving its > IBS state stale after resume. Fix this by explicitly setting CPU0's IBS > state in the syscore ->resume() callback. > > Fixes: efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race") > Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> Reviewed-by: Jim Mattson <jmattson@google.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER 2026-10-15 6:04 [PATCH 0/3] perf/amd/ibs: Assorted fixes Ravi Bangoria 2026-10-15 6:04 ` [PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume Ravi Bangoria @ 2026-10-15 6:04 ` Ravi Bangoria 2026-10-15 20:42 ` Jim Mattson 2026-10-15 6:04 ` [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS Ravi Bangoria 2 siblings, 1 reply; 8+ messages in thread From: Ravi Bangoria @ 2026-10-15 6:04 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Jim Mattson Cc: Ravi Bangoria, Jayapal Ashwin Nair, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers, x86, linux-perf-users, linux-kernel, Santosh Shukla, Ananth Narayan, Sandipan Das, Manali Shukla IBS hardware captures a sample and raises an NMI. Since the NMI can be delivered with a delay, a privilege change may occur between sample capture and NMI delivery. Because IBS only provides the sample RIP, the kernel records GPRs from the NMI pt_regs, which may reflect kernel mode if a privilege switch occurred in the interim. This can leak kernel addresses to unprivileged users through PERF_SAMPLE_REGS_USER. Fix this by discarding samples where the NMI arrives in kernel mode and the user has set exclude_kernel. Reported-by: Jayapal Ashwin Nair <janair@amd.com> Fixes: d29e744c7167 ("perf/x86: Relax privilege filter restriction on AMD IBS") Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> --- arch/x86/events/amd/ibs.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c index bfb51f8e184f..3cb46c775eaa 100644 --- a/arch/x86/events/amd/ibs.c +++ b/arch/x86/events/amd/ibs.c @@ -1373,12 +1373,15 @@ static bool perf_ibs_is_kernel_br_target(struct perf_event *event, } static bool perf_ibs_discard_sample(struct perf_ibs *perf_ibs, struct perf_event *event, - struct pt_regs *regs, struct perf_ibs_data *ibs_data, - int br_target_idx) + struct pt_regs *regs, struct pt_regs *iregs, + struct perf_ibs_data *ibs_data, int br_target_idx) { if (perf_exclude_event(event, regs)) return true; + if (perf_exclude_event(event, iregs)) + return true; + if (perf_ibs != &perf_ibs_op || !event->attr.exclude_kernel) return false; @@ -1529,7 +1532,7 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs) if (((ibs_caps & IBS_CAPS_BIT63_FILTER) || (event->attr.config2 & IBS_SW_FILTER_MASK)) && - perf_ibs_discard_sample(perf_ibs, event, ®s, &ibs_data, br_target_idx)) { + perf_ibs_discard_sample(perf_ibs, event, ®s, iregs, &ibs_data, br_target_idx)) { throttle = perf_event_account_interrupt(event); goto out; } -- 2.55.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER 2026-10-15 6:04 ` [PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER Ravi Bangoria @ 2026-10-15 20:42 ` Jim Mattson 0 siblings, 0 replies; 8+ messages in thread From: Jim Mattson @ 2026-10-15 20:42 UTC (permalink / raw) To: Ravi Bangoria Cc: Peter Zijlstra, Ingo Molnar, Jayapal Ashwin Nair, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers, x86, linux-perf-users, linux-kernel, Santosh Shukla, Ananth Narayan, Sandipan Das, Manali Shukla On Wed, Oct 14, 2026 at 11:05 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > IBS hardware captures a sample and raises an NMI. Since the NMI can be > delivered with a delay, a privilege change may occur between sample > capture and NMI delivery. Because IBS only provides the sample RIP, the > kernel records GPRs from the NMI pt_regs, which may reflect kernel mode > if a privilege switch occurred in the interim. This can leak kernel > addresses to unprivileged users through PERF_SAMPLE_REGS_USER. > > Fix this by discarding samples where the NMI arrives in kernel mode and > the user has set exclude_kernel. This will systematically under-sample instructions just before a transition to the kernel. For this special case, perhaps the GPRs could be obtained from task_pt_regs(current) instead of the NMI pt_regs? > Reported-by: Jayapal Ashwin Nair <janair@amd.com> > Fixes: d29e744c7167 ("perf/x86: Relax privilege filter restriction on AMD IBS") > Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> > --- > arch/x86/events/amd/ibs.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c > index bfb51f8e184f..3cb46c775eaa 100644 > --- a/arch/x86/events/amd/ibs.c > +++ b/arch/x86/events/amd/ibs.c > @@ -1373,12 +1373,15 @@ static bool perf_ibs_is_kernel_br_target(struct perf_event *event, > } > > static bool perf_ibs_discard_sample(struct perf_ibs *perf_ibs, struct perf_event *event, > - struct pt_regs *regs, struct perf_ibs_data *ibs_data, > - int br_target_idx) > + struct pt_regs *regs, struct pt_regs *iregs, > + struct perf_ibs_data *ibs_data, int br_target_idx) > { > if (perf_exclude_event(event, regs)) > return true; > > + if (perf_exclude_event(event, iregs)) > + return true; This excludes events with an NMI skid past a privilege change in both directions. Do you want to omit a kernel sample taken just before returning to userspace if the NMI arrives in userspace and exclude_user is set? ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS 2026-10-15 6:04 [PATCH 0/3] perf/amd/ibs: Assorted fixes Ravi Bangoria 2026-10-15 6:04 ` [PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume Ravi Bangoria 2026-10-15 6:04 ` [PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER Ravi Bangoria @ 2026-10-15 6:04 ` Ravi Bangoria 2026-10-15 21:32 ` Jim Mattson 2 siblings, 1 reply; 8+ messages in thread From: Ravi Bangoria @ 2026-10-15 6:04 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Jim Mattson Cc: Ravi Bangoria, Jayapal Ashwin Nair, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers, x86, linux-perf-users, linux-kernel, Santosh Shukla, Ananth Narayan, Sandipan Das, Manali Shukla 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. Add a separate stop path for these systems. Preserve Val while disabling the event so that a delayed NMI can be identified and consumed, and wait briefly for such an NMI before restarting the event. Leave the existing handling unchanged on systems without IBS_CAPS_DIS. Reported-by: Jim Mattson <jmattson@google.com> Closes: https://lore.kernel.org/r/20261007234815.4026028-1-jmattson@google.com Fixes: efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race") Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> --- arch/x86/events/amd/ibs.c | 145 ++++++++++++++++++++++++++++++++------ 1 file changed, 122 insertions(+), 23 deletions(-) diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c index 3cb46c775eaa..f49f30f53399 100644 --- a/arch/x86/events/amd/ibs.c +++ b/arch/x86/events/amd/ibs.c @@ -28,6 +28,7 @@ static u32 ibs_caps; #include <linux/hardirq.h> #include <asm/nmi.h> +#include <asm/delay.h> #include <asm/amd/ibs.h> /* attr.config2 */ @@ -73,7 +74,12 @@ static u32 ibs_caps; * * XXX: we could probably be using !atomic bitops for all this. */ - +/* + * NOTE: IBS_CAPS_DIS eliminates the RMW race on the IBS_{FETCH|OP}_CTL, making + * IBS_STARTED and IBS_STOPPED states unnecessary. We use only IBS_ENABLED and + * IBS_STOPPING states when IBS_CAPS_DIS is present. Semantics of IBS_STOPPING + * changes a bit as well. + */ enum ibs_states { IBS_ENABLED = 0, IBS_STARTED = 1, @@ -557,7 +563,9 @@ static void perf_ibs_start(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); + unsigned int delay = 50; u64 period, config = 0; + u64 ctl; if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED))) return; @@ -575,23 +583,87 @@ static void perf_ibs_start(struct perf_event *event, int flags) } config |= period >> 4; - /* - * Reset the IBS_{FETCH|OP}_CTL MSR before updating pcpu->state. - * Doing so prevents a race condition in which an NMI due to other - * source might accidentally activate the event before we enable - * it ourselves. - */ - perf_ibs_disable_event(perf_ibs, hwc, 0); + 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); + } + } else { + /* + * Reset the IBS_{FETCH|OP}_CTL MSR before updating pcpu->state. + * Doing so prevents a race condition in which an NMI due to + * other source might accidentally activate the event before we + * enable it ourselves. + */ + perf_ibs_disable_event(perf_ibs, hwc, 0); + /* + * Set STARTED before enabling the hardware, such that a + * subsequent NMI must observe it. + */ + set_bit(IBS_STARTED, pcpu->state); + clear_bit(IBS_STOPPING, pcpu->state); + } + perf_ibs_enable_event(perf_ibs, hwc, config); + + perf_event_update_userpage(event); +} + +/* ->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); + + rdmsrq(hwc->extra_reg.reg, ctl2); + if (ctl2 & perf_ibs->disable_mask) + goto reset_state; + + wrmsrq(hwc->extra_reg.reg, perf_ibs->disable_mask); + + WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED); + hwc->state |= PERF_HES_STOPPED; + + if (hwc->state & PERF_HES_UPTODATE) + goto reset_state; + + rdmsrq(hwc->config_base, config); + + config &= ~perf_ibs->valid_mask; /* - * Set STARTED before enabling the hardware, such that a subsequent NMI - * must observe it. + * Clear the Val bit otherwise ->start() will unnecessarily + * wait for 50us. */ - set_bit(IBS_STARTED, pcpu->state); - clear_bit(IBS_STOPPING, pcpu->state); - perf_ibs_enable_event(perf_ibs, hwc, config); + if (in_nmi()) + wrmsrq(hwc->config_base, config); - perf_event_update_userpage(event); + perf_ibs_event_update(perf_ibs, event, &config); + hwc->state |= PERF_HES_UPTODATE; + +reset_state: + clear_bit(IBS_STOPPING, pcpu->state); } static void perf_ibs_stop(struct perf_event *event, int flags) @@ -602,6 +674,9 @@ static void perf_ibs_stop(struct perf_event *event, int flags) u64 config; int stopping; + if (ibs_caps & IBS_CAPS_DIS) + return perf_ibs_stop_dis(event, flags); + if (test_and_set_bit(IBS_STOPPING, pcpu->state)) return; @@ -1419,19 +1494,43 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs) unsigned int msr; u64 *buf, *config, period, new_config = 0; int br_target_idx = -1; + u64 ctl2, ctl; - if (!test_bit(IBS_STARTED, pcpu->state)) { -fail: + if (ibs_caps & IBS_CAPS_DIS) { /* - * 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. + * ->stop() { + * set_bit(IBS_STOPPING); + * <-- NMI (race 1) + * CTL2[Dis]=1; + * ... <-- NMI (race 2) + * } */ - if (test_and_clear_bit(IBS_STOPPED, pcpu->state)) - return 1; + rdmsrq(perf_ibs->msr2, ctl2); + if (test_bit(IBS_STOPPING, pcpu->state) /* race 1 */ || + (ctl2 & perf_ibs->disable_mask) /* race 2 */) { + rdmsrq(perf_ibs->msr, ctl); + if (ctl & perf_ibs->valid_mask) { + ctl &= ~(perf_ibs->enable_mask | + perf_ibs->valid_mask); + wrmsrq(perf_ibs->msr, ctl); + return 1; + } + return 0; + } + } 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; + } } if (WARN_ON_ONCE(!event)) -- 2.55.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS 2026-10-15 6:04 ` [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS Ravi Bangoria @ 2026-10-15 21:32 ` Jim Mattson 2026-10-18 11:31 ` Ravi Bangoria 0 siblings, 1 reply; 8+ messages in thread From: Jim Mattson @ 2026-10-15 21:32 UTC (permalink / raw) To: Ravi Bangoria Cc: Peter Zijlstra, Ingo Molnar, Jayapal Ashwin Nair, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers, x86, linux-perf-users, linux-kernel, Santosh Shukla, Ananth Narayan, Sandipan Das, Manali Shukla On Wed, Oct 14, 2026 at 11:05 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote: > > 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. > Add a separate stop path for these systems. Preserve Val while disabling > the event so that a delayed NMI can be identified and consumed, and wait > briefly for such an NMI before restarting the event. > > Leave the existing handling unchanged on systems without IBS_CAPS_DIS. > > Reported-by: Jim Mattson <jmattson@google.com> > Closes: https://lore.kernel.org/r/20261007234815.4026028-1-jmattson@google.com > Fixes: efa5700ec0da ("perf/amd/ibs: Support IBS_{FETCH|OP}_CTL2[Dis] to eliminate RMW race") > Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> > --- > arch/x86/events/amd/ibs.c | 145 ++++++++++++++++++++++++++++++++------ > 1 file changed, 122 insertions(+), 23 deletions(-) [ ... ] > +/* ->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() [ .. ] > + 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS 2026-10-15 21:32 ` Jim Mattson @ 2026-10-18 11:31 ` Ravi Bangoria 0 siblings, 0 replies; 8+ messages in thread From: Ravi Bangoria @ 2026-10-18 11:31 UTC (permalink / raw) To: Jim Mattson Cc: Peter Zijlstra, Ingo Molnar, Jayapal Ashwin Nair, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers, x86, linux-perf-users, linux-kernel, Santosh Shukla, Ananth Narayan, Sandipan Das, Manali Shukla, Ravi Bangoria 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-10-18 11:32 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-10-15 6:04 [PATCH 0/3] perf/amd/ibs: Assorted fixes Ravi Bangoria 2026-10-15 6:04 ` [PATCH 1/3] perf/amd/ibs: Reset CPU0 IBS state on syscore resume Ravi Bangoria 2026-10-15 20:01 ` Jim Mattson 2026-10-15 6:04 ` [PATCH 2/3] perf/amd/ibs: Fix kernel address leak via REGS_USER Ravi Bangoria 2026-10-15 20:42 ` Jim Mattson 2026-10-15 6:04 ` [PATCH 3/3] perf/amd/ibs: Simplify event state handling with IBS_CAPS_DIS Ravi Bangoria 2026-10-15 21:32 ` Jim Mattson 2026-10-18 11:31 ` Ravi Bangoria
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®