[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, March 24, 2025 11:48 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; Nicola Vetrini <nicola.vetr...@bugseng.com>
> 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
