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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, April 17, 2025 11:23 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> 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.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).
>

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