RE: [PATCH v2 04/11] xen/amd: export processor max frequency value

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, February 18, 2025 10:59 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andryuk, Jason
> <Jason.Andryuk@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxx>
> >> Sent: Tuesday, February 11, 2025 9:57 PM
> >> To: Penny, Zheng <penny.zheng@xxxxxxx>
> >> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andryuk, Jason
> >> <Jason.Andryuk@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
> >> Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> >> xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> 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@xxxxxxxxxxxxxxxxxxx/

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