RE: [PATCH v3 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: Monday, March 24, 2025 11:48 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; Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> Subject: Re: [PATCH v3 07/15] xen/cpufreq: fix core frequency calculation for 
> AMD
> Family 1Ah CPUs
>
> On 06.03.2025 09:39, Penny Zheng wrote:
> > This commit fixes core frequency calculation for AMD Family 1Ah CPUs,
> > 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:
>
> Why 1Ah+? In the code you correctly limit to just 1Ah.
>
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -572,12 +572,24 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
> >                                                            :
> > c->cpu_core_id);  }
> >
> > +static uint64_t amd_parse_freq(const struct cpuinfo_x86 *c, uint64_t
> > +value) {
> > +   ASSERT(c->x86 <= 0x1A);
> > +
> > +   if (c->x86 < 0x17)
> > +           return (((value & 0x3f) + 0x10) * 100) >> ((value >> 6) & 7);
> > +   else if (c->x86 <= 0x19)
> > +           return ((value & 0xff) * 25 * 8) / ((value >> 8) & 0x3f);
> > +   else
> > +           return (value & 0xfff) * 5;
> > +}
>
> Could I talk you into omitting the unnecessary "else" in cases like this one?
> (This may also make sense to express as switch().)
>

Sorry, bad habit... will change it to switch

> > @@ -658,19 +670,20 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
> >     if (!(lo >> 63))
> >             return;
> >
> > -#define FREQ(v) (c->x86 < 0x17 ? ((((v) & 0x3f) + 0x10) * 100) >> (((v) >> 
> > 6) &
> 7) \
> > -                                : (((v) & 0xff) * 25 * 8) / (((v) >> 8) & 
> > 0x3f))
> >     if (idx && idx < h &&
> >         !rdmsr_safe(0xC0010064 + idx, val) && (val >> 63) &&
> >         !rdmsr_safe(0xC0010064, hi) && (hi >> 63))
> >             printk("CPU%u: %lu (%lu ... %lu) MHz\n",
> > -                  smp_processor_id(), FREQ(val), FREQ(lo), FREQ(hi));
> > +                  smp_processor_id(),
> > +                  amd_parse_freq(c, val),
> > +                  amd_parse_freq(c, lo), amd_parse_freq(c, hi));
>
> I fear Misra won't like multiple function calls to evaluate the parameters to 
> pass to
> another function. Iirc smp_process_id() has special exception, so that's okay 
> here.
> This may be possible to alleviate by marking the new helper pure or even const
> (see gcc doc as to caveats with passing pointers to const functions). Cc-ing 
> Nicola
> for possible clarification or correction.
>

Maybe we shall declare the function __pure. Having checked the gcc doc,
``
a function that has pointer arguments must not be declared const
``
Otherwise we store the "c->x86" value to avoid using the pointer

> Jan