[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 07/15] xen/cpufreq: fix core frequency calculation for AMD Family 1Ah CPUs
On 26.03.2025 11:14, Nicola Vetrini wrote: > On 2025-03-26 10:54, Penny, Zheng wrote: >>> -----Original Message----- >>> From: Jan Beulich <jbeulich@xxxxxxxx> >>> Sent: Monday, March 24, 2025 11:48 PM >>> >>> 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. Indeed this matches my reading of it. Yet things are somewhat delicate here, so I like to always leave room for being proven wrong. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |