Re: [PATCH v3 07/14 RESEND] cpufreq: Export HWP parameters to userspace

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.05.2023 21:30, Jason Andryuk wrote:
> Extend xen_get_cpufreq_para to return hwp parameters.  These match the
> hardware rather closely.
> 
> We need the features bitmask to indicated fields supported by the actual
> hardware.
> 
> 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.

Still it feels a little odd for values to be this narrow. Aiui the
scaling_governor[] and scaling_{max,min}_freq fields aren't (really)
used by HWP. So you could widen the union in struct
xen_get_cpufreq_para (in a binary but not necessarily source compatible
manner), gaining you 6 more uint32_t slots. Possibly the somewhat oddly
placed scaling_cur_freq could be included as well ...

> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> @@ -506,6 +506,31 @@ static const struct cpufreq_driver __initconstrel 
> hwp_cpufreq_driver =
>      .update = hwp_cpufreq_update,
>  };
>  
> +int get_hwp_para(const struct cpufreq_policy *policy,

While I don't really mind a policy being passed into here, ...

> +                 struct xen_hwp_para *hwp_para)
> +{
> +    unsigned int cpu = policy->cpu;

... this is its only use afaics, and hence the caller could as well pass
in just a CPU number?

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -292,6 +292,31 @@ struct xen_ondemand {
>      uint32_t up_threshold;
>  };
>  
> +struct xen_hwp_para {
> +    /*
> +     * bits 6:0   - 7bit mantissa
> +     * bits 9:7   - 3bit base-10 exponent
> +     * btis 15:10 - Unused - must be 0
> +     */
> +#define HWP_ACT_WINDOW_MANTISSA_MASK  0x7f
> +#define HWP_ACT_WINDOW_EXPONENT_MASK  0x7
> +#define HWP_ACT_WINDOW_EXPONENT_SHIFT 7
> +    uint16_t activity_window;
> +    /* energy_perf range 0-255 if 1. Otherwise 0-15 */
> +#define XEN_SYSCTL_HWP_FEAT_ENERGY_PERF (1 << 0)
> +    /* activity_window supported if 1 */
> +#define XEN_SYSCTL_HWP_FEAT_ACT_WINDOW  (1 << 1)
> +    uint8_t features; /* bit flags for features */
> +    uint8_t lowest;
> +    uint8_t most_efficient;
> +    uint8_t guaranteed;
> +    uint8_t highest;
> +    uint8_t minimum;
> +    uint8_t maximum;
> +    uint8_t desired;
> +    uint8_t energy_perf;

These fields could do with some more commentary. To be honest I had
trouble figuring (from the SDM) what exact meaning specific numeric
values have. Readers of this header should at the very least be told
where they can turn to in order to understand what these fields
communicate. (FTAOD this could be section names, but please not
section numbers. The latter are fine to use in a discussion, but
they're changing too frequently to make them useful in code
comments.)

> +};

Also, if you decide to stick to uint8_t, then the trailing padding
field (another uint8_t) wants making explicit. I'm on the edge
whether to ask to also check the field: Right here the struct is
"get only", and peeking ahead you look to be introducing a separate
sub-op for "set". Perhaps if you added /* OUT */ at the top of the
new struct? (But if you don't check the field for being zero, then
you'll want to set it to zero for forward compatibility.)

Jan