RE: [PATCH v1 05/11] xen/x86: introduce a new amd pstate driver for cpufreq scaling

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, January 9, 2025 6:55 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Stabellini, Stefano <stefano.stabellini@xxxxxxx>; Huang, Ray
> <Ray.Huang@xxxxxxx>; Ragiadakou, Xenia <Xenia.Ragiadakou@xxxxxxx>;
> Andryuk, Jason <Jason.Andryuk@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 05/11] xen/x86: introduce a new amd pstate driver for
> cpufreq scaling
>
> On 03.12.2024 09:11, Penny Zheng wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/amd-pstate.c
> > +++ b/xen/arch/x86/acpi/cpufreq/amd-pstate.c
> > @@ -15,6 +15,53 @@
> >  #include <xen/init.h>
> >  #include <xen/param.h>
> >  #include <acpi/cpufreq/cpufreq.h>
> > +#include <asm/msr.h>
> > +
> > +#define amd_pstate_err(cpu, fmt, args...) \
> > +    printk(XENLOG_ERR "AMD_PSTATE: CPU%u error: " fmt, cpu, ## args)
> > +#define amd_pstate_verbose(fmt, args...)                         \
> > +({                                                               \
> > +    if ( cpufreq_verbose )                                       \
> > +        printk(XENLOG_DEBUG "AMD_PSTATE: " fmt, ## args);        \
> > +})
> > +#define amd_pstate_warn(fmt, args...) \
> > +    printk(XENLOG_WARNING "AMD_PSTATE: CPU%u warning: " fmt, cpu,
> ##
> > +args)
> > +
> > +struct amd_pstate_drv_data
> > +{
> > +    struct xen_processor_cppc *cppc_data;
> > +    union
> > +    {
> > +        uint64_t amd_caps;
> > +        struct
> > +        {
> > +            unsigned int lowest_perf:8;
> > +            unsigned int lowest_nonlinear_perf:8;
> > +            unsigned int nominal_perf:8;
> > +            unsigned int highest_perf:8;
> > +            unsigned int :32;
> > +        } hw;
>
> Please can this be
>
>
>     union {
>         uint64_t raw;
>         struct {
>             unsigned int lowest_perf:8;
>             unsigned int lowest_nonlinear_perf:8;
>             unsigned int nominal_perf:8;
>             unsigned int highest_perf:8;
>             unsigned int :32;
>         };
>     } caps;
>
> such that at use sites (e.g. amd_pstate_write_request()) it is possible to 
> actually
> spot that the same thing is being accessed through the two parts of the union?
>

Understood.

> > +    {
> > +        /* Read Processor Max Speed(mhz) from DMI table as anchor point */
> > +        mul = dmi_max_speed_mhz;
> > +        div = data->hw.highest_perf;
> > +
> > +        return (unsigned int)(mul / div) * data->hw.lowest_perf *
> > + 1000;
>
> No risk of the cast chopping off set bits?

Mul shall be uint64_t, highest_perf and lowest_perf shall be uint8_t, and 
normally
the equation output shall not be a too big value...
If you think it's safer to do cast after comparing with the UINT32_MAX, I will 
add the check

>
> > +
> > +static int cf_check amd_pstate_cpufreq_verify(struct cpufreq_policy
> > +*policy) {
> > +    struct amd_pstate_drv_data *data = per_cpu(amd_pstate_drv_data,
> > +policy->cpu);
>
> Const? (I won't further repeat this. Please make pointers pointer-to- const
> wherever possible. We aim at having fully const-correct code.)
>

Sure.

> > +}
> > +
> > +static void cf_check amd_pstate_init_msrs(void *info) {
> > +    struct cpufreq_policy *policy = info;
> > +    struct amd_pstate_drv_data *data = this_cpu(amd_pstate_drv_data);
> > +    uint64_t val;
> > +    unsigned int min_freq, nominal_freq, max_freq;
> > +
> > +    /* Package level MSR */
> > +    if ( rdmsr_safe(MSR_AMD_CPPC_ENABLE, val) )
>
> Before trying this, wouldn't you better exclude certain very old families?
> Even looking at a random Fam17 PPR I can't spot documentation of this MSR
> (nor the other two), despite you merely saying Zen elsewhere (without any
> version number).
>

I shall comment more specifically, this feature is firstly introduced on some 
Zen2 serie, like Renoir
I'll exclude the families before Zen(Fam17h)

> > +    {
> > +        amd_pstate_err(policy->cpu,
> "rdmsr_safe(MSR_AMD_CPPC_ENABLE)\n");
> > +        data->err = -EINVAL;
> > +        return;
> > +    }
> > +
> > +    if ( !(val & AMD_CPPC_ENABLE) )
> > +    {
> > +        val |= AMD_CPPC_ENABLE;
> > +        if ( wrmsr_safe(MSR_AMD_CPPC_ENABLE, val) )
> > +        {
> > +            amd_pstate_err(policy->cpu,
> "wrmsr_safe(MSR_AMD_CPPC_ENABLE, %lx)\n", val);
> > +            data->err = -EINVAL;
> > +            return;
> > +        }
> > +    }
>
> Do you really need to enable befor reading ...
>
> > +    if ( rdmsr_safe(MSR_AMD_CPPC_CAP1, data->amd_caps) )
>
> ... capabilities and ...
>

Yes.
Only when CPPC is enabled, the hardware will calculate the processor’s 
performance capabilities and
initialize the performance level fields in the CPPC capability registers.
See 17.6.3 
https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf

> > +    {
> > +        amd_pstate_err(policy->cpu, "rdmsr_safe(MSR_AMD_CPPC_CAP1)\n");
> > +        goto error;
> > +    }
> > +
> > +    if ( data->hw.highest_perf == 0 || data->hw.lowest_perf == 0 ||
> > +         data->hw.nominal_perf == 0 || data->hw.lowest_nonlinear_perf == 0 
> > )
> > +    {
> > +        amd_pstate_err(policy->cpu, "Platform malfunction, read CPPC
> highest_perf: %u, lowest_perf: %u, nominal_perf: %u, lowest_nonlinear_perf: %u
> zero value\n",
> > +                       data->hw.highest_perf, data->hw.lowest_perf,
> > +                       data->hw.nominal_perf, 
> > data->hw.lowest_nonlinear_perf);
> > +        goto error;
> > +    }
> > +
> > +    min_freq = amd_get_min_freq(data);
> > +    nominal_freq = amd_get_nominal_freq(data);
> > +    max_freq = amd_get_max_freq(data);
> > +    if ( min_freq > max_freq )
> > +    {
> > +        amd_pstate_err(policy->cpu, "min_freq(%u) or max_freq(%u) value is
> incorrect\n",
> > +                       min_freq, max_freq);
> > +        goto error;
> > +    }
>
> ... checking them? Error handling would be easier if you enabled only 
> afterwards.
>
> > +    highest_perf = data->hw.highest_perf;
> > +    nominal_perf = data->hw.nominal_perf;
> > +
> > +    if ( highest_perf <= nominal_perf )
> > +        return;
> > +
> > +    policy->turbo = CPUFREQ_TURBO_ENABLED; }
>
> And why the helper variables in the first place?
>

Sorry, maybe a bit more specific, got confused about the question ;/

>
> Jan

Many thanks,
Penny