Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/09/15 03:32, Wei Wang wrote:
> By default, the old P-state driver (acpi-freq) is used. Adding
> "intel_pstate" to the Xen booting param list to enable the
> use of intel_pstate. However, if intel_pstate is enabled on a
> machine which does not support the driver (e.g. Nehalem), the
> old P-state driver will be loaded due to the failure loading of
> intel_pstate.
>
> Also, adding the intel_pstate booting parameter to
> xen-command-line.markdown.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  docs/misc/xen-command-line.markdown      | 7 +++++++
>  xen/arch/x86/acpi/cpufreq/cpufreq.c      | 9 ++++++---
>  xen/arch/x86/acpi/cpufreq/intel_pstate.c | 4 ++++
>  3 files changed, 17 insertions(+), 3 deletions(-)
>
>  changes in v5:
>  1) move the booting parameter into the intel_pstate_init() function - have
>     it be a local variable;
>  2) rename "intel_pstate_load" to "load".
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index a2e427c..2d70137 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -849,6 +849,13 @@ debug hypervisor only).
>  ### idle\_latency\_factor
>  > `= <integer>`
>  
> +### intel\_pstate
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Enable the loading of the intel pstate driver.
> +
>  ### ioapic\_ack
>  > `= old | new`
>  
> diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> index 8494fa0..7e517b9 100644
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -40,6 +40,7 @@
>  #include <asm/processor.h>
>  #include <asm/percpu.h>
>  #include <asm/cpufeature.h>
> +#include <asm/cpufreq.h>
>  #include <acpi/acpi.h>
>  #include <acpi/cpufreq/cpufreq.h>
>  
> @@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)
>      int ret = 0;
>  
>      if ((cpufreq_controller == FREQCTL_xen) &&
> -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
> -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> -    else if ((cpufreq_controller == FREQCTL_xen) &&
> +        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)) {
> +            ret = intel_pstate_init();
> +            if (ret)
> +                ret = cpufreq_register_driver(&acpi_cpufreq_driver);

This looks like a mess (although not your fault to start with).

We shouldn't have multiple different top level command line options.  In
particular, having "mwait-idle" and "intel_pstate" seems wrong, given a
perfectly good "cpufreq=" option.

Is the driver in use available to change at runtime from `xenpm` ?

> +    } else if ((cpufreq_controller == FREQCTL_xen) &&
>          (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
>          ret = powernow_register_driver();
>  
> diff --git a/xen/arch/x86/acpi/cpufreq/intel_pstate.c b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> index 3292bcd..4ebd9c7 100644
> --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> @@ -843,7 +843,11 @@ int __init intel_pstate_init(void)
>      int cpu, rc = 0;
>      const struct x86_cpu_id *id;
>      struct cpu_defaults *cpu_info;
> +    static bool_t __read_mostly load;

__initdata

~Andrew