On 06.07.2023 20:54, Jason Andryuk wrote:
> Extend xen_get_cpufreq_para to return hwp parameters. HWP is an
> implementation of ACPI CPPC (Collaborative Processor Performance
> Control). Use the CPPC name since that might be useful in the future
> for AMD P-state.
>
> We need the features bitmask to indicate fields supported by the actual
> hardware - this only applies to activity window for the time being.
>
> The HWP most_efficient is mapped to CPPC lowest_nonlinear, and guaranteed is
> mapped to nominal. CPPC has a guaranteed that is optional while nominal
> is required. ACPI spec says "If this register is not implemented, OSPM
> assumes guaranteed performance is always equal to nominal performance."
>
> The use of uint8_t parameters matches the hardware size. uint32_t
> entries grows the sysctl_t past the build assertion in setup.c. The
> uint8_t ranges are supported across multiple generations, so hopefully
> they won't change.
Isn't this paragraph stale now?
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -251,46 +251,52 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
> else
> strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
>
> - if ( !(scaling_available_governors =
> - xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
> - return -ENOMEM;
> - if ( (ret = read_scaling_available_governors(scaling_available_governors,
> - gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
> + if ( !strncmp(op->u.get_para.scaling_driver, XEN_HWP_DRIVER_NAME,
> + CPUFREQ_NAME_LEN) )
> + ret = get_hwp_para(policy->cpu, &op->u.get_para.u.cppc_para);
> + else
> {
> + if ( !(scaling_available_governors =
> + xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
> + return -ENOMEM;
> + if ( (ret = read_scaling_available_governors(
> + scaling_available_governors,
> + gov_num * CPUFREQ_NAME_LEN *
> + sizeof(*scaling_available_governors) )) )
Nit: Too deep indentation of this last line. If you want to visually
express the continuation of the last argument, add a pair of parens:
if ( (ret = read_scaling_available_governors(
scaling_available_governors,
(gov_num * CPUFREQ_NAME_LEN *
sizeof(*scaling_available_governors)))) )
Otherwise all line beginnings want to align with one another, no matter
whether new or continued argument.
Also there's a stray blank after the 1st closing paren.
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -296,6 +296,61 @@ struct xen_ondemand {
> uint32_t up_threshold;
> };
>
> +struct xen_cppc_para {
> + /* OUT */
> + /* activity_window supported if set */
> +#define XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW (1 << 0)
> + uint32_t features; /* bit flags for features */
> + /*
> + * See Intel SDM: HWP Performance Range and Dynamic Capabilities
> + *
> + * These four are 0-255 hardware-provided values. They "continuous,
Nit: "They're"
> + * abstract unit-less, performance" values. Smaller numbers are slower
> + * and larger ones are faster.
> + */
With the adjustments (provided you agree)
Acked-by: Jan Beulich <jbeul...@suse.com>
Jan
