[PATCH v5 08/18] xen/cpu: Expand core frequency calculation for AMD Family 1Ah CPUs

Penny Zheng posted 18 patches 5 months, 1 week ago
There is a newer version of this series
[PATCH v5 08/18] xen/cpu: Expand core frequency calculation for AMD Family 1Ah CPUs
Posted by Penny Zheng 5 months, 1 week ago
AMD Family 1Ah CPU needs a different COF(Core Operating Frequency) formula,
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:
    CoreCOF = Core::X86::Msr::PStateDef[CpuFid[11:0]] * 5MHz

We introduce a helper amd_parse_freq() to parse COF(Core Operating Frequency)
from PstateDef register, to replace the original macro FREQ(v).
amd_parse_freq() is declared as const, as it mainly consists of mathematical
conputation.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v2 -> v3:
- new commit
---
v3 -> v4:
 - introduce amd_parse_freq() and declare it as const
 - express if-else-arry() as switch()
---
v4 -> v5:
- change title from "fix xxx" to "expand xxx"
- change function parameter type and return type to "unsigned int"
- blank lines between non-fall-through case blocks
---
 xen/arch/x86/cpu/amd.c | 49 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 37d67dd15c..3770d75150 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -583,12 +583,40 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
                                                           : c->cpu_core_id);
 }
 
+static unsigned int attr_const amd_parse_freq(unsigned int family,
+					      unsigned int value)
+{
+	unsigned int freq = 0;
+
+	switch (family) {
+	case 0x10 ... 0x16:
+		freq = (((value & 0x3f) + 0x10) * 100) >> ((value >> 6) & 7);
+		break;
+
+	case 0x17 ... 0x19:
+		freq = ((value & 0xff) * 25 * 8) / ((value >> 8) & 0x3f);
+		break;
+
+	case 0x1A:
+		freq = (value & 0xfff) * 5;
+		break;
+
+	default:
+		printk(XENLOG_ERR
+		       "Unsupported cpu family 0x%x on cpufreq parsing",
+		       family);
+		break;
+	}
+
+	return freq;
+}
+
 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 ||
+	if (c->x86 < 0x10 || c->x86 > 0x1A ||
 	    (c != &boot_cpu_data &&
 	     (!opt_cpu_info || (c->apicid & (c->x86_num_siblings - 1)))))
 		return;
@@ -669,19 +697,22 @@ 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));
+		printk("CPU%u: %u (%u ... %u) MHz\n",
+		       smp_processor_id(),
+		       amd_parse_freq(c->x86, val),
+		       amd_parse_freq(c->x86, lo),
+		       amd_parse_freq(c->x86, hi));
 	else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63))
-		printk("CPU%u: %lu ... %lu MHz\n",
-		       smp_processor_id(), FREQ(lo), FREQ(hi));
+		printk("CPU%u: %u ... %u MHz\n",
+		       smp_processor_id(),
+		       amd_parse_freq(c->x86, lo),
+		       amd_parse_freq(c->x86, hi));
 	else
-		printk("CPU%u: %lu MHz\n", smp_processor_id(), FREQ(lo));
-#undef FREQ
+		printk("CPU%u: %u MHz\n", smp_processor_id(),
+		       amd_parse_freq(c->x86, lo));
 }
 
 void cf_check early_init_amd(struct cpuinfo_x86 *c)
-- 
2.34.1
Re: [PATCH v5 08/18] xen/cpu: Expand core frequency calculation for AMD Family 1Ah CPUs
Posted by Jan Beulich 4 months, 2 weeks ago
On 27.05.2025 10:48, Penny Zheng wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -583,12 +583,40 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
>                                                            : c->cpu_core_id);
>  }
>  
> +static unsigned int attr_const amd_parse_freq(unsigned int family,
> +					      unsigned int value)
> +{
> +	unsigned int freq = 0;
> +
> +	switch (family) {
> +	case 0x10 ... 0x16:
> +		freq = (((value & 0x3f) + 0x10) * 100) >> ((value >> 6) & 7);
> +		break;
> +
> +	case 0x17 ... 0x19:
> +		freq = ((value & 0xff) * 25 * 8) / ((value >> 8) & 0x3f);
> +		break;
> +
> +	case 0x1A:
> +		freq = (value & 0xfff) * 5;
> +		break;
> +
> +	default:
> +		printk(XENLOG_ERR
> +		       "Unsupported cpu family 0x%x on cpufreq parsing",
> +		       family);

I think I requested before (elsewhere) to prefer %#x over 0x%x.

However, why the log message? With ...

> +		break;
> +	}
> +
> +	return freq;
> +}
> +
>  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 ||
> +	if (c->x86 < 0x10 || c->x86 > 0x1A ||

... this condition, there simply could be ASSERT_UNREACHABLE() there? Happy to
adjust while committing, so long as you agree. With the adjustment:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
RE: [PATCH v5 08/18] xen/cpu: Expand core frequency calculation for AMD Family 1Ah CPUs
Posted by Penny, Zheng 4 months, 2 weeks ago
[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, June 12, 2025 9:08 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v5 08/18] xen/cpu: Expand core frequency calculation for
> AMD Family 1Ah CPUs
>
> On 27.05.2025 10:48, Penny Zheng wrote:
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -583,12 +583,40 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
> >                                                            :
> > c->cpu_core_id);  }
> >
> > +static unsigned int attr_const amd_parse_freq(unsigned int family,
> > +                                         unsigned int value)
> > +{
> > +   unsigned int freq = 0;
> > +
> > +   switch (family) {
> > +   case 0x10 ... 0x16:
> > +           freq = (((value & 0x3f) + 0x10) * 100) >> ((value >> 6) & 7);
> > +           break;
> > +
> > +   case 0x17 ... 0x19:
> > +           freq = ((value & 0xff) * 25 * 8) / ((value >> 8) & 0x3f);
> > +           break;
> > +
> > +   case 0x1A:
> > +           freq = (value & 0xfff) * 5;
> > +           break;
> > +
> > +   default:
> > +           printk(XENLOG_ERR
> > +                  "Unsupported cpu family 0x%x on cpufreq parsing",
> > +                  family);
>
> I think I requested before (elsewhere) to prefer %#x over 0x%x.
>
> However, why the log message? With ...
>
> > +           break;
> > +   }
> > +
> > +   return freq;
> > +}
> > +
> >  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 ||
> > +   if (c->x86 < 0x10 || c->x86 > 0x1A ||
>
> ... this condition, there simply could be ASSERT_UNREACHABLE() there? Happy
> to adjust while committing, so long as you agree. With the adjustment:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>

Agreed, thx

> Jan