In D58256#1336173, @ali_mashtizadeh.com wrote:

Maybe I'm missing something but it doesn't look like this feature is completely implemented.

Address review: use explicit 'ctrspec != NULL' in rapl_allocate_pmc (mhorne, style(9)).

Address review feedback: drop the unneeded (int) casts on pe in rapl_allocate_pmc, since the RAPL bounds are enum pmc_event members (mhorne); reword the microjoule-scaling sentence and describe wrap handling as a virtual 64-bit counter in pmc.rapl.3 (ali_mashtizadeh).

In D58028#1333646, @mhorne wrote:

LGTM. Ali, I will follow your lead that this is the most useful way to expose these counters.

Please also bump PMC_VERSION_MINOR in sys/sys/pmc.h.

Gate the RAPL class on a pmc_rapl_present() probe (AMDPM_RAPL CPUID bit on AMD/Hygon, unit-MSR rdmsr_safe on both vendors) instead of reserving a slot and dropping it on failure; move the guard tick prototype to the top of hwpmc_rapl.c; note RAPL in the amd64/i386 pmc_mdep.h class lists.

Superseded by an hwpmc-integrated implementation, per review feedback to integrate RAPL into hwpmc(4) rather than ship a standalone driver. The new work is a three-part stack:

In D55603#1326261, @ali_mashtizadeh.com wrote:

Just to follow up with our call can you quickly layout the plan for integrating with:

1. hwpmc
2. You don't need to implement it but are we going to use one driver for Intel/AMD? I'd like the hwpmc counters exposed in a unified way like Linux does.

Note that package power readings are strongly affected by CPU idle state.

Just a summary of this driver interface.

Abandoning this duplicate review. The driver update has been moved to the original continued review D55603, which now sits on top of D57758 and D57759.

Correcting the stack: this driver update continues D55603 and depends on the prerequisite AMD RAPL MSR definitions in D57759.

Correct stack onto D55603 and add Co-authored-by trailer for ShengYi Hung.

Correct stack onto D55603 and add Co-authored-by trailer for ShengYi Hung.

Correct stack onto D55603 and add Co-authored-by trailer for ShengYi Hung.

Hi @aokblast, just a heads-up that I've taken ownership of this revision to carry it forward. Thanks for laying the groundwork with the initial driver; I'll credit you with a Co-authored-by: trailer on the commit. I'll be pushing an updated diff shortly. Feel free to stay subscribed, your feedback is welcome!

Hi @aokblast, thanks for starting amdrapl!. I've been working on top of this driver and would like to carry the review forward to land. Would you be willing to reassign this revision to me, or add me so I can arc update it? Happy to keep you as co-author on the commit (I've added a Co-authored-by: trailer crediting you). I did work that covers on-demand energy sampling and socket-aware package mapping for example

@alfredo

@ali_mashtizadeh.com

Hi @aokblast, I've found your rapl driver for amd and I'm interested in continuing the work. It's been sitting since March and I'd like to pick it up. Are you still planning to work on it, or would you be OK with me taking over? Thanks

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

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.

Thanks for the review, Ali.

Address ali_mashtizadeh.com review feedback

Address ali_mashtizadeh.com review feedback

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

Address mhorne review feedback

@mashtizadeh_gmail.com

Add mpr driver into GENERIC64LE configuration.

Fixes after review. Make mps_read_extended_config_page return IOCStatus already byte-swapped. Other missing swaps fixed. Updated man page with TODO about flash operations not working on BE.

Any comment on this?

Someone any additional comment on this?

Review fixes.

Review comments fixes.

Comments and indentation.

In D23401#554073, @arichardson wrote:

This commit appears to have broken using QEMU MALTA (MIPS64 BE) with -device virtio-rng-pci.
With this change I get an infinite loop on boot that goes away if I revert it:

In D23990#533631, @jhibbits wrote:

What do you plan to do to continue this work?

Print information message about number of bpvo pool entries and memory used by this pool when booting in verbose mode.

Style (indentation, comparison) fixes.

Adjust moea64_bpvo_pool_size value only if hw_direct_map is false, otherwise set it to previous hard-coded value.

Removing blank line.

Removing extra white spaces.

Thanks @bryanv for the review.

Use the vtpci_write_config in vtpci_write_header macros for consistency. Typo fixes.

Use functions htole16 and htole32 when writing VirtIO header instead of le16toh and le32toh for better comprehension.

In D23401#513986, @alfredo.junior_eldorado.org.br wrote:

nice job @afscoelho_gmail.com !
I just tried your patch in a image uploaded to a POWER8 instance at https://openpower.ic.unicamp.br/minicloud/ and it worked out of the box.
Please check my comment on the diff.

In D23401#513326, @bdragon wrote:

Ohhhhhh, so THAT is why this has been such a headache?

This is an awesome find.