Re: [PATCH v4] x86/cpufreq: Add Kconfig option for CPU frequency scaling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.02.2026 00:51, Stefano Stabellini wrote:
> Add CONFIG_CPUFREQ to allow CPU frequency scaling support to be
> disabled at build time. When disabled, this removes cpufreq code
> from the build.
> 
> Introduce IS_ENABLED(CONFIG_CPUFREQ) checks for relevant do_pm_op and
> do_platform_op subops and other functions that require CONFIG_CPUFREQ to
> work.
> 
> Add stubs where necessary.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> ---
> Changes in v4:
> - fix IS_ENABLED(CPUFREQ)
> - remove #ifdef in platform_hypercall.c and use DCE
> - move cpufreq_controller enum out of #ifdef

Where did the v3 revlog go? Especially when submit patches faster than people
can look at them, they might skip a version (or more), while still having
looked at a yet earlier one.

> --- a/xen/drivers/acpi/pm-op.c
> +++ b/xen/drivers/acpi/pm-op.c
> @@ -367,7 +367,8 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>          return ret;
>      }
>  
> -    if ( op->cpuid >= nr_cpu_ids || !cpu_online(op->cpuid) )
> +    if ( op->cpuid >= nr_cpu_ids || !cpu_online(op->cpuid) ||
> +         !IS_ENABLED(CONFIG_CPUFREQ) )

Have the compile time constant part first? Else any possible side effects
of the other expressions may prevent the compiler from fully dropping all
code here.

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -381,8 +381,19 @@ int write_ondemand_up_threshold(unsigned int 
> up_threshold);
>  
>  int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq);
>  
> +#ifdef CONFIG_CPUFREQ
> +int cpufreq_add_cpu(unsigned int cpu);
> +int cpufreq_del_cpu(unsigned int cpu);
> +
>  void cpufreq_dbs_timer_suspend(void);
>  void cpufreq_dbs_timer_resume(void);
> +#else
> +static inline int cpufreq_add_cpu(unsigned int cpu) { return -EOPNOTSUPP; }
> +static inline int cpufreq_del_cpu(unsigned int cpu) { return -EOPNOTSUPP; }

Returning an error here looks wrong, even if technically it is benign right now
(as the one [each] remaining call doesn't check the error code).

> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -186,7 +186,15 @@ static inline void acpi_set_csubstate_limit(unsigned int 
> new_limit) { return; }
>  #endif
>  
>  #ifdef XEN_GUEST_HANDLE
> +#ifdef CONFIG_CPUFREQ
>  int acpi_set_pdc_bits(unsigned int acpi_id, XEN_GUEST_HANDLE(uint32));
> +#else
> +static inline int acpi_set_pdc_bits(unsigned int acpi_id,
> +                                    XEN_GUEST_HANDLE(uint32) pdc)
> +{
> +    return -EOPNOTSUPP;

Here use of an error indicator would result in the XENPF_set_processor_pminfo
sub-op failing. That's not correct, as this is a notification from Dom0 to us.
If we can't make use of the provided data, we should silently ignore it.

> --- a/xen/include/xen/pmstat.h
> +++ b/xen/include/xen/pmstat.h
> @@ -5,10 +5,23 @@
>  #include <public/platform.h> /* for struct xen_processor_power */
>  #include <public/sysctl.h>   /* for struct pm_cx_stat */
>  
> +#ifdef CONFIG_CPUFREQ
>  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);
> +#else
> +static inline int set_px_pminfo(uint32_t acpi_id,
> +                                struct xen_processor_performance *perf)
> +{
> +    return -EOPNOTSUPP;
> +}
> +static inline int set_cppc_pminfo(unsigned int acpi_id,
> +                                  const struct xen_processor_cppc *cppc_data)
> +{
> +    return -EOPNOTSUPP;
> +}

Same here, I suppose.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1259,6 +1259,7 @@ extern enum cpufreq_controller {
>      FREQCTL_none, FREQCTL_dom0_kernel, FREQCTL_xen
>  } cpufreq_controller;

As previously indicated, these might better be invisible when CPUFREQ=n. But
see also below.

How about (name subject to improvement)

static inline is_hwdom_cpufreq_controller(void)
{
#ifdef CONFIG_CPUFREQ
    return cpufreq_controller == FREQCTL_dom0_kernel;
#else
    return false;
#endif
}

for use both ...

> +#ifdef CONFIG_CPUFREQ
>  static always_inline bool is_cpufreq_controller(const struct domain *d)
>  {
>      /*
> @@ -1274,6 +1275,12 @@ static always_inline bool is_cpufreq_controller(const 
> struct domain *d)
>      return (is_pv_domain(d) && is_hardware_domain(d) &&
>              cpufreq_controller == FREQCTL_dom0_kernel);

... here and in do_platform_op()? Or, keeping the enum visible,

static inline is_hwdom_cpufreq_controller(void)
{
    return IS_ENABLED(CONFIG_CPUFREQ) &&
           cpufreq_controller == FREQCTL_dom0_kernel;
}

Of course the possibly-compile-time-constant part of the expression would
then want to move first, to allow the fencing in is_pv_domain() to also be
dropped.

>  }
> +#else
> +static always_inline bool is_cpufreq_controller(const struct domain *d)
> +{
> +    return false;
> +}
> +#endif

Too much redundancy: The #ifdef would better be in the function body. But
with the other adjustment the need for an #ifdef would disappear here
anyway.

Jan