Page MenuHomeFreeBSD

Add Power8 and Power9 PMCs

Authored by luporl on Jul 8 2021, 12:40 PM.
Referenced Files
Unknown Object (File)
Thu, Feb 15, 3:57 PM
Unknown Object (File)
Thu, Jan 25, 3:53 PM
Unknown Object (File)
Thu, Jan 25, 3:53 PM
Unknown Object (File)
Thu, Jan 25, 3:52 PM
Unknown Object (File)
Jan 10 2024, 3:24 PM
Unknown Object (File)
Dec 24 2023, 5:16 PM
Unknown Object (File)
Dec 20 2023, 2:05 AM
Unknown Object (File)
Dec 12 2023, 2:31 AM



Added support to allocate Power8 and 9 PMCs.

Diff Detail

rG FreeBSD src repository
Lint Not Applicable
Tests Not Applicable

Event Timeline

Very nice to see this change already, I was hoping someone would pick this up for PPC.

A couple of minor comments, but this looks pretty good.


It seems that this function could be eliminated altogether, since there is no distinction in behaviour between power8 and power9.

To elaborate, the presence of a pmc_cpuid value matching the regex in pmu-events/arch/powerpc/mapfile.csv is enough to detect whether a CPU is supported or not. This is checked in pmu_events_map_get().


I would suggest splitting the removal of these events into a separate change.


Worth having a simple comment here, something like /* Set the value for kern.hwpmc.cpuid */.

Removed "pmu_events_mfr" function
Did not remove event codes in this review
Added space setting "pcd->pcd_allocate_pmc"
Added comment when setting "kern.hwpmc.cpuid"

@mhorne Thanks for the review!

Added a new version with requested changes.
I will send another one removing the event codes.


Did you mean to add this back? It doesn't appear necessary.


Yes, otherwise I would need to remove the enum events as they are not used anywhere.

Looks good. One final nitpick, but I won't block based on that alone.


0xFFFF is the maximum width of the event code? A named macro would make this more obvious, and perhaps it's worth returning EINVAL if pm_ev exceeds this value.


Got it, thanks :)

This revision is now accepted and ready to land.Jul 9 2021, 2:22 PM

Removing and operation as it is not necessary.

This revision now requires review to proceed.Jul 11 2021, 4:12 PM added inline comments.

I tested and the config can receive the entire code to allocate the counter.
I have extracted only the code event, removing the PMC flags, but it is not necessary as both ways work, so I removed the and operation.

I didn't add a maximum value check to return EINVAL because it is not a continuous number.
Do you think it is necessary to get the maximum value combination of PMC code and event code to add a check as you mentioned?

mhorne added inline comments.

If there is no clear upper bound then it is not necessary to add anything. It is only helpful as a defensive check when the width of the event code is defined to be, for example, 16 bits wide.

This revision is now accepted and ready to land.Jul 13 2021, 1:58 PM

Overall the changes look good.

But I tested them on a POWER9 VM and it seems there is something wrong.

Running this:

stress -c 1 -t 5 & pmcstat -O sample.out -S pm_inst_cmpl -l 5
pmcstat -R sample.out -G sample.graph

resulted in sample.graph listing pm_cmplu_stall_fxlong events, instead of pm_inst_cmpl.
pmcstat -R sample.out -g also reports pm_cmplu_stall_fxlong events, instead of pm_inst_cmpl ones.

So, it seems either the wrong event is being selected or there is something wrong with the event to name mapping part.
An interesting thing is that pmcstat -L lists pm_cmplu_stall_fxlong as the third event (event 2 if counting from 0) and the event value of pm_inst_cmpl is 2.

Trying the same command with another event resulted in a core dump:

stress -c 1 -t 5 & pmcstat -O sample.out -S pm_inst_from_rl4 -l 5
pmcstat -R sample.out -G sample.graph
Assertion failed: (pme->table[idx].name), function pmc_pmu_event_get_by_idx, file /usr/src/lib/libpmc/libpmc_pmu_util.c, line 251.
Abort trap (core dumped)

Analysing it, gdb says that idx is 0x2404a.

Note however that these tests were performed using incremental kernel/world builds and only reinstalling the kernel, its modules, pmc libraries and binaries (because llvm 12 is currently broken on PowerPC).


nitpick: long statements should be indented with 4 spaces.


nitpick: long statements should be indented with 4 spaces.

I have investigated the issue further and have some suggestions of how it can be fixed (inline).

These were tested on a VM and seem to work fine.


It seems pm_ev needs to point to idx for libpmc to map correctly between names and events.
The event code could be passed in a pm_md field, such as pm_md.pm_event.

This field should be added to sys/powerpc/include/pmc_mdep.h, like:

union pmc_md_op_pmcallocate {
-       uint64_t                __pad[4];
+       uint32_t                pm_event;
+       uint32_t                __pad1;
+       uint64_t                __pad2[3];

With pm_md.pm_event being passed from libpmc, config can be obtained from the lower half of it.
The upper half holds the counter number.

The counter value should be between 1 to 4 and it must match ri + 1, in order to select the correct PMC for the given config, unless counter is 0, then any PMC may be used.

We should also handle 2 special cases: PMC5 and PMC6, that are not programmable, but always count instructions and cycles, respectively.
This way, we save the other 4 programmable PMCs to count other events.

I've seen some event codes in libpmc event database with counter values greater than 4. I don't know how they should be programmed, but I guess it's safe to treat them as invalid for now.

The macros in the suggestion above are:

#define PM_EVENT_CODE(pe)      (pe & 0xffff)
#define PM_EVENT_COUNTER(pe)   ((pe >> 16) & 0xffff)

Thanks for narrowing this down, the use of idx vs ped.ped_event is not obvious at all in the amd64 code, and if you look you'll see that my arm64 version is accidentally incomplete :) I agree with the analysis.


One mistake I've just noted in D31221: pmc_md_op_pmcallocate is a union, not a struct. Therefore the padding shouldn't be touched.


Oh, that's true, I didn't realize it was a union, thanks for pointing it out.

luporl edited reviewers, added:; removed: luporl.

Commandeering revision to finish changes while Leonardo is out.

luporl marked an inline comment as done.
  • Fix event name mapping
  • Handle non-programmable PMCs (PMC5 and PMC6)
  • Add missing event counter check
This revision now requires review to proceed.Jul 21 2021, 6:46 PM

Looks good, thanks for finishing this. Will you handle the removal of the static power8 definitions as well?


No need to make this a struct.

This revision is now accepted and ready to land.Jul 23 2021, 1:48 PM

Looks good, thanks for finishing this. Will you handle the removal of the static power8 definitions as well?

Yes, I'll post a new revision with it soon.


Ok, I added it to hold future fields, but it can be added later too, when needed.
I'll change it before committing.

This revision was automatically updated to reflect the committed changes.