| Message ID | 20250206083255.1296363-5-Penny.Zheng@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | amd-cppc CPU Performance Scaling Driver | expand |
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. Jan
[AMD Official Use Only - AMD Internal Distribution Only] Hi, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, February 11, 2025 9:57 PM > To: Penny, Zheng <penny.zheng@amd.com> > Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason > <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; > Roger Pau Monné <roger.pau@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... May I ask what is the more addressed specific reason for not applying to Fam1A? > > Jan Many thanks, Penny
On 18.02.2025 07:14, Penny, Zheng wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > Hi, > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, February 11, 2025 9:57 PM >> To: Penny, Zheng <penny.zheng@amd.com> >> Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason >> <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; >> Roger Pau Monné <roger.pau@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". Jan
[AMD Official Use Only - AMD Internal Distribution Only] Hi, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, February 18, 2025 10:59 PM > To: Penny, Zheng <penny.zheng@amd.com> > Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason > <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; > Roger Pau Monné <roger.pau@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 <jbeulich@suse.com> > >> Sent: Tuesday, February 11, 2025 9:57 PM > >> To: Penny, Zheng <penny.zheng@amd.com> > >> Cc: Huang, Ray <Ray.Huang@amd.com>; Andryuk, Jason > >> <Jason.Andryuk@amd.com>; Andrew Cooper <andrew.cooper3@citrix.com>; > >> Roger Pau Monné <roger.pau@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-4efbc141fe93@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
On 19.02.2025 08:32, Penny, Zheng wrote: > I've written the following codes to let amd_log_freq() also adapt for 1a+ > ``` > --- 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; On what basis do you drop the upper bound here altogether? Is there some guarantee given somewhere that the MSR layout isn't going to change again? You also want to pay attention to style (indentation here in particular) when making such adjustments. The conditional here also continues to mean the rest of the function won't normally be executed for all CPUs. Maybe that was intentional, with the goal of just adding Fam1A support here. Hard to tell without a patch description. > @@ -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)) Please be sure to parenthesize macro arguments when used in expressions. Not doing so violates at least one Misra rule. At the same time, seeing how many parentheses there are already in e.g. FREQ(), please try to avoid adding excess ones (here and there). Also, if you add such validation, Wouldn't that be equally needed for e.g. Fam19 (didn't check others)? Plus if you validate FID there, wouldn't it be yet more important to validate the divisor, too? (So far we've gone from the assumption that firmware will put sane values in when setting PstateEn.) > 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)) This is getting unwieldy, I'm afraid. We may need to introduce a helper function here. Or it would need re-wrapping / re-indentation to become halfway readable again. Plus can we please arrange things so we handle families in either consistently ascending order, or consistently descending one? Jan
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index 597b0f073d..489e092815 100644 --- 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); + static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo, unsigned int *hi) { @@ -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); #undef FREQ } diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h index 9c9599a622..96367ba646 100644 --- a/xen/arch/x86/include/asm/amd.h +++ b/xen/arch/x86/include/asm/amd.h @@ -174,4 +174,5 @@ bool amd_setup_legacy_ssbd(void); void amd_set_legacy_ssbd(bool enable); void amd_set_cpuid_user_dis(bool enable); +DECLARE_PER_CPU(uint64_t, max_freq_mhz); #endif /* __AMD_H__ */
When _CPC table could not provide processor frequency range values for OS governor, we need to read processor max frequency as anchor point. For AMD processors, we export max frequency value from amd_log_freq() Signed-off-by: Penny Zheng <Penny.Zheng@amd.com> --- v1 -> v2: - new commit --- xen/arch/x86/cpu/amd.c | 7 +++++++ xen/arch/x86/include/asm/amd.h | 1 + 2 files changed, 8 insertions(+)