[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Thursday, April 17, 2025 11:23 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>; xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v4 07/15] xen/cpufreq: fix core frequency calculation for
> AMD
> Family 1Ah CPUs
>
> 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).
>
I will change it to "Expand core frequency calculation for AMD Family 1Ah
CPUs", or any better suggestion?
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -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?
>
Value is the value of 64bit PstateDef MSR, although we are only using the lower
32bit to calculate frequency
Maybe its better to leave it as uint64_t ?
I'll change the return type to unsigned int, and do the following check anyhow
#define INVAL_FREQ_MHZ (~(unsigned int)0)
if ( freq >= UINT_MAX )
return INVAL_FREQ_MHZ;
else
return (unsigned int) freq;
> I also think the first argument would better be unsigned int, and would
> better be
> named e.g. "family".
>
Understood
> Jan
