Re: [PATCH v5 09/15] cpufreq: Export HWP parameters to userspace as CPPC

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jbeulich@xxxxxxxx>

Jan