Re: [PATCH v5 08/18] xen/cpu: Expand core frequency calculation for AMD Family 1Ah CPUs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2025 10:48, Penny Zheng wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -583,12 +583,40 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
>                                                            : c->cpu_core_id);
>  }
>  
> +static unsigned int attr_const amd_parse_freq(unsigned int family,
> +                                           unsigned int value)
> +{
> +     unsigned int freq = 0;
> +
> +     switch (family) {
> +     case 0x10 ... 0x16:
> +             freq = (((value & 0x3f) + 0x10) * 100) >> ((value >> 6) & 7);
> +             break;
> +
> +     case 0x17 ... 0x19:
> +             freq = ((value & 0xff) * 25 * 8) / ((value >> 8) & 0x3f);
> +             break;
> +
> +     case 0x1A:
> +             freq = (value & 0xfff) * 5;
> +             break;
> +
> +     default:
> +             printk(XENLOG_ERR
> +                    "Unsupported cpu family 0x%x on cpufreq parsing",
> +                    family);

I think I requested before (elsewhere) to prefer %#x over 0x%x.

However, why the log message? With ...

> +             break;
> +     }
> +
> +     return freq;
> +}
> +
>  void amd_log_freq(const struct cpuinfo_x86 *c)
>  {
>       unsigned int idx = 0, h;
>       uint64_t hi, lo, val;
>  
> -     if (c->x86 < 0x10 || c->x86 > 0x19 ||
> +     if (c->x86 < 0x10 || c->x86 > 0x1A ||

... this condition, there simply could be ASSERT_UNREACHABLE() there? Happy to
adjust while committing, so long as you agree. With the adjustment:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan