|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 04/18] xen/cpufreq: introduce new sub-hypercall to propagate CPPC data
On 27.05.2025 10:48, Penny Zheng wrote:
> @@ -635,6 +641,124 @@ out:
> 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 || !cppc_data )
The !cppc_data part isn't really needed, is it?
> + {
> + 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",
> + 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 )
> + {
> + ret = check_psd_pminfo(cppc_data->shared_type);
> + if ( ret )
> + 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.nominal_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 )
> + {
> + 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);
> +#ifndef NDEBUG
> + if ( ret )
> + dprintk(XENLOG_WARNING,
> + "CPU %u failed to be initialized with amd-cppc mode, and
> users could only reboot and re-define cmdline with \"cpufreq=xen\"",
> + cpuid);
> +#endif
What use if the #ifdef here? The more that NDEBUG controls behavior of ASSERT(),
not that of (debug) logging.
> --- 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
> +
> struct xen_power_register {
> uint32_t space_id;
> uint32_t bit_width;
> @@ -457,6 +462,30 @@ 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; /* flag for CPPC sub info type */
A common way of commenting on such would be /* XEN_CPPC_... */.
In any event, here and ...
> + uint8_t pad[3]; /* padding and must be zero */
... here (and of course anywhere else) - please adhere to designated comment
style.
> + /*
> + * 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;
> + /* Coordination type of this processor */
> +#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed coordination */
> +#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs should set freq
> */
> +#define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be set from any dependent
> CPU */
What's this? The exact same #define-s already exist in struct
xen_processor_performance, unless I'm missing a hard to notice spelling
difference. You can simply ...
> + uint32_t shared_type;
... use the comment suggestion from above here as well.
> --- a/xen/include/xen/pmstat.h
> +++ b/xen/include/xen/pmstat.h
> @@ -7,8 +7,13 @@
>
> int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf);
> long set_cx_pminfo(uint32_t acpi_id, struct xen_processor_power *power);
> +int set_cppc_pminfo(unsigned int acpi_id,
> + const struct xen_processor_cppc *cppc_data);
Nit: Relative ordering would be nice to be consistent between this group
and ...
> #ifdef CONFIG_COMPAT
> +struct compat_processor_cppc;
> +int compat_set_cppc_pminfo(unsigned int acpi_id,
> + const struct compat_processor_cppc *cppc_data);
> struct compat_processor_performance;
> int compat_set_px_pminfo(uint32_t acpi_id, struct
> compat_processor_performance *perf);
> struct compat_processor_power;
... this one. (Personally I dislike additions at the top of any group,
as this gives the impression that the new stuff is "more important" than
what has been there already. See also how in a number of places we simply
sort alphabetically, to avoid the common "insert at top" and "insert at
bottom" patterns.)
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -168,6 +168,7 @@
> ! processor_performance platform.h
> ! processor_power platform.h
> ? processor_px platform.h
> +! processor_cppc platform.h
> ! psd_package platform.h
> ? xenpf_enter_acpi_sleep platform.h
> ? xenpf_pcpu_version platform.h
Please obey to sorting in this file.
Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |