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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-03-26 10:54, Penny, Zheng wrote:
> [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
Either way could work. ECLAIR will automatically pick up 
__attribute__((pure)) or __attribute__((const)) from the declaration. 
Maybe it could be const, as from a cursory look I don't think the gcc 
restriction on pointer arguments applies, as the pointee is not modified 
between successive calls, but I might be mistaken.
> > Jan
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253