Re: [PATCH V1] tools/power turbostat: Fix RAPL MSR address selection on AMD platforms

[Len Brown]

Thanks for noticing, reporting, and suggesting a fix!

Please let me know if this fix already in my tree does not address the
issue for you.

https://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git/commit/?h=turbostat&id=16cc8f249c702b7cbb4c2c2be7cd8f4fdd5d1d0c

-Len

On Thu, Jan 29, 2026 at 8:54 PM Qinyun Tan <qinyuntan@xxxxxxxxxxxxxxxxx> wrote:
>
> idx_to_offset() uses valid_rapl_msrs to determine which MSR address to
> return for RAPL counters (Intel's MSR_PKG_ENERGY_STATUS at 0x611 vs AMD's
> MSR_PKG_ENERGY_STAT at 0xc001029b).
>
> However, probe_rapl_msrs() calls idx_to_offset() before valid_rapl_msrs
> has been set - it's only assigned at the end of probe_rapl_msrs() after
> successful MSR validation. This causes idx_to_offset() to always return
> the Intel MSR address (0x611) during the probe phase, even on AMD systems.
>
> On Intel platforms this works by coincidence since the default (else
> branch) returns the correct Intel address. On AMD platforms, this causes
> turbostat to fail with:
>
>   turbostat: cpu0: msr offset 0x611 read failed: Input/output error
>
> Fix this by:
> 1. Falling back to platform->plat_rapl_msrs when valid_rapl_msrs is zero
>    (not yet validated). This ensures the correct platform-specific MSR
>    address is used during the initial probe.
> 2. Changing valid_rapl_msrs type from 'unsigned int' to 'int' to match
>    plat_rapl_msrs type and avoid sign comparison warnings.
>
> Fixes: 19476a592bf2 ("tools/power turbostat: Validate RAPL MSRs for AWS Nitro Hypervisor")
> Signed-off-by: Qinyun Tan <qinyuntan@xxxxxxxxxxxxxxxxx>
> ---
>  tools/power/x86/turbostat/turbostat.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 5ad45c2ac5bd..449492d5e043 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -492,7 +492,7 @@ unsigned int quiet;
>  unsigned int shown;
>  unsigned int sums_need_wide_columns;
>  unsigned int rapl_joules;
> -unsigned int valid_rapl_msrs;
> +int valid_rapl_msrs;
>  unsigned int summary_only;
>  unsigned int list_header_only;
>  unsigned int dump_only;
> @@ -2132,10 +2132,18 @@ struct msr_sum_array *per_cpu_msr_sum;
>  off_t idx_to_offset(int idx)
>  {
>         off_t offset;
> +       int rapl_msrs;
> +
> +       /*
> +        * Use valid_rapl_msrs if available (non-zero), otherwise fall back
> +        * to platform->plat_rapl_msrs. This allows probe_rapl_msrs() to call
> +        * this function before valid_rapl_msrs has been set.
> +        */
> +       rapl_msrs = valid_rapl_msrs ? valid_rapl_msrs : platform->plat_rapl_msrs;
>
>         switch (idx) {
>         case IDX_PKG_ENERGY:
> -               if (valid_rapl_msrs & RAPL_AMD_F17H)
> +               if (rapl_msrs & RAPL_AMD_F17H)
>                         offset = MSR_PKG_ENERGY_STAT;
>                 else
>                         offset = MSR_PKG_ENERGY_STATUS;
> --
> 2.43.5
>
>


-- 
Len Brown, Intel Open Source Technology Center