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.zh...@amd.com>
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
