On 01.05.2023 21:30, Jason Andryuk wrote:
> @@ -531,6 +533,100 @@ int get_hwp_para(const struct cpufreq_policy *policy,
> return 0;
> }
>
> +int set_hwp_para(struct cpufreq_policy *policy,
> + struct xen_set_hwp_para *set_hwp)
const?
> +{
> + unsigned int cpu = policy->cpu;
> + struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
> +
> + if ( data == NULL )
> + return -EINVAL;
> +
> + /* Validate all parameters first */
> + if ( set_hwp->set_params & ~XEN_SYSCTL_HWP_SET_PARAM_MASK )
> + return -EINVAL;
> +
> + if ( set_hwp->activity_window & ~XEN_SYSCTL_HWP_ACT_WINDOW_MASK )
> + return -EINVAL;
Below you limit checks to when the respective control bit is set. I
think you want the same here.
> + if ( !feature_hwp_energy_perf &&
> + (set_hwp->set_params & XEN_SYSCTL_HWP_SET_ENERGY_PERF) &&
> + set_hwp->energy_perf > IA32_ENERGY_BIAS_MAX_POWERSAVE )
> + return -EINVAL;
> +
> + if ( (set_hwp->set_params & XEN_SYSCTL_HWP_SET_DESIRED) &&
> + set_hwp->desired != 0 &&
> + (set_hwp->desired < data->hw.lowest ||
> + set_hwp->desired > data->hw.highest) )
> + return -EINVAL;
> +
> + /*
> + * minimum & maximum are not validated as hardware doesn't seem to care
> + * and the SDM says CPUs will clip internally.
> + */
> +
> + /* Apply presets */
> + switch ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_PRESET_MASK )
> + {
> + case XEN_SYSCTL_HWP_SET_PRESET_POWERSAVE:
> + data->minimum = data->hw.lowest;
> + data->maximum = data->hw.lowest;
> + data->activity_window = 0;
> + if ( feature_hwp_energy_perf )
> + data->energy_perf = HWP_ENERGY_PERF_MAX_POWERSAVE;
> + else
> + data->energy_perf = IA32_ENERGY_BIAS_MAX_POWERSAVE;
> + data->desired = 0;
> + break;
> +
> + case XEN_SYSCTL_HWP_SET_PRESET_PERFORMANCE:
> + data->minimum = data->hw.highest;
> + data->maximum = data->hw.highest;
> + data->activity_window = 0;
> + data->energy_perf = HWP_ENERGY_PERF_MAX_PERFORMANCE;
> + data->desired = 0;
> + break;
> +
> + case XEN_SYSCTL_HWP_SET_PRESET_BALANCE:
> + data->minimum = data->hw.lowest;
> + data->maximum = data->hw.highest;
> + data->activity_window = 0;
> + if ( feature_hwp_energy_perf )
> + data->energy_perf = HWP_ENERGY_PERF_BALANCE;
> + else
> + data->energy_perf = IA32_ENERGY_BIAS_BALANCE;
> + data->desired = 0;
> + break;
> +
> + case XEN_SYSCTL_HWP_SET_PRESET_NONE:
> + break;
> +
> + default:
> + return -EINVAL;
> + }
So presets set all the values for which the individual item control bits
are clear. That's not exactly what I would have expected, and it took me
reading the code several times until I realized that you write life per-
CPU data fields here, not fields of some intermediate variable. I think
this could do with saying explicitly in the public header (if indeed the
intended model).
> + /* Further customize presets if needed */
> + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_MINIMUM )
> + data->minimum = set_hwp->minimum;
> +
> + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_MAXIMUM )
> + data->maximum = set_hwp->maximum;
> +
> + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_ENERGY_PERF )
> + data->energy_perf = set_hwp->energy_perf;
> +
> + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_DESIRED )
> + data->desired = set_hwp->desired;
> +
> + if ( set_hwp->set_params & XEN_SYSCTL_HWP_SET_ACT_WINDOW )
> + data->activity_window = set_hwp->activity_window &
> + XEN_SYSCTL_HWP_ACT_WINDOW_MASK;
> +
> + hwp_cpufreq_target(policy, 0, 0);
> +
> + return 0;
I don't think you should assume here that hwp_cpufreq_target() will
only ever return 0. Plus by returning its return value here you
allow the compiler to tail-call optimize this code.
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -398,6 +398,20 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
> return ret;
> }
>
> +static int set_cpufreq_hwp(struct xen_sysctl_pm_op *op)
const?
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -317,6 +317,34 @@ struct xen_hwp_para {
> uint8_t energy_perf;
> };
>
> +/* set multiple values simultaneously when set_args bit is set */
What "set_args bit" does this comment refer to?
> +struct xen_set_hwp_para {
> +#define XEN_SYSCTL_HWP_SET_DESIRED (1U << 0)
> +#define XEN_SYSCTL_HWP_SET_ENERGY_PERF (1U << 1)
> +#define XEN_SYSCTL_HWP_SET_ACT_WINDOW (1U << 2)
> +#define XEN_SYSCTL_HWP_SET_MINIMUM (1U << 3)
> +#define XEN_SYSCTL_HWP_SET_MAXIMUM (1U << 4)
> +#define XEN_SYSCTL_HWP_SET_PRESET_MASK 0xf000
> +#define XEN_SYSCTL_HWP_SET_PRESET_NONE 0x0000
> +#define XEN_SYSCTL_HWP_SET_PRESET_BALANCE 0x1000
> +#define XEN_SYSCTL_HWP_SET_PRESET_POWERSAVE 0x2000
> +#define XEN_SYSCTL_HWP_SET_PRESET_PERFORMANCE 0x3000
> +#define XEN_SYSCTL_HWP_SET_PARAM_MASK ( \
> + XEN_SYSCTL_HWP_SET_PRESET_MASK | \
> + XEN_SYSCTL_HWP_SET_DESIRED | \
> + XEN_SYSCTL_HWP_SET_ENERGY_PERF | \
> + XEN_SYSCTL_HWP_SET_ACT_WINDOW | \
> + XEN_SYSCTL_HWP_SET_MINIMUM | \
> + XEN_SYSCTL_HWP_SET_MAXIMUM )
> + uint16_t set_params; /* bitflags for valid values */
> +#define XEN_SYSCTL_HWP_ACT_WINDOW_MASK 0x03ff
> + uint16_t activity_window; /* See comment in struct xen_hwp_para */
> + uint8_t minimum;
> + uint8_t maximum;
> + uint8_t desired;
> + uint8_t energy_perf; /* 0-255 or 0-15 depending on HW support */
Instead of (or in addition to) the "HW support" reference, could this
gain a reference to the "get para" bit determining which range to use?
Jan
