Re: [PATCH 16/32] cpufreq: Stop using 32-bit MSR interfaces

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 18896 bytes --]

On 29.06.26 16:55, Zhongqiu Han wrote:
> On 6/29/2026 2:05 PM, Juergen Gross wrote:
>> The 32-bit MSR interfaces rdmsr() and wrmsr() are planned to be
>> removed. Use the related 64-bit variants instead.
> 
> Hello Juergen,
> 
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   drivers/cpufreq/acpi-cpufreq.c       | 22 ++++----
>>   drivers/cpufreq/e_powersaver.c       | 48 ++++++++---------
>>   drivers/cpufreq/longhaul.c           | 15 +++---
>>   drivers/cpufreq/longrun.c            | 78 +++++++++++++++-------------
>>   drivers/cpufreq/powernow-k6.c        | 12 ++---
>>   drivers/cpufreq/powernow-k8.c        | 67 ++++++++++++------------
>>   drivers/cpufreq/speedstep-centrino.c | 16 +++---
>>   drivers/cpufreq/speedstep-lib.c      | 63 +++++++++++-----------
>>   8 files changed, 166 insertions(+), 155 deletions(-)
>>
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 21639d9ac753..b40fa99b5ab2 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -246,32 +246,32 @@ static unsigned extract_freq(struct cpufreq_policy 
>> *policy, u32 val)
>>   static u32 cpu_freq_read_intel(struct acpi_pct_register *not_used)
>>   {
>> -    u32 val, dummy __always_unused;
>> +    u64 val;
>> -    rdmsr(MSR_IA32_PERF_CTL, val, dummy);
>> -    return val;
>> +    rdmsrq(MSR_IA32_PERF_CTL, val);
>> +    return (u32)val;
>>   }
>>   static void cpu_freq_write_intel(struct acpi_pct_register *not_used, u32 val)
>>   {
>> -    u32 lo, hi;
>> +    struct msr msrval;
>> -    rdmsr(MSR_IA32_PERF_CTL, lo, hi);
>> -    lo = (lo & ~INTEL_MSR_RANGE) | (val & INTEL_MSR_RANGE);
>> -    wrmsr(MSR_IA32_PERF_CTL, lo, hi);
>> +    rdmsrq(MSR_IA32_PERF_CTL, msrval.q);
>> +    msrval.h = (msrval.h & ~INTEL_MSR_RANGE) | (val & INTEL_MSR_RANGE);
> 
> msrval.l

Oh, indeed.

> 
> please feel free to correct me if there is any misunderstanding.
> 
> how about using u64 directly?
> 
>      u64 msrval;
>      rdmsrq(MSR_IA32_PERF_CTL, msrval);
>      msrval = (msrval & ~(u64)INTEL_MSR_RANGE) | (val & INTEL_MSR_RANGE);
>      wrmsrq(MSR_IA32_PERF_CTL, msrval);
> 
> As per my understanding, we can use u64 instead of struct msr in many
> other places in this patch as well.

I didn't look at all the MSR definitions to find out whether a plain u64 would
work.

Will look into this in more detail.

> 
>> +    wrmsrq(MSR_IA32_PERF_CTL, msrval.q);
>>   }
>>   static u32 cpu_freq_read_amd(struct acpi_pct_register *not_used)
>>   {
>> -    u32 val, dummy __always_unused;
>> +    u64 val;
>> -    rdmsr(MSR_AMD_PERF_CTL, val, dummy);
>> -    return val;
>> +    rdmsrq(MSR_AMD_PERF_CTL, val);
>> +    return (u32)val;
>>   }
>>   static void cpu_freq_write_amd(struct acpi_pct_register *not_used, u32 val)
>>   {
>> -    wrmsr(MSR_AMD_PERF_CTL, val, 0);
>> +    wrmsrq(MSR_AMD_PERF_CTL, val);
>>   }
>>   static u32 cpu_freq_read_io(struct acpi_pct_register *reg)
>> diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
>> index eb5a9209d828..13709f9667ea 100644
>> --- a/drivers/cpufreq/e_powersaver.c
>> +++ b/drivers/cpufreq/e_powersaver.c
>> @@ -90,7 +90,7 @@ static int eps_acpi_exit(struct cpufreq_policy *policy)
>>   static unsigned int eps_get(unsigned int cpu)
>>   {
>>       struct eps_cpu_data *centaur;
>> -    u32 lo, hi;
>> +    u64 val;
>>       if (cpu)
>>           return 0;
>> @@ -99,35 +99,35 @@ static unsigned int eps_get(unsigned int cpu)
>>           return 0;
>>       /* Return current frequency */
>> -    rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
>> -    return centaur->fsb * ((lo >> 8) & 0xff);
>> +    rdmsrq(MSR_IA32_PERF_STATUS, val);
>> +    return centaur->fsb * ((val >> 8) & 0xff);
>>   }
>>   static int eps_set_state(struct eps_cpu_data *centaur,
>>                struct cpufreq_policy *policy,
>>                u32 dest_state)
>>   {
>> -    u32 lo, hi;
>> +    u64 val;
>>       int i;
>>       /* Wait while CPU is busy */
>> -    rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
>> +    rdmsrq(MSR_IA32_PERF_STATUS, val);
>>       i = 0;
>> -    while (lo & ((1 << 16) | (1 << 17))) {
>> +    while (val & ((1 << 16) | (1 << 17))) {
>>           udelay(16);
>> -        rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
>> +        rdmsrq(MSR_IA32_PERF_STATUS, val);
>>           i++;
>>           if (unlikely(i > 64)) {
>>               return -ENODEV;
>>           }
>>       }
>>       /* Set new multiplier and voltage */
>> -    wrmsr(MSR_IA32_PERF_CTL, dest_state & 0xffff, 0);
>> +    wrmsrq(MSR_IA32_PERF_CTL, dest_state & 0xffff);
>>       /* Wait until transition end */
>>       i = 0;
>>       do {
>>           udelay(16);
>> -        rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
>> +        rdmsrq(MSR_IA32_PERF_STATUS, val);
>>           i++;
>>           if (unlikely(i > 64)) {
>>               return -ENODEV;
> 
> 
>      do {
>          udelay(16);
>          rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
>          i++;
>          if (unlikely(i > 64)) {
>              return -ENODEV;
>          }
>      } while (lo & ((1 << 16) | (1 << 17)));-->lo has been deleted?

Thanks for spotting that. Seems by 32-bit allyesconfig tree lost the config
entry needed for building this source.

Will fix this.

> 
> 
> 
> 
> 
>> @@ -139,10 +139,10 @@ static int eps_set_state(struct eps_cpu_data *centaur,
>>       u8 current_multiplier, current_voltage;
>>       /* Print voltage and multiplier */
>> -    rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
>> -    current_voltage = lo & 0xff;
>> +    rdmsrq(MSR_IA32_PERF_STATUS, val);
>> +    current_voltage = val & 0xff;
>>       pr_info("Current voltage = %dmV\n", current_voltage * 16 + 700);
>> -    current_multiplier = (lo >> 8) & 0xff;
>> +    current_multiplier = (val >> 8) & 0xff;
>>       pr_info("Current multiplier = %d\n", current_multiplier);
>>       }
>>   #endif
>> @@ -171,8 +171,8 @@ static int eps_target(struct cpufreq_policy *policy, 
>> unsigned int index)
>>   static int eps_cpu_init(struct cpufreq_policy *policy)
>>   {
>>       unsigned int i;
>> -    u32 lo, hi;
>>       u64 val;
>> +    struct msr status;
>>       u8 current_multiplier, current_voltage;
>>       u8 max_multiplier, max_voltage;
>>       u8 min_multiplier, min_voltage;
>> @@ -195,13 +195,13 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
>>       switch (c->x86_model) {
>>       case 10:
>> -        rdmsr(0x1153, lo, hi);
>> -        brand = (((lo >> 2) ^ lo) >> 18) & 3;
>> +        rdmsrq(0x1153, val);
>> +        brand = (((val >> 2) ^ val) >> 18) & 3;
>>           pr_cont("Model A ");
>>           break;
>>       case 13:
>> -        rdmsr(0x1154, lo, hi);
>> -        brand = (((lo >> 4) ^ (lo >> 2))) & 0x000000ff;
>> +        rdmsrq(0x1154, val);
>> +        brand = (((val >> 4) ^ (val >> 2))) & 0x000000ff;
>>           pr_cont("Model D ");
>>           break;
>>       }
>> @@ -237,20 +237,20 @@ static int eps_cpu_init(struct cpufreq_policy *policy)
>>       }
>>       /* Print voltage and multiplier */
>> -    rdmsr(MSR_IA32_PERF_STATUS, lo, hi);
>> -    current_voltage = lo & 0xff;
>> +    rdmsrq(MSR_IA32_PERF_STATUS, status.q);
>> +    current_voltage = status.l & 0xff;
>>       pr_info("Current voltage = %dmV\n", current_voltage * 16 + 700);
>> -    current_multiplier = (lo >> 8) & 0xff;
>> +    current_multiplier = (status.l >> 8) & 0xff;
>>       pr_info("Current multiplier = %d\n", current_multiplier);
>>       /* Print limits */
>> -    max_voltage = hi & 0xff;
>> +    max_voltage = status.h & 0xff;
>>       pr_info("Highest voltage = %dmV\n", max_voltage * 16 + 700);
>> -    max_multiplier = (hi >> 8) & 0xff;
>> +    max_multiplier = (status.h >> 8) & 0xff;
>>       pr_info("Highest multiplier = %d\n", max_multiplier);
>> -    min_voltage = (hi >> 16) & 0xff;
>> +    min_voltage = (status.h >> 16) & 0xff;
>>       pr_info("Lowest voltage = %dmV\n", min_voltage * 16 + 700);
>> -    min_multiplier = (hi >> 24) & 0xff;
>> +    min_multiplier = (status.h >> 24) & 0xff;
>>       pr_info("Lowest multiplier = %d\n", min_multiplier);
>>       /* Sanity checks */
>> diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
>> index a18d1d11725f..4c2599264333 100644
>> --- a/drivers/cpufreq/longhaul.c
>> +++ b/drivers/cpufreq/longhaul.c
>> @@ -118,13 +118,14 @@ static unsigned int calc_speed(int mult)
>>   static int longhaul_get_cpu_mult(void)
>>   {
>> -    unsigned long invalue = 0, lo, hi;
>> +    unsigned long invalue = 0;
>> +    u64 val;
>> -    rdmsr(MSR_IA32_EBL_CR_POWERON, lo, hi);
>> -    invalue = (lo & (1<<22|1<<23|1<<24|1<<25))>>22;
>> +    rdmsrq(MSR_IA32_EBL_CR_POWERON, val);
>> +    invalue = (val & (1<<22|1<<23|1<<24|1<<25))>>22;
>>       if (longhaul_version == TYPE_LONGHAUL_V2 ||
>>           longhaul_version == TYPE_POWERSAVER) {
>> -        if (lo & (1<<27))
>> +        if (val & (1<<27))
>>               invalue += 16;
>>       }
>>       return eblcr[invalue];
>> @@ -761,7 +762,7 @@ static int longhaul_cpu_init(struct cpufreq_policy *policy)
>>       struct cpuinfo_x86 *c = &cpu_data(0);
>>       char *cpuname = NULL;
>>       int ret;
>> -    u32 lo, hi;
>> +    u64 val;
>>       /* Check what we have on this motherboard */
>>       switch (c->x86_model) {
>> @@ -835,8 +836,8 @@ static int longhaul_cpu_init(struct cpufreq_policy *policy)
>>       }
>>       /* Check Longhaul ver. 2 */
>>       if (longhaul_version == TYPE_LONGHAUL_V2) {
>> -        rdmsr(MSR_VIA_LONGHAUL, lo, hi);
>> -        if (lo == 0 && hi == 0)
>> +        rdmsrq(MSR_VIA_LONGHAUL, val);
>> +        if (val == 0)
>>               /* Looks like MSR isn't present */
>>               longhaul_version = TYPE_LONGHAUL_V1;
>>       }
>> diff --git a/drivers/cpufreq/longrun.c b/drivers/cpufreq/longrun.c
>> index f3aaca0496a4..99abef32e7e5 100644
>> --- a/drivers/cpufreq/longrun.c
>> +++ b/drivers/cpufreq/longrun.c
>> @@ -35,27 +35,27 @@ static unsigned int longrun_low_freq, longrun_high_freq;
>>    */
>>   static void longrun_get_policy(struct cpufreq_policy *policy)
>>   {
>> -    u32 msr_lo, msr_hi;
>> +    struct msr msr;
>> -    rdmsr(MSR_TMTA_LONGRUN_FLAGS, msr_lo, msr_hi);
>> -    pr_debug("longrun flags are %x - %x\n", msr_lo, msr_hi);
>> -    if (msr_lo & 0x01)
>> +    rdmsrq(MSR_TMTA_LONGRUN_FLAGS, msr.q);
>> +    pr_debug("longrun flags are %x - %x\n", msr.l, msr.h);
>> +    if (msr.l & 0x01)
>>           policy->policy = CPUFREQ_POLICY_PERFORMANCE;
>>       else
>>           policy->policy = CPUFREQ_POLICY_POWERSAVE;
>> -    rdmsr(MSR_TMTA_LONGRUN_CTRL, msr_lo, msr_hi);
>> -    pr_debug("longrun ctrl is %x - %x\n", msr_lo, msr_hi);
>> -    msr_lo &= 0x0000007F;
>> -    msr_hi &= 0x0000007F;
>> +    rdmsrq(MSR_TMTA_LONGRUN_CTRL, msr.q);
>> +    pr_debug("longrun ctrl is %x - %x\n", msr.l, msr.h);
>> +    msr.l &= 0x0000007F;
>> +    msr.h &= 0x0000007F;
>>       if (longrun_high_freq <= longrun_low_freq) {
>>           /* Assume degenerate Longrun table */
>>           policy->min = policy->max = longrun_high_freq;
>>       } else {
>> -        policy->min = longrun_low_freq + msr_lo *
>> +        policy->min = longrun_low_freq + msr.l *
>>               ((longrun_high_freq - longrun_low_freq) / 100);
>> -        policy->max = longrun_low_freq + msr_hi *
>> +        policy->max = longrun_low_freq + msr.h *
>>               ((longrun_high_freq - longrun_low_freq) / 100);
>>       }
>>       policy->cpu = 0;
>> @@ -71,7 +71,7 @@ static void longrun_get_policy(struct cpufreq_policy *policy)
>>    */
>>   static int longrun_set_policy(struct cpufreq_policy *policy)
>>   {
>> -    u32 msr_lo, msr_hi;
>> +    struct msr msr;
>>       u32 pctg_lo, pctg_hi;
>>       if (!policy)
>> @@ -93,24 +93,24 @@ static int longrun_set_policy(struct cpufreq_policy *policy)
>>           pctg_lo = pctg_hi;
>>       /* performance or economy mode */
>> -    rdmsr(MSR_TMTA_LONGRUN_FLAGS, msr_lo, msr_hi);
>> -    msr_lo &= 0xFFFFFFFE;
>> +    rdmsrq(MSR_TMTA_LONGRUN_FLAGS, msr.q);
>> +    msr.l &= 0xFFFFFFFE;
>>       switch (policy->policy) {
>>       case CPUFREQ_POLICY_PERFORMANCE:
>> -        msr_lo |= 0x00000001;
>> +        msr.l |= 0x00000001;
>>           break;
>>       case CPUFREQ_POLICY_POWERSAVE:
>>           break;
>>       }
>> -    wrmsr(MSR_TMTA_LONGRUN_FLAGS, msr_lo, msr_hi);
>> +    wrmsrq(MSR_TMTA_LONGRUN_FLAGS, msr.q);
>>       /* lower and upper boundary */
>> -    rdmsr(MSR_TMTA_LONGRUN_CTRL, msr_lo, msr_hi);
>> -    msr_lo &= 0xFFFFFF80;
>> -    msr_hi &= 0xFFFFFF80;
>> -    msr_lo |= pctg_lo;
>> -    msr_hi |= pctg_hi;
>> -    wrmsr(MSR_TMTA_LONGRUN_CTRL, msr_lo, msr_hi);
>> +    rdmsrq(MSR_TMTA_LONGRUN_CTRL, msr.q);
>> +    msr.l &= 0xFFFFFF80;
>> +    msr.h &= 0xFFFFFF80;
>> +    msr.l |= pctg_lo;
>> +    msr.h |= pctg_hi;
>> +    wrmsrq(MSR_TMTA_LONGRUN_CTRL, msr.q);
>>       return 0;
>>   }
>> @@ -160,7 +160,7 @@ static unsigned int longrun_get(unsigned int cpu)
>>   static int longrun_determine_freqs(unsigned int *low_freq,
>>                                 unsigned int *high_freq)
>>   {
>> -    u32 msr_lo, msr_hi;
>> +    struct msr msr;
>>       u32 save_lo, save_hi;
>>       u32 eax, ebx, ecx, edx;
>>       u32 try_hi;
>> @@ -178,15 +178,17 @@ static int longrun_determine_freqs(unsigned int *low_freq,
>>            * For maximum frequency, read out level zero.
>>            */
>>           /* minimum */
>> -        rdmsr(MSR_TMTA_LRTI_READOUT, msr_lo, msr_hi);
>> -        wrmsr(MSR_TMTA_LRTI_READOUT, msr_hi, msr_hi);
>> -        rdmsr(MSR_TMTA_LRTI_VOLT_MHZ, msr_lo, msr_hi);
>> -        *low_freq = msr_lo * 1000; /* to kHz */
>> +        rdmsrq(MSR_TMTA_LRTI_READOUT, msr.q);
>> +        msr.l = msr.h;
>> +        wrmsrq(MSR_TMTA_LRTI_READOUT, msr.q);
>> +        rdmsrq(MSR_TMTA_LRTI_VOLT_MHZ, msr.q);
>> +        *low_freq = msr.l * 1000; /* to kHz */
>>           /* maximum */
>> -        wrmsr(MSR_TMTA_LRTI_READOUT, 0, msr_hi);
>> -        rdmsr(MSR_TMTA_LRTI_VOLT_MHZ, msr_lo, msr_hi);
>> -        *high_freq = msr_lo * 1000; /* to kHz */
>> +        msr.l = 0;
>> +        wrmsrq(MSR_TMTA_LRTI_READOUT, msr.q);
>> +        rdmsrq(MSR_TMTA_LRTI_VOLT_MHZ, msr.q);
>> +        *high_freq = msr.l * 1000; /* to kHz */
>>           pr_debug("longrun table interface told %u - %u kHz\n",
>>                   *low_freq, *high_freq);
>> @@ -202,9 +204,9 @@ static int longrun_determine_freqs(unsigned int *low_freq,
>>       pr_debug("high frequency is %u kHz\n", *high_freq);
>>       /* get current borders */
>> -    rdmsr(MSR_TMTA_LONGRUN_CTRL, msr_lo, msr_hi);
>> -    save_lo = msr_lo & 0x0000007F;
>> -    save_hi = msr_hi & 0x0000007F;
>> +    rdmsrq(MSR_TMTA_LONGRUN_CTRL, msr.q);
>> +    save_lo = msr.l & 0x0000007F;
>> +    save_hi = msr.h & 0x0000007F;
>>       /* if current perf_pctg is larger than 90%, we need to decrease the
>>        * upper limit to make the calculation more accurate.
>> @@ -214,16 +216,18 @@ static int longrun_determine_freqs(unsigned int *low_freq,
>>        * on some barrier values */
>>       for (try_hi = 80; try_hi > 0 && ecx > 90; try_hi -= 10) {
>>           /* set to 0 to try_hi perf_pctg */
>> -        msr_lo &= 0xFFFFFF80;
>> -        msr_hi &= 0xFFFFFF80;
>> -        msr_hi |= try_hi;
>> -        wrmsr(MSR_TMTA_LONGRUN_CTRL, msr_lo, msr_hi);
>> +        msr.l &= 0xFFFFFF80;
>> +        msr.h &= 0xFFFFFF80;
>> +        msr.h |= try_hi;
>> +        wrmsrq(MSR_TMTA_LONGRUN_CTRL, msr.q);
>>           /* read out current core MHz and current perf_pctg */
>>           cpuid(0x80860007, &eax, &ebx, &ecx, &edx);
>>           /* restore values */
>> -        wrmsr(MSR_TMTA_LONGRUN_CTRL, save_lo, save_hi);
>> +        msr.l = save_lo;
>> +        msr.h = save_hi;
>> +        wrmsrq(MSR_TMTA_LONGRUN_CTRL, msr.q);
>>       }
> 
> As sashiko.dev pointed, from the second iter, this patch code
> will clear upper 25 bits to zero.
> 
> https://sashiko.dev/#/patchset/20260629060526.3638272-1-jgross%40suse.com
> 
> for (try_hi = 80; ...; ...) {
> -   msr_lo &= 0xFFFFFF80;    /* iter 1: X.lo[31:7]; iter 2+: same */
> -   msr_hi &= 0xFFFFFF80;
> -   msr_hi |= try_hi;
> -   wrmsr(reg, msr_lo, msr_hi);/* writes X.hi[31:7] | try_hi    */
> +   msr.l &= 0xFFFFFF80;      /* iter 2+: locals=0 -> mask = 0  */
> +   msr.h &= 0xFFFFFF80;
> +   msr.h |= try_hi;
> +   wrmsrq(reg, msr.q);   /* <-- iter 2+ writes 0 (vs X.hi[31:7])*/
> 
>      cpuid(...);
> 
>      /* restore */
> -   wrmsr(reg, save_lo, save_hi);  /* locals untouched */
> +   msr.l = save_lo;              /* poisons locals ->next iter bug  */
> +   msr.h = save_hi;
> +   wrmsrq(reg, msr.q);
> }
> 
> I'm not sure whether the upper 25 bits are reserved, but the original
> code preserved them across iterations. It might be safer to keep that
> behavior unchanged.

Will fix that.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]