On 11.07.2025 05:51, Penny Zheng wrote:
> --- a/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> @@ -67,7 +67,14 @@
> * max_perf.
> * Field des_perf conveys performance level Xen governor is requesting. And
> it
> * may be set to any performance value in the range [min_perf, max_perf],
> - * inclusive.
> + * inclusive. In active mode, desf_perf must be zero.
Nit (typo): des_perf
> @@ -259,11 +276,18 @@ static void cf_check amd_cppc_write_request_msrs(void
> *info)
> }
>
> static void amd_cppc_write_request(unsigned int cpu, uint8_t min_perf,
> - uint8_t des_perf, uint8_t max_perf)
> + uint8_t des_perf, uint8_t max_perf,
> + uint8_t epp)
> {
> struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data, cpu);
> uint64_t prev = data->req.raw;
>
> + if ( !opt_active_mode )
> + data->req.des_perf = des_perf;
> + else
> + data->req.des_perf = 0;
In amd_cppc_epp_set_policy() you pass 0 anyway. Why is this needed? With this
change dropped, opt_active_mode can become __initdata. (But of course you may
want to add an assertion instead, in which case the variable needs to stay
where it is at least in debug builds.)
> + data->req.epp = epp;
Ahead of this patch, aren't you mis-handling this field then, in that you
clear it (as you never read the MSR)?
> data->req.min_perf = min_perf;
> data->req.max_perf = max_perf;
> data->req.des_perf = des_perf;
Don't you need to delete this line with the addition above, or alternatively
change the above to
if ( opt_active_mode )
data->req.des_perf = 0;
?
> @@ -274,6 +298,14 @@ static void amd_cppc_write_request(unsigned int cpu,
> uint8_t min_perf,
> on_selected_cpus(cpumask_of(cpu), amd_cppc_write_request_msrs, data, 1);
> }
>
> +static void read_epp_init(void)
> +{
> + uint64_t val;
> +
> + rdmsrl(MSR_AMD_CPPC_REQ, val);
> + this_cpu(epp_init) = MASK_EXTR(val, AMD_CPPC_EPP_MASK);
> +}
I'm unconvinced this is worth a separate function.
> +static int cf_check amd_cppc_epp_set_policy(struct cpufreq_policy *policy)
> +{
> + const struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data,
> + policy->cpu);
> + uint8_t max_perf, min_perf, epp;
> +
> + /*
> + * On default, set min_perf with lowest_nonlinear_perf, and max_perf
> + * with the highest, to ensure performance scaling in P-states range.
> + */
> + max_perf = data->caps.highest_perf;
> + min_perf = data->caps.lowest_nonlinear_perf;
> +
> + /*
> + * In policy CPUFREQ_POLICY_PERFORMANCE, increase min_perf to
> + * highest_perf to achieve ultmost performance.
> + * In policy CPUFREQ_POLICY_POWERSAVE, decrease max_perf to
> + * lowest_nonlinear_perf to achieve ultmost power saving.
> + */
> + switch ( policy->policy )
> + {
> + case CPUFREQ_POLICY_PERFORMANCE:
> + /* Force the epp value to be zero for performance policy */
> + epp = CPPC_ENERGY_PERF_MAX_PERFORMANCE;
> + min_perf = data->caps.highest_perf;
Use the local variable you have, i.e. max_perf?
> + break;
> +
> + case CPUFREQ_POLICY_POWERSAVE:
> + /* Force the epp value to be 0xff for powersave policy */
> + epp = CPPC_ENERGY_PERF_MAX_POWERSAVE;
> + max_perf = data->caps.lowest_nonlinear_perf;
Use the local variable you have, i.e. min_perf?
> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -245,6 +245,7 @@
> #define MSR_AMD_CPPC_ENABLE 0xc00102b1U
> #define AMD_CPPC_ENABLE (_AC(1, ULL) << 0)
> #define MSR_AMD_CPPC_REQ 0xc00102b3U
> +#define AMD_CPPC_EPP_MASK (_AC(0xff, ULL) << 24)
The reason I noticed the EPP issue in amd_cppc_write_request() is because
I wondered why you would need this, when you have the fields defined in
struct amd_cppc_drv_data.
Jan
