Re: [PATCH v4 08/15] xen/amd: introduce amd_process_freq() to get processor frequency

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 09:40, Penny Zheng wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -57,7 +57,6 @@ bool __initdata amd_virt_spec_ctrl;
>  static bool __read_mostly fam17_c6_disabled;
>  
>  static uint64_t attr_const amd_parse_freq(unsigned char c, uint64_t value);
> -#define INVAL_FREQ_MHZ  ~(uint64_t)0
>  
>  static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
>                                unsigned int *hi)
> @@ -596,14 +595,13 @@ static uint64_t amd_parse_freq(unsigned char c, 
> uint64_t value)
>       return freq;
>  }
>  
> -void amd_log_freq(const struct cpuinfo_x86 *c)
> +void amd_process_freq(const struct cpuinfo_x86 *c,

Misra demands that functions only used in a single CU be static.

> +                   uint64_t *low_mhz, uint64_t *nom_mhz, uint64_t *hi_mhz)

See comments on patch 07 as to types used here.

> @@ -684,20 +682,21 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
>  
>       if (idx && idx < h &&
>           !rdmsr_safe(0xC0010064 + idx, val) && (val >> 63) &&
> -         !rdmsr_safe(0xC0010064, hi) && (hi >> 63))
> -             printk("CPU%u: %lu (%lu ... %lu) MHz\n",
> -                    smp_processor_id(),
> -                    amd_parse_freq(c->x86, val),
> -                    amd_parse_freq(c->x86, lo),
> -                    amd_parse_freq(c->x86, hi));
> -     else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63))
> -             printk("CPU%u: %lu ... %lu MHz\n",
> -                    smp_processor_id(),
> -                    amd_parse_freq(c->x86, lo),
> -                    amd_parse_freq(c->x86, hi));
> -     else
> -             printk("CPU%u: %lu MHz\n", smp_processor_id(),
> -                    amd_parse_freq(c->x86, lo));
> +         !rdmsr_safe(0xC0010064, hi) && (hi >> 63)) {
> +             if (nom_mhz)
> +                     *nom_mhz = amd_parse_freq(c->x86, val);
> +             if (low_mhz)
> +                     *low_mhz = amd_parse_freq(c->x86, lo);
> +             if (hi_mhz)
> +                     *hi_mhz = amd_parse_freq(c->x86, hi);
> +     } else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63)) {
> +             if (low_mhz)
> +                     *low_mhz = amd_parse_freq(c->x86, lo);
> +             if (hi_mhz)
> +                     *hi_mhz = amd_parse_freq(c->x86, hi);
> +     } else
> +             if (low_mhz)

Why does this "else if()" extend across two lines?

> @@ -708,6 +707,29 @@ void cf_check early_init_amd(struct cpuinfo_x86 *c)
>       ctxt_switch_levelling(NULL);
>  }
>  
> +void amd_log_freq(const struct cpuinfo_x86 *c)
> +{
> +     uint64_t low_mhz, nom_mhz, hi_mhz;
> +
> +     if (c != &boot_cpu_data &&
> +         (!opt_cpu_info || (c->apicid & (c->x86_num_siblings - 1))))
> +             return;
> +
> +     low_mhz = nom_mhz = hi_mhz = INVAL_FREQ_MHZ;
> +     amd_process_freq(c, &low_mhz, &nom_mhz, &hi_mhz);
> +
> +     if (low_mhz != INVAL_FREQ_MHZ && nom_mhz != INVAL_FREQ_MHZ &&
> +         hi_mhz != INVAL_FREQ_MHZ)
> +             printk("CPU%u: %lu (%lu ... %lu) MHz\n",
> +                    smp_processor_id(),
> +                    low_mhz, nom_mhz, hi_mhz);

This doesn't match the original order of frequencies logged.

Jan