[AMD Official Use Only - AMD Internal Distribution Only]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Thursday, January 9, 2025 6:55 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Stabellini, Stefano <stefano.stabell...@amd.com>; Huang, Ray
> <ray.hu...@amd.com>; Ragiadakou, Xenia <xenia.ragiada...@amd.com>;
> Andryuk, Jason <jason.andr...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>; xen-
> de...@lists.xenproject.org
> 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
