[AMD Official Use Only - AMD Internal Distribution Only]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, February 18, 2025 10:59 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andryuk, Jason
> <jason.andr...@amd.com>; Andrew Cooper <andrew.coop...@citrix.com>;
> Roger Pau Monné <roger....@citrix.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2 04/11] xen/amd: export processor max frequency value
>
> On 18.02.2025 07:14, Penny, Zheng wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi,
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeul...@suse.com>
> >> Sent: Tuesday, February 11, 2025 9:57 PM
> >> To: Penny, Zheng <penny.zh...@amd.com>
> >> Cc: Huang, Ray <ray.hu...@amd.com>; Andryuk, Jason
> >> <jason.andr...@amd.com>; Andrew Cooper <andrew.coop...@citrix.com>;
> >> Roger Pau Monné <roger....@citrix.com>;
> >> xen-devel@lists.xenproject.org
> >> Subject: Re: [PATCH v2 04/11] xen/amd: export processor max frequency
> >> value
> >>
> >> On 06.02.2025 09:32, Penny Zheng wrote:
> >>> --- a/xen/arch/x86/cpu/amd.c
> >>> +++ b/xen/arch/x86/cpu/amd.c
> >>> @@ -56,6 +56,8 @@ bool __initdata amd_virt_spec_ctrl;
> >>>
> >>> static bool __read_mostly fam17_c6_disabled;
> >>>
> >>> +DEFINE_PER_CPU_READ_MOSTLY(uint64_t, max_freq_mhz);
> >>
> >> Such an AMD-only variable would better have an amd_ prefix.
> >>
> >>> @@ -669,7 +671,12 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
> >>> printk("CPU%u: %lu ... %lu MHz\n",
> >>> smp_processor_id(), FREQ(lo), FREQ(hi));
> >>> else
> >>> + {
> >>> printk("CPU%u: %lu MHz\n", smp_processor_id(),
> >>> FREQ(lo));
> >>> + return;
> >>> + }
> >>> +
> >>> + per_cpu(max_freq_mhz, smp_processor_id()) = FREQ(hi);
> >>
> >> this_cpu() please, or latch the result of smp_processor_id() into a
> >> local variable (there are further uses in the function which then would
> >> want
> replacing).
> >>
> >> The function has "log" in its name for a reason. Did you look at the
> >> conditional at its very top? You won't get here for all CPUs. You
> >> won't get here at all for Fam1A CPUs, as for them the logic will first need
> amending.
> >
> > Sorry to overlook that
> > Then I shall add a specific amd_export_cpufreq_mhz to cover all scenarios...
> > For Fam1A, I could think of bringing back early DMI method right now...
>
> How reliable is DMI data going to be? Not to speak of it being available
> everwhere.
>
> > May I ask what is the more addressed specific reason for not applying to
> > Fam1A?
>
> I'm sorry, I may not understand the question. What I understand was already
> addressed by me having said "for them the logic will first need amending".
I've checked the latest spec
https://bugzilla.kernel.org/attachment.cgi?id=307010&action=edit
and found Linux already has similar patch to fix it,
https://lore.kernel.org/lkml/9ff1faf8-eec4-4776-a590-4efbc141f...@linuxfoundation.org/
I've written the following codes to let amd_log_freq() also adapt for 1a+
```
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 489e092815..c29e59d556 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -579,8 +579,7 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
unsigned int idx = 0, h;
uint64_t hi, lo, val;
- if (c->x86 < 0x10 || c->x86 > 0x19 ||
- (c != &boot_cpu_data &&
+ if (c->x86 < 0x10 || (c != &boot_cpu_data &&
(!opt_cpu_info || (c->apicid & (c->x86_num_siblings - 1)))))
return;
@@ -653,21 +652,23 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
wrmsrl(MSR_AMD64_NB_CFG, nbcfg);
}
+#define VALIDATE_FID(v) (c->x86 < 0x19 ? true : ((v & 0xfff) > 0x0f))
lo = 0; /* gcc may not recognize the loop having at least 5 iterations
*/
for (h = c->x86 == 0x10 ? 5 : 8; h--; )
- if (!rdmsr_safe(0xC0010064 + h, lo) && (lo >> 63))
- break;
+ if (!rdmsr_safe(0xC0010064 + h, lo) && (lo >> 63) &&
VALIDATE_FID(lo))
+ break;
if (!(lo >> 63))
return;
-#define FREQ(v) (c->x86 < 0x17 ? ((((v) & 0x3f) + 0x10) * 100) >> (((v) >> 6)
& 7) \
- : (((v) & 0xff) * 25 * 8) / (((v) >> 8) &
0x3f))
+#define FREQ(v) (c->x86 > 0x19 ? ((v & 0xfff) * 5) :
\
+ 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));
- else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63))
+ else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63) &&
VALIDATE_FID(hi))
printk("CPU%u: %lu ... %lu MHz\n",
smp_processor_id(), FREQ(lo), FREQ(hi));
else
@@ -678,6 +679,7 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
per_cpu(max_freq_mhz, smp_processor_id()) = FREQ(hi);
#undef FREQ
+#undef VALIDATE_FID
}
```
>
> Jan
