On 11.07.2025 05:50, Penny Zheng wrote:
> --- a/xen/drivers/acpi/pm-op.c
> +++ b/xen/drivers/acpi/pm-op.c
> @@ -91,7 +91,8 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
> pmpt = processor_pminfo[op->cpuid];
> policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
>
> - if ( !pmpt || !pmpt->perf.states ||
> + if ( !pmpt || ((pmpt->init & XEN_PX_INIT) && !pmpt->perf.states) ||
> + ((pmpt->init & XEN_CPPC_INIT) && pmpt->perf.state_count) ||
Nit: I think this would be neater if the PX_INIT part was also moved to its own
line.
> @@ -697,6 +703,122 @@ int acpi_set_pdc_bits(unsigned int acpi_id,
> XEN_GUEST_HANDLE(uint32) pdc)
> return ret;
> }
>
> +static void print_CPPC(const struct xen_processor_cppc *cppc_data)
> +{
> + printk("\t_CPC: highest_perf=%u, lowest_perf=%u, "
> + "nominal_perf=%u, lowest_nonlinear_perf=%u, "
> + "nominal_mhz=%uMHz, lowest_mhz=%uMHz\n",
> + cppc_data->cpc.highest_perf, cppc_data->cpc.lowest_perf,
> + cppc_data->cpc.nominal_perf, cppc_data->cpc.lowest_nonlinear_perf,
> + cppc_data->cpc.nominal_mhz, cppc_data->cpc.lowest_mhz);
> +}
> +
> +int set_cppc_pminfo(unsigned int acpi_id,
> + const struct xen_processor_cppc *cppc_data)
> +{
> + int ret = 0, cpuid;
> + struct processor_pminfo *pm_info;
> +
> + cpuid = get_cpu_id(acpi_id);
> + if ( cpuid < 0 )
> + {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if ( cppc_data->pad[0] || cppc_data->pad[1] || cppc_data->pad[2] )
> + {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if ( cpufreq_verbose )
> + printk("Set CPU acpi_id(%u) cpuid(%d) CPPC State info:\n",
May I suggest "Set CPU%d (ACPI ID %u) CPPC state info:\n"
> + acpi_id, cpuid);
> +
> + pm_info = processor_pminfo[cpuid];
> + if ( !pm_info )
> + {
> + pm_info = xvzalloc(struct processor_pminfo);
> + if ( !pm_info )
> + {
> + ret = -ENOMEM;
> + goto out;
> + }
> + processor_pminfo[cpuid] = pm_info;
> + }
> + pm_info->acpi_id = acpi_id;
> + pm_info->id = cpuid;
> + pm_info->cppc_data = *cppc_data;
> +
> + if ( cppc_data->flags & XEN_CPPC_PSD )
> + if ( !check_psd_pminfo(cppc_data->shared_type) )
Please convert these into a single if().
> + {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if ( cppc_data->flags & XEN_CPPC_CPC )
> + {
> + if ( cppc_data->cpc.highest_perf == 0 ||
> + cppc_data->cpc.highest_perf > UINT8_MAX ||
> + cppc_data->cpc.nominal_perf == 0 ||
> + cppc_data->cpc.nominal_perf > UINT8_MAX ||
> + cppc_data->cpc.lowest_nonlinear_perf == 0 ||
> + cppc_data->cpc.lowest_nonlinear_perf > UINT8_MAX ||
> + cppc_data->cpc.lowest_perf == 0 ||
> + cppc_data->cpc.lowest_perf > UINT8_MAX ||
> + cppc_data->cpc.lowest_perf >
> + cppc_data->cpc.lowest_nonlinear_perf ||
> + cppc_data->cpc.lowest_nonlinear_perf >
> + cppc_data->cpc.nominal_perf ||
Indentation is a little odd here. Best may be to use parentheses:
cppc_data->cpc.lowest_perf > UINT8_MAX ||
(cppc_data->cpc.lowest_perf >
cppc_data->cpc.lowest_nonlinear_perf) ||
(cppc_data->cpc.lowest_nonlinear_perf >
cppc_data->cpc.nominal_perf) ||
Otherwise, strictly speaking, no extra indentation should be used. I can see
though that this would hamper readability, so the next best alternative would
appear to be to make the extra indentation a proper level (i.e. 4 blanks):
cppc_data->cpc.lowest_perf > UINT8_MAX ||
cppc_data->cpc.lowest_perf >
cppc_data->cpc.lowest_nonlinear_perf ||
cppc_data->cpc.lowest_nonlinear_perf >
cppc_data->cpc.nominal_perf ||
> + cppc_data->cpc.nominal_perf > cppc_data->cpc.highest_perf )
> + /*
> + * Right now, Xen doesn't actually use highest_perf/nominal_perf/
> + * lowest_nonlinear_perf/lowest_perf values read from ACPI _CPC
> + * table. Xen reads CPPC capability MSR to get these four values.
> + * So warning is enough.
> + */
> + printk_once(XENLOG_WARNING
> + "Broken CPPC perf values: lowest(%u),
> nonlinear_lowest(%u), nominal(%u), highest(%u)\n",
> + cppc_data->cpc.lowest_perf,
> + cppc_data->cpc.lowest_nonlinear_perf,
> + cppc_data->cpc.nominal_perf,
> + cppc_data->cpc.highest_perf);
> +
> + /* lowest_mhz and nominal_mhz are optional value */
> + if ( cppc_data->cpc.lowest_mhz > cppc_data->cpc.nominal_mhz )
If they're optional, what if lowest_mhz is provided but nominal_mhz isn't?
Wouldn't the warning needlessly trigger in that case?
> + {
> + printk_once(XENLOG_WARNING
> + "Broken CPPC freq values: lowest(%u), nominal(%u)\n",
> + cppc_data->cpc.lowest_mhz,
> + cppc_data->cpc.nominal_mhz);
> + /* Re-set with zero values, instead of keeping invalid values */
> + pm_info->cppc_data.cpc.nominal_mhz = 0;
> + pm_info->cppc_data.cpc.lowest_mhz = 0;
> + }
> + }
> +
> + if ( cppc_data->flags == (XEN_CPPC_PSD | XEN_CPPC_CPC) )
> + {
> + if ( cpufreq_verbose )
> + {
> + print_PSD(&pm_info->cppc_data.domain_info);
> + print_CPPC(&pm_info->cppc_data);
> + }
> +
> + pm_info->init = XEN_CPPC_INIT;
> + ret = cpufreq_cpu_init(cpuid);
> + if ( ret )
> + printk_once(XENLOG_WARNING
> + "CPU%u failed amd-cppc mode init; use
> \"cpufreq=xen\" instead",
> + cpuid);
cpuid is still int, so wants printing with %d.
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -363,6 +363,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
> #define XEN_PM_PX 1
> #define XEN_PM_TX 2
> #define XEN_PM_PDC 3
> +#define XEN_PM_CPPC 4
>
> /* Px sub info type */
> #define XEN_PX_PCT 1
> @@ -370,6 +371,10 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
> #define XEN_PX_PPC 4
> #define XEN_PX_PSD 8
>
> +/* CPPC sub info type */
> +#define XEN_CPPC_PSD 1
> +#define XEN_CPPC_CPC 2
As per this, ...
> @@ -457,6 +462,26 @@ struct xen_processor_performance {
> typedef struct xen_processor_performance xen_processor_performance_t;
> DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
>
> +struct xen_processor_cppc {
> + uint8_t flags; /* IN: XEN_CPPC_xxx */
... it's a type that's living here, not a collection of flags. Any reason the
field isn't named "type"?
> + uint8_t pad[3];
> + /*
> + * IN: Subset _CPC fields useful for CPPC-compatible cpufreq
> + * driver's initialization
> + */
> + struct {
> + uint32_t highest_perf;
> + uint32_t nominal_perf;
> + uint32_t lowest_nonlinear_perf;
> + uint32_t lowest_perf;
> + uint32_t lowest_mhz;
> + uint32_t nominal_mhz;
> + } cpc;
What, again, was the reason to wrap these into a sub-struct?
Jan
