[v1] tools/turbostat: fix microcode patch level reading for AMD/Hygon

[PATCH] tools/turbostat: fix microcode patch level reading for AMD/Hygon

Posted by Serhii Pievniev 1 month, 1 week ago

turbostat always used the same logic to read the microcode patch level,
which is correct for Intel but not for AMD/Hygon.
While Intel stores the patch level in the upper 32 bits of MSR, AMD
stores it in the lower 32 bits, which previously caused turbostat to
report the microcode version as 0x0 on AMD/Hygon.

Split the logic into two paths, using upper bits of MSR_IA32_UCODE_REV
for Intel and lower bits of MSR_AMD64_PATCH_LEVEL for AMD/Hygon.
Although both MSRs share the same address (0x8b), separate constants
make this semantic difference explicit.

Signed-off-by: Serhii Pievniev <spevnev16@gmail.com>
---
 tools/power/x86/turbostat/turbostat.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 1a2671c2820..2698ac89376 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -9122,10 +9122,21 @@ void process_cpuid()
 	cpuid_has_hv = ecx_flags & (1 << 31);
 
 	if (!no_msr) {
-		if (get_msr(sched_getcpu(), MSR_IA32_UCODE_REV, &ucode_patch))
-			warnx("get_msr(UCODE)");
-		else
-			ucode_patch_valid = true;
+		if (authentic_amd || hygon_genuine) {
+			if (get_msr(sched_getcpu(), MSR_AMD64_PATCH_LEVEL, &ucode_patch)) {
+				warnx("get_msr(UCODE)");
+			} else {
+				ucode_patch_valid = true;
+				ucode_patch &= 0xFFFFFFFF;
+			}
+		} else {
+			if (get_msr(sched_getcpu(), MSR_IA32_UCODE_REV, &ucode_patch)) {
+				warnx("get_msr(UCODE)");
+			} else {
+				ucode_patch_valid = true;
+				ucode_patch = (ucode_patch >> 32) & 0xFFFFFFFF;
+			}
+		}
 	}
 
 	/*
@@ -9139,7 +9150,7 @@ void process_cpuid()
 	if (!quiet) {
 		fprintf(outf, "CPUID(1): family:model:stepping 0x%x:%x:%x (%d:%d:%d)", family, model, stepping, family, model, stepping);
 		if (ucode_patch_valid)
-			fprintf(outf, " microcode 0x%x", (unsigned int)((ucode_patch >> 32) & 0xFFFFFFFF));
+			fprintf(outf, " microcode 0x%llx", ucode_patch);
 		fputc('\n', outf);
 
 		fprintf(outf, "CPUID(0x80000000): max_extended_levels: 0x%x\n", max_extended_level);
-- 
2.53.0

Re: [PATCH] tools/turbostat: fix microcode patch level reading for AMD/Hygon

Posted by K Prateek Nayak 1 month, 1 week ago

On 2/24/2026 8:07 AM, Serhii Pievniev wrote:
> @@ -9139,7 +9150,7 @@ void process_cpuid()
>  	if (!quiet) {
>  		fprintf(outf, "CPUID(1): family:model:stepping 0x%x:%x:%x (%d:%d:%d)", family, model, stepping, family, model, stepping);
>  		if (ucode_patch_valid)
> -			fprintf(outf, " microcode 0x%x", (unsigned int)((ucode_patch >> 32) & 0xFFFFFFFF));

Since "MSR_IA32_UCODE_REV" and "MSR_AMD64_PATCH_LEVEL" are the same,
all that essentially changes is the shift.

Can't we just 0 the shift for "authentic_amd || hygon_genuine"?

> +			fprintf(outf, " microcode 0x%llx", ucode_patch);
>  		fputc('\n', outf);
>  
>  		fprintf(outf, "CPUID(0x80000000): max_extended_levels: 0x%x\n", max_extended_level);

-- 
Thanks and Regards,
Prateek

Re: [PATCH] tools/turbostat: fix microcode patch level reading for AMD/Hygon

Posted by Serhii 1 month, 1 week ago

Yes, the only essential change is shift.
However, I believe that handling the two paths separately is better
because (a) using different constants better highlights the semantic
difference of the values stored and (b) AMD currently reserves upper
bits, which could require handling in the future making separate paths
more appropriate.
What do you think, is it still better to merge back into a single path
with conditional shift for Intel?

On Mon, Feb 23, 2026 at 10:30 PM K Prateek Nayak <kprateek.nayak@amd.com> wrote:
>
> On 2/24/2026 8:07 AM, Serhii Pievniev wrote:
> > @@ -9139,7 +9150,7 @@ void process_cpuid()
> >       if (!quiet) {
> >               fprintf(outf, "CPUID(1): family:model:stepping 0x%x:%x:%x (%d:%d:%d)", family, model, stepping, family, model, stepping);
> >               if (ucode_patch_valid)
> > -                     fprintf(outf, " microcode 0x%x", (unsigned int)((ucode_patch >> 32) & 0xFFFFFFFF));
>
> Since "MSR_IA32_UCODE_REV" and "MSR_AMD64_PATCH_LEVEL" are the same,
> all that essentially changes is the shift.
>
> Can't we just 0 the shift for "authentic_amd || hygon_genuine"?
>
> > +                     fprintf(outf, " microcode 0x%llx", ucode_patch);
> >               fputc('\n', outf);
> >
> >               fprintf(outf, "CPUID(0x80000000): max_extended_levels: 0x%x\n", max_extended_level);
>
> --
> Thanks and Regards,
> Prateek
>

Re: [PATCH] tools/turbostat: fix microcode patch level reading for AMD/Hygon

Posted by K Prateek Nayak 1 month, 1 week ago

Hello Serhii,

On 2/24/2026 7:40 PM, Serhii wrote:
> Yes, the only essential change is shift.
> However, I believe that handling the two paths separately is better
> because (a) using different constants better highlights the semantic
> difference of the values stored and (b) AMD currently reserves upper
> bits, which could require handling in the future making separate paths
> more appropriate.
> What do you think, is it still better to merge back into a single path
> with conditional shift for Intel?

I'll let Len answer which one he prefers but having 2 paths is more
code duplication for error handling when we are essentially doing the
same thing.

-- 
Thanks and Regards,
Prateek

Re: [PATCH] tools/turbostat: fix microcode patch level reading for AMD/Hygon

Posted by Len Brown 1 month, 1 week ago

Thanks for noticing and reporting this issue!

Since the MSR is common, my vote is to keep a single MSR read and
setting of ucode_patch_valid for all vendors,
and just check the vendor to tweak the "ucode_patch" variable for printing.

On Tue, Feb 24, 2026 at 10:35 PM K Prateek Nayak <kprateek.nayak@amd.com> wrote:
>
> Hello Serhii,
>
> On 2/24/2026 7:40 PM, Serhii wrote:
> > Yes, the only essential change is shift.
> > However, I believe that handling the two paths separately is better
> > because (a) using different constants better highlights the semantic
> > difference of the values stored and (b) AMD currently reserves upper
> > bits, which could require handling in the future making separate paths
> > more appropriate.
> > What do you think, is it still better to merge back into a single path
> > with conditional shift for Intel?
>
> I'll let Len answer which one he prefers but having 2 paths is more
> code duplication for error handling when we are essentially doing the
> same thing.
>
> --
> Thanks and Regards,
> Prateek
>


-- 
Len Brown, Intel Open Source Technology Center

[PATCH v2] tools/turbostat: fix microcode patch level reading for AMD/Hygon

Posted by Serhii Pievniev 1 month, 1 week ago

turbostat always used the same logic to read the microcode patch level,
which is correct for Intel but not for AMD/Hygon.
While Intel stores the patch level in the upper 32 bits of MSR, AMD
stores it in the lower 32 bits, which causes turbostat to report the
microcode version as 0x0 on AMD/Hygon.

Fix by shifting right by 32 for non-AMD/Hygon, preserving the existing
behavior for Intel and unknown vendors.

Signed-off-by: Serhii Pievniev <spevnev16@gmail.com>
---
v1 -> v2: Changed to single MSR path with conditional shift

v1: https://lore.kernel.org/linux-pm/20260224023719.65165-1-spevnev16@gmail.com
---
 tools/power/x86/turbostat/turbostat.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 1a2671c2820..7545142b3a6 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -9122,10 +9122,13 @@ void process_cpuid()
 	cpuid_has_hv = ecx_flags & (1 << 31);
 
 	if (!no_msr) {
-		if (get_msr(sched_getcpu(), MSR_IA32_UCODE_REV, &ucode_patch))
+		if (get_msr(sched_getcpu(), MSR_IA32_UCODE_REV, &ucode_patch)) {
 			warnx("get_msr(UCODE)");
-		else
+		} else {
 			ucode_patch_valid = true;
+			if (!authentic_amd && !hygon_genuine)
+				ucode_patch >>= 32;
+		}
 	}
 
 	/*
@@ -9139,7 +9142,7 @@ void process_cpuid()
 	if (!quiet) {
 		fprintf(outf, "CPUID(1): family:model:stepping 0x%x:%x:%x (%d:%d:%d)", family, model, stepping, family, model, stepping);
 		if (ucode_patch_valid)
-			fprintf(outf, " microcode 0x%x", (unsigned int)((ucode_patch >> 32) & 0xFFFFFFFF));
+			fprintf(outf, " microcode 0x%x", (unsigned int)ucode_patch);
 		fputc('\n', outf);
 
 		fprintf(outf, "CPUID(0x80000000): max_extended_levels: 0x%x\n", max_extended_level);
-- 
2.53.0

Re: [PATCH v2] tools/turbostat: fix microcode patch level reading for AMD/Hygon

Posted by Len Brown 3 weeks, 4 days ago

Applied, thanks!

ps. I added a Fixes tag...
Fixes: 3e4048466c39 ("tools/power turbostat: Add --no-msr option")

That is in Linux-6.9, where this cleanly applies.  Technically the logic
was already broken before that in an earlier 6.9 patch, and the original code
was broken years before that, but I expect going back 2-years is more
than sufficient.

On Wed, Feb 25, 2026 at 6:16 PM Serhii Pievniev <spevnev16@gmail.com> wrote:
>
> turbostat always used the same logic to read the microcode patch level,
> which is correct for Intel but not for AMD/Hygon.
> While Intel stores the patch level in the upper 32 bits of MSR, AMD
> stores it in the lower 32 bits, which causes turbostat to report the
> microcode version as 0x0 on AMD/Hygon.
>
> Fix by shifting right by 32 for non-AMD/Hygon, preserving the existing
> behavior for Intel and unknown vendors.
>
> Signed-off-by: Serhii Pievniev <spevnev16@gmail.com>
> ---
> v1 -> v2: Changed to single MSR path with conditional shift
>
> v1: https://lore.kernel.org/linux-pm/20260224023719.65165-1-spevnev16@gmail.com
> ---
>  tools/power/x86/turbostat/turbostat.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 1a2671c2820..7545142b3a6 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -9122,10 +9122,13 @@ void process_cpuid()
>         cpuid_has_hv = ecx_flags & (1 << 31);
>
>         if (!no_msr) {
> -               if (get_msr(sched_getcpu(), MSR_IA32_UCODE_REV, &ucode_patch))
> +               if (get_msr(sched_getcpu(), MSR_IA32_UCODE_REV, &ucode_patch)) {
>                         warnx("get_msr(UCODE)");
> -               else
> +               } else {
>                         ucode_patch_valid = true;
> +                       if (!authentic_amd && !hygon_genuine)
> +                               ucode_patch >>= 32;
> +               }
>         }
>
>         /*
> @@ -9139,7 +9142,7 @@ void process_cpuid()
>         if (!quiet) {
>                 fprintf(outf, "CPUID(1): family:model:stepping 0x%x:%x:%x (%d:%d:%d)", family, model, stepping, family, model, stepping);
>                 if (ucode_patch_valid)
> -                       fprintf(outf, " microcode 0x%x", (unsigned int)((ucode_patch >> 32) & 0xFFFFFFFF));
> +                       fprintf(outf, " microcode 0x%x", (unsigned int)ucode_patch);
>                 fputc('\n', outf);
>
>                 fprintf(outf, "CPUID(0x80000000): max_extended_levels: 0x%x\n", max_extended_level);
> --
> 2.53.0
>


-- 
Len Brown, Intel Open Source Technology Center