Re: [PATCH v5 09/18] xen/amd: introduce amd_process_freq() to get processor frequency

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2025 10:48, Penny Zheng wrote:
> When _CPC table could not provide processor frequency range
> values for Xen governor, we need to read processor max frequency
> as anchor point.
> So we extract amd cpu core frequency calculation logic from amd_log_freq(),
> and wrap it as a new helper amd_process_freq().
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
> v1 -> v2:
> - new commit
> ---
> v3 -> v4
> - introduce amd_process_freq()
> ---
> v4 -> v5:
> - make amd_process_freq() static to statisfy Misra demand
> - change "low_mhz", "nom_mhz" and "hi_mhz" parameter to unsigned int
> - fix order of logged frequencies
> ---
>  xen/arch/x86/cpu/amd.c | 58 +++++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index 3770d75150..8c985466fa 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -611,14 +611,15 @@ static unsigned int attr_const amd_parse_freq(unsigned 
> int family,
>       return freq;
>  }
>  
> -void amd_log_freq(const struct cpuinfo_x86 *c)
> +static void amd_process_freq(const struct cpuinfo_x86 *c,
> +                          unsigned int *low_mhz,
> +                          unsigned int *nom_mhz,
> +                          unsigned int *hi_mhz)
>  {
>       unsigned int idx = 0, h;
>       uint64_t hi, lo, val;
>  
> -     if (c->x86 < 0x10 || c->x86 > 0x1A ||
> -         (c != &boot_cpu_data &&
> -          (!opt_cpu_info || (c->apicid & (c->x86_num_siblings - 1)))))
> +     if (c->x86 < 0x10 || c->x86 > 0x1A)
>               return;
>  
>       if (c->x86 < 0x17) {
> @@ -699,20 +700,20 @@ 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: %u (%u ... %u) 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: %u ... %u MHz\n",
> -                    smp_processor_id(),
> -                    amd_parse_freq(c->x86, lo),
> -                    amd_parse_freq(c->x86, hi));
> -     else
> -             printk("CPU%u: %u 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)
> +             *low_mhz = amd_parse_freq(c->x86, lo);
>  }
>  
>  void cf_check early_init_amd(struct cpuinfo_x86 *c)
> @@ -723,6 +724,27 @@ void cf_check early_init_amd(struct cpuinfo_x86 *c)
>       ctxt_switch_levelling(NULL);
>  }
>  
> +void amd_log_freq(const struct cpuinfo_x86 *c)
> +{
> +     unsigned int low_mhz = 0, nom_mhz = 0, hi_mhz = 0;
> +
> +     if (c != &boot_cpu_data &&
> +         (!opt_cpu_info || (c->apicid & (c->x86_num_siblings - 1))))
> +             return;
> +
> +     amd_process_freq(c, &low_mhz, &nom_mhz, &hi_mhz);
> +
> +     if (!low_mhz && !nom_mhz && !hi_mhz)

With this ...

> +             printk("CPU%u: %u (%u ... %u) MHz\n",
> +                    smp_processor_id(),
> +                    nom_mhz, low_mhz, hi_mhz);

.. aren't you logging three zeroes here? Considering the earlier mistake
of the order of logged frequencies, did you not think of looking at the
resulting output in the hypervisor boot log?

Same issue again below then.

With all instances corrected:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan