Re: [PATCH v4 07/15] xen/cpufreq: fix core frequency calculation for AMD Family 1Ah CPUs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 09:40, Penny Zheng wrote:
> AMD Family 1Ah CPU needs a different COF(Core Operating Frequency) formula,
> due to a change in the PStateDef MSR layout in AMD Family 1Ah.
> In AMD Family 1Ah, Core current operating frequency in MHz is calculated as
> follows:
>     CoreCOF = Core::X86::Msr::PStateDef[CpuFid[11:0]] * 5MHz
> 
> We introduce a helper amd_parse_freq() to parse cpu min/nominal/max core
> frequency from PstateDef register, to replace the original macro FREQ(v).
> amd_parse_freq() is declared as const, as it mainly consists of mathematical
> conputation.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>

As to the title: I don't think "fix" is appropriate here. Or else I'd expect
a Fixes: tag to be there, which I think would be hard for you to fish out
(as the earlier changes here weren't broken; information on Fam1A simply
wasn't available at the time).

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -56,6 +56,9 @@ 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);

Why is this declaration needed?

> +#define INVAL_FREQ_MHZ  ~(uint64_t)0

This may want to move down, ahead of the function. And the expansion wants
to be wrapped in parentheses.

> @@ -570,12 +573,35 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
>                                                            : c->cpu_core_id);
>  }
>  
> +static uint64_t amd_parse_freq(unsigned char c, uint64_t value)

Considering how it's used, does "value" need to be any wider than unsigned
int? What about the return type?

I also think the first argument would better be unsigned int, and would
better be named e.g. "family".

> +{
> +     uint64_t freq = INVAL_FREQ_MHZ;
> +
> +     switch (c) {
> +     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:

Nit: Blank lines please between non-fall-through case blocks.

> +             printk(XENLOG_ERR
> +                    "Unsupported cpu familly %c on cpufreq parsing", c);

Nit: family

Jan