Details

Reviewers

ali_mashtizadeh.com
mhorne
gnn

Summary

Add kernel and userland support for Zen6 IBS extensions per AMD pub
69205 (rev 1.00, March 2026): alternate fetch/op disable via ctl2[0],
fetch latency filtering, virtual address bit 63 filtering, and
streaming-store filtering. Decode the new IbsOpData2 StrmSt and
RmtSocket bits. Update libpmc, pmcstat and manpage.

Pre-Zen6 systems work unchanged with ibs_ctl2 == 0.

Sponsored by: AMD
Signed-off-by: Andre Silva <andasilv@amd.com>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 72960
Build 69843: arc lint + arc unit

Event Timeline

afscoelho_gmail.com created this revision.Sun, May 10, 3:04 AM

afscoelho_gmail.com held this revision as a draft.

Herald added a reviewer: gnn. · View Herald TranscriptSun, May 10, 3:04 AM

Herald added a reviewer: ali_mashtizadeh.com. · View Herald Transcript

Herald added subscribers: ziaee, imp. · View Herald Transcript

Harbormaster completed remote builds in B72960: Diff 177522.Sun, May 10, 3:04 AM

Submitting diff

Harbormaster completed remote builds in B73083: Diff 177802.Wed, May 13, 6:54 PM

afscoelho_gmail.com published this revision for review.Wed, May 13, 6:58 PM

afscoelho_gmail.com edited reviewers, added: mhorne; removed: gnn.Tue, May 19, 1:32 PM

Herald added a reviewer: gnn. · View Herald TranscriptTue, May 19, 1:32 PM

@mashtizadeh_gmail.com

I did not look in detail at all of the code, but here is some initial comments.

lib/libpmc/libpmc.c
728–734	This seems unrelated to adding Zen6 feature support...
737–743	I think this type of validation is a little messy. In most cases, it can be removed; for example, it is harmless to allow the user to specify `l3miss` multiple times.
785–802	Here we can honor the final requested value for the flag. This is typical in unix utility option parsing. So if the user requests: `addr63=0,l3miss,add63=1`, then we would take "1" as the final choice. The logical advice is "don't do this, it's confusing", but we don't really need to guard against it.
867–870	Just checking, this behaviour is correct, and not reversed?
sys/dev/hwpmc/hwpmc_ibs.c
624–626	Oh I see. Something about this scheme seems quite fragile... Can't we just report `pl_mpdata[PMC_MPIDX_FETCH_CTL2]` as zero/empty if it is not supported?
sys/dev/hwpmc/hwpmc_ibs.h
135–137	You need to bump this, no? (And `PMC_MPIDX_OP_MAX` below.)

mhorne added a subscriber: pmc.Tue, May 19, 4:02 PM

afscoelho_gmail.com added inline comments.Tue, May 19, 7:37 PM

lib/libpmc/libpmc.c
728–734	Fair point, I'll restore the CPUID read and the userland gating for (l3miss/ldlat=/opcount/OPCNTEXT rate). Kernel-side EXTERROR validation can stay and libpmc as a cheap early reject. Will keep this patch focused on the Zen6 additions.
737–743	That's ok, I'll drop the seen* variables, adopt the "last value wins" convention
867–870	That's correct, it is not reversed. Per AMD pub 69205, IbsOpCtl2 bit 1 (ExclRip63Eq0) excludes RIP[63]==0 and bit 2 (ExclRip63Eq1) excludes RIP[63]==1. addr63=0 --> keep bit63=0 --> set EXCLRIP63EQ1 addr63=1 --> keep bit63=1 --> set EXCLRIP63EQ0
sys/dev/hwpmc/hwpmc_ibs.c
624–626	Thanks! I think this was a good catch. I will make pl_length = PMC_MPIDX_FETCH_MAX unconditionally and on pre-Zen6, pl_mpdata[PMC_MPIDX_FETCH_CTL2] stays zero
sys/dev/hwpmc/hwpmc_ibs.h
135–137	yes, thanks, that was an oversight. I'll handle this as part of the next fix.

Address mhorne review feedback

Harbormaster completed remote builds in B73233: Diff 178130.Tue, May 19, 7:49 PM

Thanks for the review, mhorne. Addressed all six comments in the latest diff. PTAL when you get a chance.

The only major issue is that we shouldn't expose bit63 as a flag that's just CAP_USER and CAP_SYSTEM. The other changes are real minor. Mitchell didn't like the structure size changing but we have to support it if we want any resemblance of backwards compatibility.

lib/libpmc/libpmc.c
778	The whole point of addr63 is that it allows us to introduce kernel/user filtering without worrying about skid. I.e. the sample may have been captured in kernel mode but then is reported as a user sample because it takes potentially 10k cycles to delivery the NMI. Instead of exposing bit63 as a flag we should interpret the standard user/kernel flags that are already present as capabilities. This is why I left the CAP_USER/SYSTEM stuff a bit unfinished since we didn't really have a way to deal with it. So I'd rather the standard user/kernel flags trigger setting the capability flags that we 'or' in, in the kernel similar to other counters.
854	Same here this should just be user/system cap flags.
lib/libpmc/pmc.ibs.3
92	and here with a (zen 6)
101	Update this to say on zen6 we support user/kernel filtering.
sys/dev/hwpmc/hwpmc_ibs.c
83	I'd prefer you move this up above so fetch and op aren't interleaved.
192	Again I think your doing too much validation in the kernel we should land my safe msr change and not introduce complex logic. I really think it should just be a mask of valid bits and nothing else. That seems to be enough prevent GPs and I can't even reproduce a GP on zen5 seems they stopped doing it on a recent core.
449	Why define these functions when below you just manually or/and bits directly mixing and matching is a bit harder to read
563	Referring to here
624–625	Oh actually Mitchell I made that intentional as we expect this structures to grow. For backwards compatibility we will always have to look at the length field so I think its totally reasonable that we allow it change in size based on supported registers.
sys/dev/hwpmc/hwpmc_ibs.h
37	Can you add a line here where you pulled the Zen 6 specifications from?
116	Nit Zen6 -> Zen 6
196	Nit "Zen6" -> "Zen 6"

This revision now requires changes to proceed.Tue, May 19, 9:25 PM

Address ali_mashtizadeh.com review feedback

Harbormaster completed remote builds in B73317: Diff 178334.Thu, May 21, 5:35 PM

Address ali_mashtizadeh.com review feedback

Harbormaster completed remote builds in B73318: Diff 178335.Thu, May 21, 5:36 PM

Thanks for the review, Ali.

Thanks Andre for all the hard work sorry to point out one more change.

I’m concerned with those wrmsr’s and I think it was a Zen2 errata I remember I was able to reproduce. I think you should just have avoided refactoring the start code in this change or just keep the middle bit that sets the main ctl values, the zero’ing.

If you’re going to work on RAPL, can I take a lock on hwpmc_ibs.[ch] so I can rebase and merge tools.

@mhorne @imp

I need to push the my tools its going to be a series of patches and fix two things:

Improve the man page based on my experience with the tools
Add the heuristics I got from userspace to cleanup the events reported to honor CAP_USER/SYSTEM this is needed even with Bit63 filtering.

sys/dev/hwpmc/hwpmc_ibs.c
641	I think I meant to use the middle of the function of what you were trying to do I have to look back. I’m not sure this is safe here, we have an enabled IBS counter and you’re zero’ing it out. I think it’s in Zen2 errata that says zero’ing the counter isn’t safe. It will lead to invalid NMIs that we won’t ack. That’s why I have a whole dance around clearing the counter, then disabling enable, and finally zeroing it out inside of stop.
679	Same here

This revision now requires changes to proceed.Fri, May 22, 5:43 AM

In D56914#1310001, @ali_mashtizadeh.com wrote:

Thanks Andre for all the hard work sorry to point out one more change.

I’m concerned with those wrmsr’s and I think it was a Zen2 errata I remember I was able to reproduce. I think you should just have avoided refactoring the start code in this change or just keep the middle bit that sets the main ctl values, the zero’ing.

If you’re going to work on RAPL, can I take a lock on hwpmc_ibs.[ch] so I can rebase and merge tools.

@mhorne @imp

I need to push the my tools its going to be a series of patches and fix two things:

Improve the man page based on my experience with the tools

Add the heuristics I got from userspace to cleanup the events reported to honor CAP_USER/SYSTEM this is needed even with Bit63 filtering.

Correct! Thanks for remembering me about this errata. The zero-write was fine in ibs_start_pmc (counter off), but I was also calling the helper from the NMI handlers where IBS is still live, hitting the Family 10h #420 hazard that the stop dance avoids. Linux's perf_ibs_handle_irq doesn't touch ctl in the re-arm either.
I dropped both helpers, inlined the ctl2 init into ibs_start_pmc between the zero and enable writes, and reverted the NMI sites to the single wrmsr(ctl | ENABLE). Updated diff incoming.

On the lock : go ahead and take it. RAPL is next for me and it's a separate area, so I'll keep off hwpmc_ibs.[ch] until your tools land. Ping me when you're ready.

v2: inline start helpers, simplify NMI re-arm path

Harbormaster completed remote builds in B73350: Diff 178426.Fri, May 22, 6:55 PM

afscoelho_gmail.com marked 2 inline comments as done.Fri, May 22, 6:56 PM

hwpmc: Add Zen6 IBS ctl2 filters and alternate disable
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 177522

lib/libpmc/libpmc.c

lib/libpmc/pmc.ibs.3

sys/dev/hwpmc/hwpmc_ibs.h

sys/dev/hwpmc/hwpmc_ibs.c

usr.sbin/pmcstat/pmcstat_log.c

hwpmc: Add Zen6 IBS ctl2 filters and alternate disableNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 177522

lib/libpmc/libpmc.c

lib/libpmc/pmc.ibs.3

sys/dev/hwpmc/hwpmc_ibs.h

sys/dev/hwpmc/hwpmc_ibs.c

usr.sbin/pmcstat/pmcstat_log.c

hwpmc: Add Zen6 IBS ctl2 filters and alternate disable
Needs ReviewPublic
Actions

Revision Contents
Changeset List