Skip to contentSkip to conversation

Porting the intel_pstate driver to Xen

PATCH In review Wei Wang 3 participants 14 messages Oct 28, 2015 – Nov 20, 2015

lore
Share & tools
lore

Wei Wang

author
#0 lore raw

v6 cover Porting the intel_pstate driver to Xen

v6 changes:
Changes include patch re-organization, data type change, coding style change etc. Please go to each patch for details.

v5 changes:
We have made various changes in this version, including introducing new data structures, coding styles changes etc. Please see each patch's commit message for change details.

v4 changes:
1) introduce a new struct, internal_governor, to "cpufreq_policy";
2) add a new header file, xen/include/asm-x86/cpufreq.h;
3) remove the APERF/MPERF feature detection code in cpufreq.c and powernow.c;
4) coding style changes.

Please check each patch's commit message for details.

v3 Changes:
1) coding style changes based on Jan's comments;
2) remove the function - unregister_cpu_notifier();
3) solve a bug in the CPU offline code (Patch 0007);
4) move the perf_limits struct into the per-CPU policy struct, so that each CPU can be managed individually;
5) "load_intel_pstate" is changed local to the intel_pstate.c file, and add its description to the xen-command-line.markdown.

v2 Changes:
1) The intel_pstate driver can be controlled via two ways:
A. min_perf_pct and max_perf_pct
The user directly adjusts min_perf_pct and max_perf_pct to get what they want. For example, if min_perf_pct=max_perf_pct=60%, then the user is asking for something similar to a userspace governor with setting the requested performance=60%.
B. set-scaling-governor
This one is functionally redundant, since A. can achieve all the governor functions. It is remained to give people time to get familiar with method A.
Users can choose from the four governors: Powersave, Ondemand, Powersave, Performance. The driver achieves the functionality of the selected governor via adjusting the min_perf_pct and max_perf_pct itself.
2) The xenpm "get-cpufreq-para" displays the following things:
cpu id : 10
affected_cpus : 10
cpuinfo frequency : max [3700000] min [1200000] cur [1400000]
scaling_driver : intel_pstate
scaling_avail_gov : performance powersave userspace ondemand
current_governor : ondemand
max_perf_pct : 100
min_perf_pct : 32
turbo_pct : 54
turbo mode : enabled
3) Changed "intel_pstate=disable" to "intel_pstate=enable".
If "intel_pstate=enable" is added, but the CPU does not support the intel_pstate driver, the old P-state driver (acpi-cpufreq) will be loaded.
4) Moved the declarations under xen/include/acpi to an x86-specific header.

v1:
This patch series ports the intel_pstate driver from the Linux kernel to Xen. The intel_pstate driver is used to tune P states for SandyBridge+ processors. It needs to be enabled by adding "intel_pstate=enable" to the booting parameter list.

The intel_pstate.c file under xen/arch/x86/acpi/cpufreq/ contains all the logic for selecting the current P-state. It follows its implementation in the kernel. In order to better support future Intel CPUs (e.g. the HWP feature on Skylake+), intel_pstate changes to tune P-state based on percentage values.

The xenpm tool is also upgraded to support the intel_pstate driver. If intel_pstate is used, "get-cpufreq-para" displays percentage value based feedback. If the intel_pstate driver is not enabled, xenpm will work in the old style.

Wei Wang (6):
x86/intel_pstate: add some calculation related support x86/intel_pstate: introduce the internal_governor struct x86/intel_pstate: the main body of the intel_pstate driver x86/intel_pstate: add a booting param to select the driver to load x86/intel_pstate: support the use of intel_pstate in pmstat.c
tools: enable xenpm to control the intel_pstate driver

 docs/misc/xen-command-line.markdown      |   7 +
 tools/libxc/include/xenctrl.h            |  20 +-
 tools/libxc/xc_pm.c                      |  16 +-
 tools/misc/xenpm.c                       | 116 +++-
 xen/arch/x86/acpi/cpufreq/Makefile       |   1 +
 xen/arch/x86/acpi/cpufreq/cpufreq.c      |  15 +-
 xen/arch/x86/acpi/cpufreq/intel_pstate.c | 882 +++++++++++++++++++++++++++++++
 xen/arch/x86/oprofile/op_model_athlon.c  |   9 -
 xen/drivers/acpi/pmstat.c                | 183 ++++++-
 xen/drivers/cpufreq/cpufreq.c            |   5 +-
 xen/include/acpi/cpufreq/cpufreq.h       |  15 +
 xen/include/asm-x86/cpufreq.h            |  33 ++
 xen/include/asm-x86/div64.h              |  91 ++++
 xen/include/asm-x86/msr-index.h          |   3 +
 xen/include/public/sysctl.h              |  29 +-
 xen/include/xen/kernel.h                 |  23 +
 16 files changed, 1356 insertions(+), 92 deletions(-)
 create mode 100644 xen/arch/x86/acpi/cpufreq/intel_pstate.c
 create mode 100644 xen/include/asm-x86/cpufreq.h
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v6 1/6 x86/intel_pstate: add some calculation related support

The added calculation related functions will be used in the intel_pstate.c. They are copied from the Linux kernel(commit 2418f4f2, f3002134, eb18cba7).

---
changes in v6:
1) #define clamp() - remove the typecast on the result of max();
2) add some comments to explain the functionality of some functions;
3) alignment changes.

 xen/arch/x86/oprofile/op_model_athlon.c |  9 ----
 xen/include/asm-x86/div64.h             | 91 +++++++++++++++++++++++++++++++++
 xen/include/xen/kernel.h                | 23 +++++++++
 3 files changed, 114 insertions(+), 9 deletions(-)
xen/arch/x86/oprofile/op_model_athlon.c +0 -9
index c0a81ed..4122eee 100644
@@ -103,15 +103,6 @@ static u64 ibs_op_ctl;
 #define IBS_FETCH_CODE                  13
 #define IBS_OP_CODE                     14
 
-#define clamp(val, min, max) ({			\
-	typeof(val) __val = (val);		\
-	typeof(min) __min = (min);		\
-	typeof(max) __max = (max);		\
-	(void) (&__val == &__min);		\
-	(void) (&__val == &__max);		\
-	__val = __val < __min ? __min: __val;	\
-	__val > __max ? __max: __val; })
-
 /*
  * 16-bit Linear Feedback Shift Register (LFSR)
  */
xen/include/asm-x86/div64.h +91 -0

95 lines not on this page · raw message

xen/include/xen/kernel.h +23 -0
index 548b64d..96c7948 100644
@@ -43,6 +43,29 @@
 #define MAX(x,y) ((x) > (y) ? (x) : (y))
 
 /**
+ * clamp - return a value clamped to a given range with strict typechecking
+ * @val: current value
+ * @lo: lowest allowable value
+ * @hi: highest allowable value
+ *
+ * This macro does strict typechecking of lo/hi to make sure they are of the
+ * same type as val.  See the unnecessary pointer comparisons.
+ */
+#define clamp(val, lo, hi) min(max(val, lo), hi)
+
+/*
+ * clamp_t - return a value clamped to a given range using a given type
+ * @type: the type of variable to use
+ * @val: current value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
+ *
+ * This macro does no typechecking and uses temporary variables of type
+ * 'type' to make all the comparisons.
+ */
+#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
+
+/**
  * container_of - cast a member of a structure out to the containing structure
  *
  * @ptr:	the pointer to the member.
Signature · 1 line
1.9.1
#2 lore raw

Re: [PATCH v6 1/6] x86/intel_pstate: add some calculation related support

xen/include/asm-x86/div64.h

Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
xen/include/asm-x86/div64.h +0 -0

Considering you don't even do this in common code, why did you pick the BITS_PER_LONG == 32 variants from Linux? Would make for quite a bit smaller a patch and quite a bit better easier to read code.

Jan

author
#3 lore raw

PATCH v6 2/6 x86/intel_pstate: introduce the internal_governor struct

In reply to Wei Wang

Introduce a simple internal_governor struct to manage internal governor related variables. Also, add a condition check in cpufreq_del_cpu to avoid going through the old ACPI governor framework when an internal governor is in use.

---
changes in v6:
1) create this patch by spliting it from the next big one.

 xen/drivers/cpufreq/cpufreq.c      | 5 +++--
 xen/include/acpi/cpufreq/cpufreq.h | 7 +++++++
 2 files changed, 10 insertions(+), 2 deletions(-)
xen/drivers/cpufreq/cpufreq.c +3 -2
index 6e666e4..2c1c713 100644
@@ -334,8 +334,9 @@ int cpufreq_del_cpu(unsigned int cpu)
 
     /* for HW_ALL, stop gov for each core of the _PSD domain */
     /* for SW_ALL & SW_ANY, stop gov for the 1st core of the _PSD domain */
-    if (hw_all || (cpumask_weight(cpufreq_dom->map) ==
-                   perf->domain_info.num_processors))
+    if (!policy->internal_gov &&
+        (hw_all || (cpumask_weight(cpufreq_dom->map) ==
+                    perf->domain_info.num_processors)))
         __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 
     cpufreq_statistic_exit(cpu);
xen/include/acpi/cpufreq/cpufreq.h +7 -0
index 48ad1d0..8947368 100644
@@ -53,6 +53,12 @@ struct perf_limits {
     uint32_t min_policy_pct;
 };
 
+struct internal_governor {
+    char *avail_gov;
+    uint32_t gov_num;
+    uint32_t cur_gov;
+};
+
 struct cpufreq_policy {
     cpumask_var_t       cpus;          /* affected CPUs */
     unsigned int        shared_type;   /* ANY or ALL affected CPUs
@@ -66,6 +72,7 @@ struct cpufreq_policy {
                                  * governors are used */
     struct perf_limits  limits;
     struct cpufreq_governor     *governor;
+    struct internal_governor    *internal_gov;
 
     bool_t              resume; /* flag for cpufreq 1st run
                                  * S3 wakeup, hotplug cpu, etc */
Signature · 1 line
1.9.1
#4 lore raw

Re: [PATCH v6 2/6] x86/intel_pstate: introduce the internal_governor struct

xen/drivers/cpufreq/cpufreq.c int cpufreq_del_cpu(unsigned int cpu) · line 340

Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
xen/drivers/cpufreq/cpufreq.c +3 -2
@@ -334,8 +334,9 @@ int cpufreq_del_cpu(unsigned int cpu)
 
     /* for HW_ALL, stop gov for each core of the _PSD domain */
     /* for SW_ALL & SW_ANY, stop gov for the 1st core of the _PSD domain */
-    if (hw_all || (cpumask_weight(cpufreq_dom->map) ==
-                   perf->domain_info.num_processors))
+    if (!policy->internal_gov &&
+        (hw_all || (cpumask_weight(cpufreq_dom->map) ==
+                    perf->domain_info.num_processors)))
         __cpufreq_governor(policy, CPUFREQ_GOV_STOP);

Why do we need such a guard here, but not on any of the other __cpufreq_governor() invocations? (If they will end up unreachable by the following patches, then I'd still expect one or more ASSERT()s to be added to document/enforce this.)

xen/include/acpi/cpufreq/cpufreq.h struct perf_limits { · line 60

Quoted 11 lines
xen/include/acpi/cpufreq/cpufreq.h +5 -0
@@ -53,6 +53,12 @@ struct perf_limits {
     uint32_t min_policy_pct;
 };
 
+struct internal_governor {
+    char *avail_gov;
+    uint32_t gov_num;
+    uint32_t cur_gov;
+};

Apart from the fields being meaningless without user I don't think the gov_ prefix and the _gov suffixes are really helpful here. Also "num" isn't really unambiguous - if you mean "count", please use "count". If you mean something else, please pick an unambiguous field name.

Jan

author
#5 lore raw

PATCH v6 3/6 x86/intel_pstate: the main body of the intel_pstate driver

In reply to Wei Wang

We simply grab the fundamental logic of the intel_pstate driver from Linux kernel, and customize it to Xen style. In the kernel, a user can adjust the limits via sysfs (limits.min_sysfs_pct/max_sysfs_pct). In Xen, the policy->limits.min_perf_pct/max_perf_pct acts as the transit station. A user interacts with it via xenpm.

The new xen/include/asm-x86/cpufreq.h header file is added.

---
changes in v6:
1) change some of the unnecessary signed types to be unsigned, as requested by Jan in v2;
2) remove "__ready_mostly" from the local variable, load, in intel_pstate_init();
3) coding style changes.

 xen/arch/x86/acpi/cpufreq/Makefile       |   1 +
 xen/arch/x86/acpi/cpufreq/intel_pstate.c | 882 +++++++++++++++++++++++++++++++
 xen/include/acpi/cpufreq/cpufreq.h       |   6 +
 xen/include/asm-x86/cpufreq.h            |  31 ++
 xen/include/asm-x86/msr-index.h          |   3 +
 5 files changed, 923 insertions(+)
 create mode 100644 xen/arch/x86/acpi/cpufreq/intel_pstate.c
 create mode 100644 xen/include/asm-x86/cpufreq.h
xen/arch/x86/acpi/cpufreq/Makefile +1 -0
index f75da9b..99fa9f4 100644
@@ -1,2 +1,3 @@
 obj-y += cpufreq.o
+obj-y += intel_pstate.o
 obj-y += powernow.o
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +882 -0

882 lines not on this page · raw message

xen/include/acpi/cpufreq/cpufreq.h +6 -0
index 8947368..ef6643a 100644
@@ -107,6 +107,12 @@ struct cpufreq_freqs {
  *                          CPUFREQ GOVERNORS                        *
  *********************************************************************/
 
+/* Please add internal governors here */
+#define INTERNAL_GOV_POWERSAVE        (1)
+#define INTERNAL_GOV_PERFORMANCE      (2)
+#define INTERNAL_GOV_USERSPACE        (3)
+#define INTERNAL_GOV_ONDEMAND         (4)
+
 #define CPUFREQ_GOV_START  1
 #define CPUFREQ_GOV_STOP   2
 #define CPUFREQ_GOV_LIMITS 3
xen/include/asm-x86/cpufreq.h new file +31 -0
new file mode 100644
index 0000000..afc72df
@@ -0,0 +1,31 @@
+#ifndef _ASM_X86_CPUFREQ_H
+#define _ASM_X86_CPUFREQ_H
+
+/*
+ *  Copyright (C) 2015, Intel Corporation.
+ *  Wei Wang <[email protected]>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+/*
+ * Maximum transition latency is in nanoseconds - if it's unknown,
+ * CPUFREQ_ETERNAL shall be used.
+ */
+#define CPUFREQ_ETERNAL        (-1)
+
+#endif /* _ASM_X86_CPUFREQ_H */
xen/include/asm-x86/msr-index.h +3 -0
index 65c1d02..b9c4595 100644
@@ -52,6 +52,8 @@
 #define MSR_IA32_MCG_STATUS		0x0000017a
 #define MSR_IA32_MCG_CTL		0x0000017b
 
+#define MSR_NHM_TURBO_RATIO_LIMIT	0x000001ad
+
 #define MSR_IA32_PEBS_ENABLE		0x000003f1
 #define MSR_IA32_DS_AREA		0x00000600
 #define MSR_IA32_PERF_CAPABILITIES	0x00000345
@@ -320,6 +322,7 @@
 #define MSR_IA32_MISC_ENABLE_MONITOR_ENABLE (1<<18)
 #define MSR_IA32_MISC_ENABLE_LIMIT_CPUID  (1<<22)
 #define MSR_IA32_MISC_ENABLE_XTPR_DISABLE (1<<23)
+#define MSR_IA32_MISC_ENABLE_TURBO_DISABLE (1ULL<<38)
 
 #define MSR_IA32_TSC_DEADLINE		0x000006E0
 #define MSR_IA32_ENERGY_PERF_BIAS	0x000001b0
Signature · 1 line
1.9.1
#6 lore raw

Re: [PATCH v6 3/6] x86/intel_pstate: the main body of the intel_pstate driver

Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7 -0
@@ -0,0 +1,882 @@
+#include <xen/kernel.h>
+#include <xen/types.h>
+#include <xen/init.h>
+#include <xen/bitmap.h>
+#include <xen/cpumask.h>
+#include <xen/timer.h>
+#include <asm/msr.h>

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 17

Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10 -0
+#include <asm/msr-index.h>
+#include <asm/processor.h>
+#include <asm/div64.h>
+#include <asm/cpufreq.h>
+#include <acpi/cpufreq/cpufreq.h>
+
+#define BYT_RATIOS       0x66a
+#define BYT_VIDS         0x66b
+#define BYT_TURBO_RATIOS 0x66c
+#define BYT_TURBO_VIDS   0x66d

Considering these are MSR names, the acronym MSR should appear in them.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 25

Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7 -0
+#define FRAC_BITS 8
+#define int_tofp(X) ((uint64_t)(X) << FRAC_BITS)
+#define fp_toint(X) ((X) >> FRAC_BITS)
+
+static inline uint32_t mul_fp(uint32_t x, uint32_t y)
+{
+    return ((uint64_t)x * (uint64_t)y) >> FRAC_BITS;

Casting just one side of the * would suffice. Also please don't open-code fp_to_int() ...

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 30

Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3 -0
+static inline uint32_t div_fp(uint32_t x, uint32_t y)
+{
+    return div_s64((uint64_t)x << FRAC_BITS, y);

... or int_to_fp() (for this one it would be a different thing if you removed the - apparently pointless - cast from the macro).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 42

Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10 -0
+static inline uint32_t ceiling_fp(uint32_t x)
+{
+    uint32_t mask, ret;
+
+    ret = fp_toint(x);
+    mask = (1 << FRAC_BITS) - 1;
+    if ( x & mask )
+        ret += 1;
+    return ret;
+}

Blank line before final return statement please.

Code further down suggests there may be a sign in these fixed point numbers, yet here you converted _everything_ to uintNN_t. Did you go too far? What sense does e.g. div_s64() on a uint64_t make?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 78

Quoted 2 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +2 -0
+struct cpudata {
+    int cpu;

unsigned int (but I wonder if this is really needed, see the comment at the allocation site)

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 153

Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8 -0
+static signed int pid_calc(struct _pid *pid, uint32_t busy)
+{
+    signed int result;
+    int32_t pterm, dterm, fp_error;
+    int32_t integral_limit;
+
+    fp_error = int_tofp(pid->setpoint) - busy;
+    if ( ABS(fp_error) <= int_tofp(pid->deadband) )

int_tofp() currently returning an unsigned value - why ABS()?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 192

Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3 -0
+static inline void intel_pstate_reset_all_pid(void)
+{
+    uint32_t cpu;

unsigned int

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 210

Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10 -0
+static inline void update_turbo_state(struct cpufreq_policy *policy)
+{
+    uint64_t misc_en;
+    struct cpudata *cpu;
+
+    cpu = all_cpu_data[policy->cpu];
+    rdmsrl(MSR_IA32_MISC_ENABLE, misc_en);
+    policy->limits.turbo_disabled =
+        (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE ||
+            cpu->pstate.max_pstate == cpu->pstate.turbo_pstate);

At least the & needs parenthesization.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 248

Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8 -0
+static void byt_set_pstate(struct perf_limits *limits,
+                struct cpudata *cpudata, uint32_t pstate)
+{
+    uint64_t val;
+    uint32_t vid_fp;
+    uint32_t vid;
+
+    val = pstate << 8;

Didn't we agree not to have any unnamed literals?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 250

Quoted 2 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +2 -0
+    if ( limits->no_turbo && !limits->turbo_disabled )
+        val |= (uint64_t)1 << BYT_TURBO_CONTROL_BIT;

Perhaps the sense of that bit isn't what one would imply at first glance, but the combination of conditions in the if() looks certainly odd. Please confirm this is really intended to be this way (which should also include pointing out what meaning the bit being on has).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 260

Quoted 9 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +9 -0
+    vid_fp = cpudata->vid.min + mul_fp(
+        int_tofp(pstate - cpudata->pstate.min_pstate),
+        cpudata->vid.ratio);
+
+    vid_fp = clamp(vid_fp, cpudata->vid.min, cpudata->vid.max);
+    vid = ceiling_fp(vid_fp);
+
+    if ( pstate > cpudata->pstate.max_pstate )
+        vid = cpudata->vid.turbo;

The prior calculation of vid (and vid_fp) is completely pointless in this case. Also - what if limits->no_turbo or limits->turbo_disabled?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 269

Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8 -0
+    val |= vid;
+
+    wrmsrl(MSR_IA32_PERF_CTL, val);
+}
+
+#define BYT_BCLK_FREQS 5
+#define TO_FREQ_TABLE_IDX_MASK 0x7
+static uint32_t byt_get_scaling(void)

Missing blank line.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 89

Quoted 2 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +2 -0
+{
+    const uint32_t byt_freq_table[BYT_BCLK_FREQS] =

static

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 277

Quoted 6 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +6 -0
+                   {833, 1000, 1333, 1167, 800};
+    uint64_t value;
+    int i;
+
+    rdmsrl(MSR_FSB_FREQ, value);
+    i = value & TO_FREQ_TABLE_IDX_MASK;

With this calculation i is hardly a signed quantity.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 309

Quoted 9 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +9 -0
+#define CORE_MIN_PSTATE(val) (((value) >> 40) & 0xff)
+#define CORE_MAX_PSTATE(val) (((value) >> 8) & 0xff)
+#define CORE_TURBO_PSTATE(value) ((value) & 0xff)
+static uint32_t core_get_min_pstate(void)
+{
+    uint64_t value;
+
+    rdmsrl(MSR_INTEL_PLATFORM_INFO, value);
+    return CORE_MIN_PSTATE(val);

The fact that (presumably) don't get a build error here is because the mix of variable names here happens to invert the mix of names in the macros ahead of the function.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 350

Quoted 13 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +13 -0
+static void core_set_pstate(struct perf_limits *limits,
+                            struct cpudata *cpudata, uint32_t pstate)
+{
+    uint64_t val;
+
+    val = pstate << 8;
+    if ( limits->no_turbo && !limits->turbo_disabled )
+        val |= (uint64_t)1 << CORE_TURBO_CONTROL_BIT;
+
+    wrmsrl(MSR_IA32_PERF_CTL, val);
+}
+
+static __initconst struct cpu_defaults core_params = {

static const struct cpu_defaults __initconst core_params = {

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 461

Quoted 12 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +12 -0
+static inline void intel_pstate_calc_busy(struct cpudata *cpu)
+{
+    struct sample *sample = &cpu->sample;
+    uint64_t core_pct;
+
+    core_pct = int_tofp(sample->aperf) * int_tofp(100);
+    core_pct = div64_u64(core_pct, int_tofp(sample->mperf));
+
+    sample->freq = fp_toint(mul_fp(int_tofp(cpu->pstate.max_pstate *
+                                   cpu->pstate.scaling / 100), core_pct));
+
+    sample->core_pct_busy = (int32_t)core_pct;

Pointless cast.

Quoted 6 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +6 -0
+static inline void intel_pstate_sample(struct cpudata *cpu)
+{
+    uint64_t aperf, mperf;
+    unsigned long flags;
+
+    local_irq_save(flags);

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 479

Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10 -0
+    rdmsrl(MSR_IA32_APERF, aperf);
+    rdmsrl(MSR_IA32_MPERF, mperf);
+    local_irq_restore(flags);
+
+    cpu->last_sample_time = cpu->sample.time;
+    cpu->sample.time = get_s_time();
+    cpu->sample.aperf = aperf;
+    cpu->sample.mperf = mperf;
+    cpu->sample.aperf -= cpu->prev_aperf;
+    cpu->sample.mperf -= cpu->prev_mperf;

Can these two please be done in a single statement each?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 484

Quoted 4 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +4 -0
+    intel_pstate_calc_busy(cpu);
+
+    cpu->prev_aperf = aperf;
+    cpu->prev_mperf = mperf;

And these two be moved up (intel_pstate_calc_busy() doesn't appear to be using the updated fields)?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 552

Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3 -0
+static void intel_pstate_timer_func(void *data)
+{
+    struct cpudata *cpu = (struct cpudata *) data;

Pointless cast.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 680

Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3 -0
+static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)
+{
+    int cpu_num = policy->cpu;

unsigned int

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 685

Quoted 5 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +5 -0
+    struct cpudata *cpu = all_cpu_data[cpu_num];
+
+    kill_timer(&all_cpu_data[cpu_num]->timer);
+
+    intel_pstate_set_pstate(policy, cpu, cpu->pstate.min_pstate);

Why min and not max? Or whatever the CPU was in when the governor initialized the CPU?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 702

Quoted 11 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +11 -0
+static int intel_pstate_turbo_update(int cpuid, struct cpufreq_policy *policy)
+{
+    struct cpudata *cpu = all_cpu_data[policy->cpu];
+    struct perf_limits *limits = &policy->limits;
+
+    update_turbo_state(policy);
+    if ( limits->turbo_disabled )
+    {
+        printk("Turbo disabled by BIOS or not supported on CPU\n");
+        return -EINVAL;
+    }

I'm afraid this message may get printed more often than you'd want (it should be printed at most once during any session).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 703

Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1 -0
+    limits->no_turbo = policy->turbo == CPUFREQ_TURBO_ENABLED ? 0 : 1;

limits->no_turbo = policy->turbo != CPUFREQ_TURBO_ENABLED;

Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8 -0
+#define INTEL_PSTATE_GOV_NUM 4
+static struct internal_governor* intel_pstate_internal_gov_init(void)
+{
+    unsigned int i = 0;
+    struct internal_governor *gov;
+    char *avail_gov;
+
+    gov = xzalloc(struct internal_governor);

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 747

Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10 -0
+    if ( !gov )
+        return NULL;
+    avail_gov = xzalloc_array(char,
+            INTEL_PSTATE_GOV_NUM * CPUFREQ_NAME_LEN);
+    if ( !avail_gov )
+        return NULL;
+
+    gov->avail_gov = avail_gov;
+
+    i += scnprintf(&avail_gov[0], CPUFREQ_NAME_LEN, "%s ", "performance");

i = ... (and the initializer dropped above).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 751

Quoted 4 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +4 -0
+    i += scnprintf(&avail_gov[i], CPUFREQ_NAME_LEN, "%s ", "powersave");
+    i += scnprintf(&avail_gov[i], CPUFREQ_NAME_LEN, "%s ", "userspace");
+    i += scnprintf(&avail_gov[i], CPUFREQ_NAME_LEN, "%s ", "ondemand");
+    avail_gov[i-1] = '\0';

Coding style.

But overall I wonder whether this couldn't be done in a less convoluted way (e.g. a single strlcpy(), or a strlcpy() followed by three strlcat()s). In no case is the indirection via %s warranted.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 753

Quoted 2 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +2 -0
+    gov->gov_num = INTEL_PSTATE_GOV_NUM;
+    gov->cur_gov = INTERNAL_GOV_ONDEMAND;

Shouldn't this depend on the "cpufreq=" option setting (if any)?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 763

Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7 -0
+static int intel_pstate_cpu_setup(struct cpufreq_policy *policy)
+{
+    struct cpudata *cpu;
+    struct perf_limits *limits = &policy->limits;
+    int rc;
+
+    rc = intel_pstate_init_cpu(policy->cpu);

Please use reasonably consistent naming (compare the name of the function we're in and that of the called function).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 773

Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10 -0
+    if ( rc )
+        return rc;
+
+    policy->internal_gov = intel_pstate_internal_gov_init();
+    if ( !policy->internal_gov )
+        return -ENOMEM;
+
+    cpu = all_cpu_data[policy->cpu];
+    policy->min = cpu->pstate.min_pstate * cpu->pstate.scaling;
+    policy->max = cpu->pstate.turbo_pstate * cpu->pstate.scaling;

DYM .max_pstate here?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 779

Quoted 5 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +5 -0
+    /* cpuinfo and default policy values */
+    policy->cpuinfo.min_freq = cpu->pstate.min_pstate *
+                               cpu->pstate.scaling;
+    policy->cpuinfo.max_freq = cpu->pstate.turbo_pstate *
+                               cpu->pstate.scaling;

Same here? Or if intentional, then I think a comment is warranted, and policy->turbo would then also seem to need initializing to 1 (that actually might eliminate the need for a separate comment).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 793

Quoted 6 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +6 -0
+    limits->min_policy_pct = clamp_t(uint32_t,
+                                     limits->min_policy_pct, 0, 100);
+    limits->max_policy_pct = (policy->max * 100) /
+                             policy->cpuinfo.max_freq;
+    limits->max_policy_pct = clamp_t(uint32_t,
+                                     limits->max_policy_pct, 0, 100);

These appear to be the only two uses of clamp_t(), which can be easily made just clamp(). Hence I don't see the need for introducing clamp_t() in the first place.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 819

Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1 -0
+static void __init copy_pid_params(struct pstate_adjust_policy *policy)

const

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 826

Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7 -0
+{
+    pid_params.sample_rate_ms = policy->sample_rate_ms;
+    pid_params.p_gain_pct = policy->p_gain_pct;
+    pid_params.i_gain_pct = policy->i_gain_pct;
+    pid_params.d_gain_pct = policy->d_gain_pct;
+    pid_params.deadband = policy->deadband;
+    pid_params.setpoint = policy->setpoint;

Why not just

    pid_params = *policy;

?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 829

Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1 -0
+static void __init copy_cpu_funcs(struct pstate_funcs *funcs)

const

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 836

Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7 -0
+{
+    pstate_funcs.get_max   = funcs->get_max;
+    pstate_funcs.get_min   = funcs->get_min;
+    pstate_funcs.get_turbo = funcs->get_turbo;
+    pstate_funcs.get_scaling = funcs->get_scaling;
+    pstate_funcs.set       = funcs->set;
+    pstate_funcs.get_vid   = funcs->get_vid;

And again

    pstate_funcs = *funcs;

?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 841

Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3 -0
+int __init intel_pstate_init(void)
+{
+    int cpu, rc = 0;

unsigned int cpu;
int rc;

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 844

Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3 -0
+    const struct x86_cpu_id *id;
+    struct cpu_defaults *cpu_info;
+    static bool_t load;

__initdata (and I'm sure I said so before).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 854

Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10 -0
+    boolean_param("intel_pstate", load);
+
+    if ( !load )
+        return -ENODEV;
+
+    id = x86_match_cpu(intel_pstate_cpu_ids);
+    if ( !id )
+        return -ENODEV;
+
+    cpu_info = (struct cpu_defaults *)id->driver_data;

This casts away const-ness, which is why casts should be avoided wherever possible (and the more when they're pointless like this one).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 860

Quoted 5 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +5 -0
+    copy_pid_params(&cpu_info->pid_policy);
+    copy_cpu_funcs(&cpu_info->funcs);
+
+    if ( intel_pstate_msrs_not_valid() )
+        return -ENODEV;

This reads as if the function returned a boolean. Either make it so (and the perhaps invert the sense and drop the "not" from the name) or use the function's return value (in which the function should perhaps be named ..._validate or some such).

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 862

Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1 -0
+    all_cpu_data = xzalloc_array(struct cpudata *, NR_CPUS);

Why NR_CPUS? Can't this be a per-CPU item anyway?

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 870

Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8 -0
+    if ( !all_cpu_data )
+        return -ENOMEM;
+
+    rc = cpufreq_register_driver(&intel_pstate_driver);
+    if ( rc )
+        goto out;
+
+    return rc;

Looks like this could be "return 0", albeit ...

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 871

Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1 -0
+out:

... just a single path leading here (and the comment below taken into account) just doing the cleanup in the if() body above would seem more reasonable. Otherwise - labels indented by at least one space please.

xen/arch/x86/acpi/cpufreq/intel_pstate.c line 879

Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8 -0
+    for_each_online_cpu(cpu)
+    {
+        if ( all_cpu_data[cpu] )
+        {
+            kill_timer(&all_cpu_data[cpu]->timer);
+            xfree(all_cpu_data[cpu]);
+        }
+    }

Why is all of this needed? If cpufreq_register_driver() (or one of its descendants) fails, it should clean up after itself.

Quoted 22 lines
xen/include/asm-x86/cpufreq.h new file +19 -0
@@ -0,0 +1,31 @@
+#ifndef _ASM_X86_CPUFREQ_H
+#define _ASM_X86_CPUFREQ_H
+
+/*
+ *  Copyright (C) 2015, Intel Corporation.
+ *  Wei Wang <[email protected]>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License

xen/include/asm-x86/cpufreq.h line 29

Quoted 10 lines
xen/include/asm-x86/cpufreq.h new file +10 -0
+ *  along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+/*
+ * Maximum transition latency is in nanoseconds - if it's unknown,
+ * CPUFREQ_ETERNAL shall be used.
+ */
+#define CPUFREQ_ETERNAL        (-1)

So what is x86-specific about this #define? I.e. why can't this go into an existing header?

Jan

author
#7 lore raw

PATCH v6 4/6 x86/intel_pstate: add a booting param to select the driver to load

In reply to Wei Wang

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.

---
changes in v6:
1) move the declaration of intel_pstate_init() to this patch.

 docs/misc/xen-command-line.markdown |  7 +++++++
 xen/arch/x86/acpi/cpufreq/cpufreq.c | 15 ++++++++++-----
 xen/include/asm-x86/cpufreq.h       |  2 ++
 3 files changed, 19 insertions(+), 5 deletions(-)
docs/misc/xen-command-line.markdown +7 -0
index 416e559..e57a23a 100644
@@ -858,6 +858,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`
 
xen/arch/x86/acpi/cpufreq/cpufreq.c +10 -5
index a2ba0db..d59f251 100644
@@ -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,11 +648,15 @@ 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_AMD))
+    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);
+    }
+    else if ( (cpufreq_controller == FREQCTL_xen) &&
+              (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
         ret = powernow_register_driver();
 
     return ret;
xen/include/asm-x86/cpufreq.h +2 -0
index afc72df..3ff516d 100644
@@ -22,6 +22,8 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
+extern int intel_pstate_init(void);
+
 /*
  * Maximum transition latency is in nanoseconds - if it's unknown,
  * CPUFREQ_ETERNAL shall be used.
Signature · 1 line
1.9.1
Acked-by on v6 4/6
#8 lore raw

Re: [PATCH v6 4/6] x86/intel_pstate: add a booting param to select the driver to load

Quoted 2 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:

with two remarks:

docs/misc/xen-command-line.markdown debug hypervisor only). · line 866

Quoted 12 lines
docs/misc/xen-command-line.markdown +6 -0
@@ -858,6 +858,13 @@ debug hypervisor only).
 ### idle\_latency\_factor
 > `= <integer>`
 
+### intel\_pstate
+> `= <boolean>`
+
+> Default: `false`
+
+Enable the loading of the intel pstate driver.

"Loading" is probably not the right term, as it gets always loaded. Perhaps just drop "the loading of"?

xen/include/asm-x86/cpufreq.h line 25

Quoted 7 lines
xen/include/asm-x86/cpufreq.h +1 -0
@@ -22,6 +22,8 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
+extern int intel_pstate_init(void);

Considering that I questioned the creation of this header in the previous patch, the addition of this declaration should not result in it now being created here. In fact if any new header was needed for this declaration, it would probably better live in xen/arch/x86/acpi/cpufreq/.

Moving this to a _suitable_ existing header does not void the ack given.

Jan

author
#9 lore raw

PATCH v6 5/6 x86/intel_pstate: support the use of intel_pstate in pmstat.c

In reply to Wei Wang

Add support in the pmstat.c so that the xenpm tool can request to access the intel_pstate driver.

---
changes in v6:
1) add the NON_INTERNAL_GOV macro to replace literal 0;
2) code consolidation (e.g. merging some code into if/else, as required in v5);
3) somewhere, change to use clamp, instead of clamp_t;
4) xen_perf_alias, instead of perf_alias.

 tools/libxc/include/xenctrl.h      |  20 ++--
 tools/libxc/xc_pm.c                |  16 ++--
 tools/misc/xenpm.c                 |   4 +-
 xen/drivers/acpi/pmstat.c          | 183 +++++++++++++++++++++++++++++++------
 xen/include/acpi/cpufreq/cpufreq.h |   2 +
 xen/include/public/sysctl.h        |  29 ++++--
 6 files changed, 198 insertions(+), 56 deletions(-)
tools/libxc/include/xenctrl.h +11 -9
index 3bfa00b..590eb72 100644
@@ -2270,6 +2270,17 @@ struct xc_get_cpufreq_para {
     uint32_t cpu_num;
     uint32_t freq_num;
     uint32_t gov_num;
+    int32_t turbo_enabled;
+
+    uint32_t cpuinfo_cur_freq;
+    uint32_t cpuinfo_max_freq;
+    uint32_t cpuinfo_min_freq;
+    uint32_t scaling_cur_freq;
+
+    uint32_t scaling_turbo_pct;
+    uint32_t scaling_max_perf;
+    uint32_t scaling_min_perf;
+    enum xen_perf_alias perf_alias;
 
     /* for all governors */
     /* OUT variable */
@@ -2278,22 +2289,13 @@ struct xc_get_cpufreq_para {
     char     *scaling_available_governors;
     char scaling_driver[CPUFREQ_NAME_LEN];
 
-    uint32_t cpuinfo_cur_freq;
-    uint32_t cpuinfo_max_freq;
-    uint32_t cpuinfo_min_freq;
-    uint32_t scaling_cur_freq;
-
     char scaling_governor[CPUFREQ_NAME_LEN];
-    uint32_t scaling_max_freq;
-    uint32_t scaling_min_freq;
 
     /* for specific governor */
     union {
         xc_userspace_t userspace;
         xc_ondemand_t ondemand;
     } u;
-
-    int32_t turbo_enabled;
 };
 
 int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
tools/libxc/xc_pm.c +9 -7
index 5b38cf1..6a16e8a 100644
@@ -260,13 +260,15 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
     }
     else
     {
-        user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq;
-        user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq;
-        user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq;
-        user_para->scaling_cur_freq = sys_para->scaling_cur_freq;
-        user_para->scaling_max_freq = sys_para->scaling_max_freq;
-        user_para->scaling_min_freq = sys_para->scaling_min_freq;
-        user_para->turbo_enabled    = sys_para->turbo_enabled;
+        user_para->cpuinfo_cur_freq  = sys_para->cpuinfo_cur_freq;
+        user_para->cpuinfo_max_freq  = sys_para->cpuinfo_max_freq;
+        user_para->cpuinfo_min_freq  = sys_para->cpuinfo_min_freq;
+        user_para->scaling_cur_freq  = sys_para->scaling_cur_freq;
+        user_para->scaling_max_perf  = sys_para->scaling_max_perf;
+        user_para->scaling_min_perf  = sys_para->scaling_min_perf;
+        user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct;
+        user_para->perf_alias        = sys_para->perf_alias;
+        user_para->turbo_enabled     = sys_para->turbo_enabled;
 
         memcpy(user_para->scaling_driver,
                 sys_para->scaling_driver, CPUFREQ_NAME_LEN);
tools/misc/xenpm.c +2 -2
index 08f2242..5944fdb 100644
@@ -705,8 +705,8 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
     printf("\n");
 
     printf("scaling frequency    : max [%u] min [%u] cur [%u]\n",
-           p_cpufreq->scaling_max_freq,
-           p_cpufreq->scaling_min_freq,
+           p_cpufreq->scaling_max_perf,
+           p_cpufreq->scaling_min_perf,
            p_cpufreq->scaling_cur_freq);
 
     printf("turbo mode           : %s\n",
xen/drivers/acpi/pmstat.c +155 -28

275 lines not on this page · raw message

xen/include/acpi/cpufreq/cpufreq.h +2 -0
index ef6643a..5cbe71e 100644
@@ -107,7 +107,9 @@ struct cpufreq_freqs {
  *                          CPUFREQ GOVERNORS                        *
  *********************************************************************/
 
+
 /* Please add internal governors here */
+#define NON_INTERNAL_GOV              (0)
 #define INTERNAL_GOV_POWERSAVE        (1)
 #define INTERNAL_GOV_PERFORMANCE      (2)
 #define INTERNAL_GOV_USERSPACE        (3)
xen/include/public/sysctl.h +19 -10
index 96680eb..dcfe541 100644
@@ -297,11 +297,28 @@ typedef struct xen_ondemand xen_ondemand_t;
  * same as sysfs file name of native linux
  */
 #define CPUFREQ_NAME_LEN 16
+
+enum xen_perf_alias {
+    FREQUENCY  = 0,
+    PERCENTAGE = 1
+};
+
 struct xen_get_cpufreq_para {
     /* IN/OUT variable */
     uint32_t cpu_num;
     uint32_t freq_num;
     uint32_t gov_num;
+    int32_t turbo_enabled;
+
+    uint32_t cpuinfo_cur_freq;
+    uint32_t cpuinfo_max_freq;
+    uint32_t cpuinfo_min_freq;
+    uint32_t scaling_cur_freq;
+
+    uint32_t scaling_turbo_pct;
+    uint32_t scaling_max_perf;
+    uint32_t scaling_min_perf;
+    enum xen_perf_alias perf_alias;
 
     /* for all governors */
     /* OUT variable */
@@ -309,23 +326,13 @@ struct xen_get_cpufreq_para {
     XEN_GUEST_HANDLE_64(uint32) scaling_available_frequencies;
     XEN_GUEST_HANDLE_64(char)   scaling_available_governors;
     char scaling_driver[CPUFREQ_NAME_LEN];
-
-    uint32_t cpuinfo_cur_freq;
-    uint32_t cpuinfo_max_freq;
-    uint32_t cpuinfo_min_freq;
-    uint32_t scaling_cur_freq;
-
     char scaling_governor[CPUFREQ_NAME_LEN];
-    uint32_t scaling_max_freq;
-    uint32_t scaling_min_freq;
 
     /* for specific governor */
     union {
         struct  xen_userspace userspace;
         struct  xen_ondemand ondemand;
     } u;
-
-    int32_t turbo_enabled;
 };
 
 struct xen_set_cpufreq_gov {
@@ -338,6 +345,8 @@ struct xen_set_cpufreq_para {
     #define SCALING_SETSPEED           3
     #define SAMPLING_RATE              4
     #define UP_THRESHOLD               5
+    #define SCALING_MAX_PCT            6
+    #define SCALING_MIN_PCT            7
 
     uint32_t ctrl_type;
     uint32_t ctrl_value;
Signature · 1 line
1.9.1
Acked-by on v6 5/6
#10 lore raw

Re: [PATCH v6 5/6] x86/intel_pstate: support the use of intel_pstate in pmstat.c

Quoted 11 lines · Wei Wang

Add support in the pmstat.c so that the xenpm tool can request to access the intel_pstate driver.

---
changes in v6:
1) add the NON_INTERNAL_GOV macro to replace literal 0;
2) code consolidation (e.g. merging some code into if/else, as required in v5);
3) somewhere, change to use clamp, instead of clamp_t;
4) xen_perf_alias, instead of perf_alias.

Quoted 3 lines
 tools/libxc/include/xenctrl.h      |  20 ++--
 tools/libxc/xc_pm.c                |  16 ++--
 tools/misc/xenpm.c                 |   4 +-

I think all changes to toolstack code are merely trying to mirror hypervisor side changes and there are no functional changes.

With that understanding and subject to an ack from hypervisor side maintainer:

#11 lore raw

Re: [PATCH v6 5/6] x86/intel_pstate: support the use of intel_pstate in pmstat.c

In reply to Wei Wang
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
 tools/libxc/include/xenctrl.h      |  20 ++--
 tools/libxc/xc_pm.c                |  16 ++--

I won't (again) comment on the changes of these files (I'll leave it to the tools maintainers), but I can't help thinking that there's more shuffling there than necessary.

tools/misc/xenpm.c static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) · line 710

Quoted 11 lines
tools/misc/xenpm.c +2 -2
@@ -705,8 +705,8 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
     printf("\n");
 
     printf("scaling frequency    : max [%u] min [%u] cur [%u]\n",
-           p_cpufreq->scaling_max_freq,
-           p_cpufreq->scaling_min_freq,
+           p_cpufreq->scaling_max_perf,
+           p_cpufreq->scaling_min_perf,
            p_cpufreq->scaling_cur_freq);

So where did your percentages go?

Quoted 24 lines
tools/misc/xenpm.c +14 -5
@@ -240,40 +253,85 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
     if ( ret )
         return ret;
 
-    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 ( internal_gov )
     {
+        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
+                    internal_gov->avail_gov, gov_num * CPUFREQ_NAME_LEN);
+        if ( ret )
+            return ret;
+    }
+    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(char))) )
+        {

tools/misc/xenpm.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 279

Quoted 10 lines
tools/misc/xenpm.c +7 -1
+            xfree(scaling_available_governors);
+            return ret;
+        }
+        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
+                    scaling_available_governors, gov_num * CPUFREQ_NAME_LEN);
         xfree(scaling_available_governors);
-        return ret;
+        if ( ret )
+            return ret;
     }

Even if the benefit may look tiny, code that can be shared should be shared. In the case here, the conditional return should be pulled out of the if and else branches. (It in fact seems possible to also pull out the copy_to_guest(), which would result in quite a bit less churn on the code.)

tools/misc/xenpm.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 289

Quoted 12 lines
tools/misc/xenpm.c +10 -2
-    op->u.get_para.scaling_max_freq = policy->max;
-    op->u.get_para.scaling_min_freq = policy->min;
+    if ( internal_gov )
+    {
+        op->u.get_para.scaling_max_perf = limits->max_perf_pct;
+        op->u.get_para.scaling_min_perf = limits->min_perf_pct;
+        op->u.get_para.scaling_turbo_pct = limits->turbo_pct;
+        if ( !strncmp(cpufreq_driver->name,
+                     "intel_pstate", CPUFREQ_NAME_LEN) )
+            op->u.get_para.perf_alias = PERCENTAGE;
+        else
+            op->u.get_para.perf_alias = FREQUENCY;

Shouldn't the internal governor tell you, rather than keying this on its name?

Quoted 7 lines
tools/misc/xenpm.c +1 -0
@@ -299,16 +357,36 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
 static int set_cpufreq_gov(struct xen_sysctl_pm_op *op)
 {
     struct cpufreq_policy new_policy, *old_policy;
+    struct internal_governor *internal_gov;
 
     old_policy = per_cpu(cpufreq_cpu_policy, op->cpuid);

tools/misc/xenpm.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 369

Quoted 10 lines
tools/misc/xenpm.c +2 -3
     if ( !old_policy )
         return -EINVAL;
+    internal_gov = old_policy->internal_gov;
 
     memcpy(&new_policy, old_policy, sizeof(struct cpufreq_policy));
 
-    new_policy.governor = __find_governor(op->u.set_gov.scaling_governor);
-    if (new_policy.governor == NULL)
-        return -EINVAL;
+    if ( internal_gov && internal_gov->cur_gov )

Either the right side of the && has to go away (preferred), or it has to become != NON_INTERNAL_GOV.

tools/misc/xenpm.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 382

Quoted 13 lines
tools/misc/xenpm.c +13 -0
+    {
+        if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "performance", CPUFREQ_NAME_LEN) )
+            internal_gov->cur_gov = INTERNAL_GOV_PERFORMANCE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "powersave", CPUFREQ_NAME_LEN) )
+            internal_gov->cur_gov = INTERNAL_GOV_POWERSAVE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "userspace", CPUFREQ_NAME_LEN) )
+            internal_gov->cur_gov = INTERNAL_GOV_USERSPACE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "ondemand", CPUFREQ_NAME_LEN) )
+            internal_gov->cur_gov = INTERNAL_GOV_ONDEMAND;

else ...

Quoted 15 lines
tools/misc/xenpm.c +3 -1
@@ -317,10 +395,12 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
 {
     int ret = 0;
     struct cpufreq_policy *policy;
+    struct internal_governor *internal_gov;
 
     policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
+    internal_gov = policy->internal_gov;
 
-    if ( !policy || !policy->governor )
+    if ( !policy || (!policy->governor && !internal_gov) )
         return -EINVAL;
 
     switch(op->u.set_para.ctrl_type)

tools/misc/xenpm.c static int set_cpufreq_para(struct xen_sysctl_pm_op *op) · line 413

Quoted 5 lines
tools/misc/xenpm.c +2 -0
@@ -329,6 +409,9 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
     {
         struct cpufreq_policy new_policy;
 
+        if ( !policy->governor || internal_gov )
+            return -EINVAL;

The "!policy->governor" part looks to be dead code, considering the check done in the previous hunk.

xen/include/public/sysctl.h typedef struct xen_ondemand xen_ondemand_t; · line 304

Quoted 11 lines
xen/include/public/sysctl.h +5 -0
@@ -297,11 +297,28 @@ typedef struct xen_ondemand xen_ondemand_t;
  * same as sysfs file name of native linux
  */
 #define CPUFREQ_NAME_LEN 16
+
+enum xen_perf_alias {
+    FREQUENCY  = 0,
+    PERCENTAGE = 1
+};

The need proper prefixes to avoid name space collisions, e.g. XEN_CPUFREQ_MODE_ (and the enum name being "alias" is also odd, "mode" or some such would seem better there too).

Quoted 6 lines
xen/include/public/sysctl.h +1 -0
 struct xen_get_cpufreq_para {
     /* IN/OUT variable */
     uint32_t cpu_num;
     uint32_t freq_num;
     uint32_t gov_num;
+    int32_t turbo_enabled;

xen/include/public/sysctl.h typedef struct xen_ondemand xen_ondemand_t; · line 321

Quoted 10 lines
xen/include/public/sysctl.h +10 -0
+
+    uint32_t cpuinfo_cur_freq;
+    uint32_t cpuinfo_max_freq;
+    uint32_t cpuinfo_min_freq;
+    uint32_t scaling_cur_freq;
+
+    uint32_t scaling_turbo_pct;
+    uint32_t scaling_max_perf;
+    uint32_t scaling_min_perf;
+    enum xen_perf_alias perf_alias;

You shouldn't use fields of non-fixed type in the public interface.

Quoted 12 lines
xen/include/public/sysctl.h +0 -7
@@ -309,23 +326,13 @@ struct xen_get_cpufreq_para {
     XEN_GUEST_HANDLE_64(uint32) scaling_available_frequencies;
     XEN_GUEST_HANDLE_64(char)   scaling_available_governors;
     char scaling_driver[CPUFREQ_NAME_LEN];
-
-    uint32_t cpuinfo_cur_freq;
-    uint32_t cpuinfo_max_freq;
-    uint32_t cpuinfo_min_freq;
-    uint32_t scaling_cur_freq;
-
     char scaling_governor[CPUFREQ_NAME_LEN];
-    uint32_t scaling_max_freq;

xen/include/public/sysctl.h struct xen_get_cpufreq_para { · line 336

Quoted 10 lines
xen/include/public/sysctl.h +0 -3
-    uint32_t scaling_min_freq;
 
     /* for specific governor */
     union {
         struct  xen_userspace userspace;
         struct  xen_ondemand ondemand;
     } u;
-
-    int32_t turbo_enabled;
 };

Just to repeat myself: Too much shuffling for no reason. I can see why you want turbo_enabled go up (to reduce holes), but I don't see the point of any of the other moving around of members.

Jan

author
#12 lore raw

PATCH v6 6/6 tools: enable xenpm to control the intel_pstate driver

In reply to Wei Wang

The intel_pstate driver receives percentage values to set the performance limits. This patch adds interfaces to support the input of percentage values to control the intel_pstate driver. The "get-cpufreq-para" is modified to show percentage based feedback info.
Also, some changes in identation are made to make the printed info looks tidy.

---
changes in v6:
No big change in this version, since we did not get any comments from the maintainers in the previous version.

 tools/misc/xenpm.c | 116 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 94 insertions(+), 22 deletions(-)
tools/misc/xenpm.c +94 -22
index 5944fdb..5644817 100644
@@ -32,6 +32,11 @@
 #define MAX_CORE_RESIDENCIES 8
 
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
+#define min_t(type,x,y) \
+        ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
+#define max_t(type,x,y) \
+        ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
+#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
 
 static xc_interface *xc_handle;
 static unsigned int max_cpu_nr;
@@ -46,6 +51,9 @@ void show_help(void)
             " get-cpuidle-states    [cpuid]       list cpu idle info of CPU <cpuid> or all\n"
             " get-cpufreq-states    [cpuid]       list cpu freq info of CPU <cpuid> or all\n"
             " get-cpufreq-para      [cpuid]       list cpu freq parameter of CPU <cpuid> or all\n"
+            " set-scaling-max-pct   [cpuid] <num> set max performance limit in percentage\n"
+            "                                     or as scaling speed in percentage in userspace governor\n"
+            " set-scaling-min-pct   [cpuid] <num> set min performance limit in percentage\n"
             " set-scaling-maxfreq   [cpuid] <HZ>  set max cpu frequency <HZ> on CPU <cpuid>\n"
             "                                     or all CPUs\n"
             " set-scaling-minfreq   [cpuid] <HZ>  set min cpu frequency <HZ> on CPU <cpuid>\n"
@@ -59,10 +67,10 @@ void show_help(void)
             " set-up-threshold      [cpuid] <num> set up threshold on CPU <cpuid> or all\n"
             "                                     it is used in ondemand governor.\n"
             " get-cpu-topology                    get thread/core/socket topology info\n"
-            " set-sched-smt           enable|disable enable/disable scheduler smt power saving\n"
+            " set-sched-smt                       enable|disable enable/disable scheduler smt power saving\n"
             " set-vcpu-migration-delay      <num> set scheduler vcpu migration delay in us\n"
             " get-vcpu-migration-delay            get scheduler vcpu migration delay\n"
-            " set-max-cstate        <num>         set the C-State limitation (<num> >= 0)\n"
+            " set-max-cstate                <num> set the C-State limitation (<num> >= 0)\n"
             " start [seconds]                     start collect Cx/Px statistics,\n"
             "                                     output after CTRL-C or SIGINT or several seconds.\n"
             " enable-turbo-mode     [cpuid]       enable Turbo Mode for processors that support it.\n"
@@ -677,37 +685,51 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
 
     printf("current_governor     : %s\n", p_cpufreq->scaling_governor);
     if ( !strncmp(p_cpufreq->scaling_governor,
-                  "userspace", CPUFREQ_NAME_LEN) )
+                  "userspace", CPUFREQ_NAME_LEN) &&
+         strncmp(p_cpufreq->scaling_driver,
+                 "intel_pstate", CPUFREQ_NAME_LEN) )
     {
-        printf("  userspace specific :\n");
-        printf("    scaling_setspeed : %u\n",
+        printf("userspace specific   :\n");
+        printf("scaling_setspeed     : %u\n",
                p_cpufreq->u.userspace.scaling_setspeed);
     }
     else if ( !strncmp(p_cpufreq->scaling_governor,
-                       "ondemand", CPUFREQ_NAME_LEN) )
+                       "ondemand", CPUFREQ_NAME_LEN) &&
+              strncmp(p_cpufreq->scaling_driver,
+                      "intel_pstate", CPUFREQ_NAME_LEN) )
     {
-        printf("  ondemand specific  :\n");
-        printf("    sampling_rate    : max [%u] min [%u] cur [%u]\n",
+        printf("ondemand specific    :\n");
+        printf("sampling_rate        : max [%u] min [%u] cur [%u]\n",
                p_cpufreq->u.ondemand.sampling_rate_max,
                p_cpufreq->u.ondemand.sampling_rate_min,
                p_cpufreq->u.ondemand.sampling_rate);
-        printf("    up_threshold     : %u\n",
+        printf("up_threshold         : %u\n",
                p_cpufreq->u.ondemand.up_threshold);
     }
 
-    printf("scaling_avail_freq   :");
-    for ( i = 0; i < p_cpufreq->freq_num; i++ )
-        if ( p_cpufreq->scaling_available_frequencies[i] ==
-             p_cpufreq->scaling_cur_freq )
-            printf(" *%d", p_cpufreq->scaling_available_frequencies[i]);
-        else
-            printf(" %d", p_cpufreq->scaling_available_frequencies[i]);
-    printf("\n");
-
-    printf("scaling frequency    : max [%u] min [%u] cur [%u]\n",
-           p_cpufreq->scaling_max_perf,
-           p_cpufreq->scaling_min_perf,
-           p_cpufreq->scaling_cur_freq);
+    switch ( p_cpufreq->perf_alias )
+    {
+    case PERCENTAGE:
+        printf("max_perf_pct         : %d\n", p_cpufreq->scaling_max_perf);
+        printf("min_perf_pct         : %d\n", p_cpufreq->scaling_min_perf);
+        printf("turbo_pct            : %d\n", p_cpufreq->scaling_turbo_pct);
+        break;
+    case FREQUENCY:
+    default:
+        printf("scaling_avail_freq   :");
+        for ( i = 0; i < p_cpufreq->freq_num; i++ )
+            if ( p_cpufreq->scaling_available_frequencies[i] ==
+                 p_cpufreq->scaling_cur_freq )
+                printf(" *%d", p_cpufreq->scaling_available_frequencies[i]);
+            else
+                printf(" %d", p_cpufreq->scaling_available_frequencies[i]);
+        printf("\n");
+        printf("scaling frequency    : max [%u] min [%u] cur [%u]\n",
+               p_cpufreq->scaling_max_perf,
+               p_cpufreq->scaling_min_perf,
+               p_cpufreq->scaling_cur_freq);
+        break;
+    }
 
     printf("turbo mode           : %s\n",
            p_cpufreq->turbo_enabled ? "enabled" : "disabled or n/a");
@@ -854,6 +876,54 @@ void scaling_min_freq_func(int argc, char *argv[])
     }
 }
 
+void scaling_max_pct_func(int argc, char *argv[])
+{
+    int cpuid = -1, pct = -1;
+
+    parse_cpuid_and_int(argc, argv, &cpuid, &pct, "percentage");
+    pct = clamp_t(int, pct, 0, 100);
+
+    if ( cpuid < 0 )
+    {
+        int i;
+        for ( i = 0; i < max_cpu_nr; i++ )
+            if ( xc_set_cpufreq_para(xc_handle, i, SCALING_MAX_PCT, pct) )
+                fprintf(stderr,
+                        "[CPU%d] failed to set scaling max freq (%d - %s)\n",
+                        i, errno, strerror(errno));
+    }
+    else
+    {
+        if ( xc_set_cpufreq_para(xc_handle, cpuid, SCALING_MAX_PCT, pct) )
+            fprintf(stderr, "failed to set scaling max freq (%d - %s)\n",
+                    errno, strerror(errno));
+    }
+}
+
+void scaling_min_pct_func(int argc, char *argv[])
+{
+    int cpuid = -1, pct = -1;
+
+    parse_cpuid_and_int(argc, argv, &cpuid, &pct, "percentage");
+    pct = clamp_t(int, pct, 0, 100);
+
+    if ( cpuid < 0 )
+    {
+        int i;
+        for ( i = 0; i < max_cpu_nr; i++ )
+            if ( xc_set_cpufreq_para(xc_handle, i, SCALING_MIN_PCT, pct) )
+                fprintf(stderr,
+                        "[CPU%d] failed to set scaling max pct (%d - %s)\n",
+                        i, errno, strerror(errno));
+    }
+    else
+    {
+        if ( xc_set_cpufreq_para(xc_handle, cpuid, SCALING_MIN_PCT, pct) )
+            fprintf(stderr, "failed to set scaling min pct (%d - %s)\n",
+                    errno, strerror(errno));
+    }
+}
+
 void scaling_speed_func(int argc, char *argv[])
 {
     int cpuid = -1, speed = -1;
@@ -1133,6 +1203,8 @@ struct {
     { "get-cpufreq-para", cpufreq_para_func },
     { "set-scaling-maxfreq", scaling_max_freq_func },
     { "set-scaling-minfreq", scaling_min_freq_func },
+    { "set-scaling-max-pct", scaling_max_pct_func},
+    { "set-scaling-min-pct", scaling_min_pct_func},
     { "set-scaling-governor", scaling_governor_func },
     { "set-scaling-speed", scaling_speed_func },
     { "set-sampling-rate", scaling_sampling_rate_func },
Signature · 1 line
1.9.1
Acked-by on v6 6/6
#13 lore raw

Re: [PATCH v6 6/6] tools: enable xenpm to control the intel_pstate driver

Quoted 9 lines · Wei Wang

The intel_pstate driver receives percentage values to set the performance limits. This patch adds interfaces to support the input of percentage values to control the intel_pstate driver. The "get-cpufreq-para" is modified to show percentage based feedback info.
Also, some changes in identation are made to make the printed info looks tidy.