pmc(8): handle AMD Zen3-B0 IBS errata in the frontend view

[afscoelho_gmail.com]

Summary

AMD Family 19h Model 00h-0Fh (Zen3-B0) has IBS errata that make some
fetch and op sample fields wrong. Detect this silicon from the CPUID
block in the pmc(8) log header, set one flag per erratum, and correct
the affected samples before the callchain callbacks:

#1197: drop fetch samples with a zero linear address.
#1238: omit the IC Miss column; the IcMiss bit is unreliable.
#1293: zero DATA2 and the DATA3 L2Miss/OpenMemReqs fields on the
       trigger conditions, keeping the sample.
#1347: remap the L1TLB page-size labels for display.

Signed-off-by: Andre Silva <[e-mail]>

[mhorne]

I did not review in detail.

References to pmc(1) should actually be pmc(8), as the utility is in /usr/sbin.

The change summary should become:

pmc(8): handle AMD Zen3-B0 IBS errata in the frontend view

Because you are not touching the hwpmc module here.

usr.sbin/pmc/cmd_pmc_frontend.cc

88

Does it need the zen3_b0 parameter, or can it check ibs_zen3_b0_errata directly?

[ali_mashtizadeh.com]

Thanks Andre, like I mentioned in the call I mainly want to reduce the spread of the errata code and concentrate it right before callchain callbacks as much as possible. You may want to rebase this on top of the remaining pmc bits I said I'd push.

usr.sbin/pmc/cmd_pmc_frontend.cc

99

icmiss goes up here.

145

Please move the ICMISS above the L2MISS

158

You can simplify this and just drop printing it if the errata flag is present.

179

Move IC MISS above L2 MISS and hide the column if the errata is present.

201

I think '-' is confusing just don't add the column to processors with the errata.

usr.sbin/pmc/view.cc

65

Some of this info is defined in the main cpuid code in x86/x86/cpuident and its header in x86/include or something like that, but I'm not sure that we keep those headers on other architectures.

Otherwise move these into a header of it's own inside of this folder.

215

Looks good.

806

Unneeded you can access the errata flag directly by setting it in the view.

808

Don't do decoding here.

817

Move this down near where you process #1197 and just fixup the data, zero out those flags rather than marking them valid. Since this condition is rare, we can use the valid samples still.

826

Just zero out the invalid bits because this condition isn't super common, thus the numbers we get in the other cases is still useful even if it's not complete. We should eventually mark the data with some symbol to mean its inconsistent.

868

I think all the errata should processed here in the IBS paths below or inside the individual analysis passes.

884

Move the errata code for #1293 here.

usr.sbin/pmc/view.hh

314

Unneeded access the errata flag from the view.

316

Also unneeded.

328

Not needed if we just zero out the bad samples' flag.

404

Good all the errata that are relevant should have their own flags here.

[ali_mashtizadeh.com]

Remember that the code path is shared on all platforms I’m trying to make it that we can run analysis across 64-bit big-endian machines.

To summarize

1. Inside the cpuid parsing (x86 only) you should compute all the errata flags (which you did).
2. Correcting per-event rare events in the IBS data should be donee right before the call to call chain around lines 900-920 depending if it’s fetch or op events. (a) We should eventually add some annotation to tell the user the event is imprecise, but I’m not to worried about it unless we know the event occurs often.
3. Eliding unreliable data can occur inside the analysis pass.

[ali_mashtizadeh.com]

Can you please comment on https://reviews.freebsd.org/D58698 so the two of you can agree on an API for LBR? I can clean it up and we land it if your happy with it or you can take over.

[afscoelho_gmail.com]

Restructured per review: per-erratum flags, sample fixups before the callchain callbacks, display-only decisions in the analysis pass, CPUID macros in a local cpuid.hh.

[afscoelho_gmail.com]

In D58715#1350023, @ali_mashtizadeh.com wrote:

Can you please comment on https://reviews.freebsd.org/D58698 so the two of you can agree on an API for LBR? I can clean it up and we land it if your happy with it or you can take over.

About the branch history diff: my next task is AMD LBRv2, so I'd like to take D58698 over. Since there's no branch sampling backend upstream today, the generic hook would have no consumer yet. LBRv2 would be the first, so the two need to land together anyway. I'd take both the generic API and the AMD backend, keeping the hook vendor-neutral so Intel LBR and ARM BRBE can plug in later, but I'm not planning to write those backends myself. Is that OK with you?

usr.sbin/pmc/cmd_pmc_frontend.cc

88

It can't access it directly, the helper is static function and the flag is a protected member of pmcview. I kept the parameter (now it takes the 1347 flag) and pass it from print(), where the member is in scope.

[ali_mashtizadeh.com]

The diff looks good I'm only wondering if the x86 headers are available on other architectures so we don't need the cpuid header otherwise that's fine.

usr.sbin/pmc/cpuid.hh

37

Does this break our build to take this from the existing x86 headers?

[afscoelho_gmail.com]

usr.sbin/pmc/cpuid.hh

37

I checked this with an arm64 build. With my changes it completes a full buildworld, but adding either <machine/specialreg.h> or <x86/specialreg.h> fails when compiling pmc . So I think we still need the local definitions here to keep pmc buildable .

c++  -target aarch64-unknown-freebsd16.0 --sysroot=/tmp/d58715-arm64-evidence/obj/baseline/tmp/d58715-arm64-evidence/src/arm64.aarch64/tmp -B/tmp/d58715-arm64-evidence/obj/baseline/tmp/d58715-arm64-evidence/src/arm64.aarch64/tmp/usr/
  bin -fpie  -O2 -pipe -fno-common -I/tmp/d58715-arm64-evidence/src/lib/libpmcstat -fPIE -g -gz=zlib -MD -MF.depend.view.pieo -MTview.pieo -Wno-format-zero-length -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k
  -W -Wno-unused-parameter -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wshadow -Wunused-parameter -Wcast-align -Wchar-subscripts -Wdate-time -Wmissing-variable-declarations -Wthread-safety -Wno-empty-body -Wno-
  string-plus-int -Wno-unused-const-variable -Wno-error=unused-but-set-parameter -Wno-error=cast-function-type-mismatch -Qunused-arguments   -fPIE -g -gz=zlib -std=gnu++17   -Wno-c++20-extensions -Wno-c++23-lambda-attributes -Wno-
  nullability-completeness       -c /tmp/d58715-arm64-evidence/src/usr.sbin/pmc/view.cc -o view.pieo

  With <machine/specialreg.h>:

  In file included from /tmp/d58715-arm64-evidence/src/usr.sbin/pmc/view.cc:56:
  /tmp/d58715-arm64-evidence/src/usr.sbin/pmc/cpuid.hh:32:10: fatal error: 'machine/specialreg.h' file not found
     32 | #include <machine/specialreg.h>
        |          ^~~~~~~~~~~~~~~~~~~~~~

[ali_mashtizadeh.com]

Ah okay its the dev stuff that I guess we do have in the path.