[PATCH v2 0/4] Support controlling the max C-state sub-state

[PATCH v2 0/4] Support controlling the max C-state sub-state

[Ross Lagerwall]

As discussed previously on the list, here is a patch series to allow
controlling the maximum C-state sub-state. It doesn't fix the output of
xenpm to correctly show the C-states sub-states (that can be for later).

Changes since v1:
- Use a single boot parameter to control max_cstate and max_csubstate.
- Use max_csubstate rather than max_substate global variable,
  it is less generic.
- Reuse sysctl sub-ops rather than adding new ones.
- Limit the sub-state in the ACPI cpu_idle driver.
- Use unsigned rather than signed in places
- Update docs.
- Formatting changes.

Ross Lagerwall (4):
  x86/mwait_idle: Fix trace output
  x86: Allow limiting the max C-state sub-state
  tools/libxc: Alow controlling the max C-state sub-state
  xenpm: Allow controlling the max C-state sub-state

 docs/misc/xen-command-line.markdown |  8 +++++++-
 tools/libxc/xc_pm.c                 | 28 ++++++++++++++++++++++++----
 tools/libxc/xenctrl.h               |  3 +++
 tools/misc/xenpm.c                  | 34 +++++++++++++++++++++++++++++++++-
 xen/arch/x86/acpi/cpu_idle.c        | 20 +++++++++++++++++---
 xen/arch/x86/cpu/mwait-idle.c       |  8 +++++---
 xen/drivers/acpi/pmstat.c           |  8 ++++++--
 xen/include/xen/acpi.h              | 29 +++++++++++++++++++++++++----
 8 files changed, 120 insertions(+), 18 deletions(-)

-- 
1.9.3

[PATCH v2 1/4] x86/mwait_idle: Fix trace output

[Ross Lagerwall]

Use the C-state's type when tracing, not its index since the index is
not set by the mwait_idle driver.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/cpu/mwait-idle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 72a7abf..38172e5 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -376,7 +376,7 @@ static void mwait_idle(void)
 		lapic_timer_off();
 
 	before = cpuidle_get_tick();
-	TRACE_4D(TRC_PM_IDLE_ENTRY, cx->idx, before, exp, pred);
+	TRACE_4D(TRC_PM_IDLE_ENTRY, cx->type, before, exp, pred);
 
 	if (cpu_is_haltable(cpu))
 		mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
@@ -385,7 +385,7 @@ static void mwait_idle(void)
 
 	cstate_restore_tsc();
 	trace_exit_reason(irq_traced);
-	TRACE_6D(TRC_PM_IDLE_EXIT, cx->idx, after,
+	TRACE_6D(TRC_PM_IDLE_EXIT, cx->type, after,
 		irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]);
 
 	update_idle_stats(power, cx, before, after);
-- 
1.9.3

[PATCH v2 2/4] x86: Allow limiting the max C-state sub-state

[Ross Lagerwall]

Allow limiting the max C-state sub-state by appending to the max_cstate
command-line parameter. E.g. max_cstate=1,0
The limit only applies to the highest legal C-state. For example:
 max_cstate = 1, max_csubstate = 0 ==> C0, C1 okay, but not C1E
 max_cstate = 1, max_csubstate = 1 ==> C0, C1 and C1E okay, but not C2
 max_cstate = 2, max_csubstate = 0 ==> C0, C1, C1E, C2 okay, but not C3
 max_cstate = 2, max_csubstate = 1 ==> C0, C1, C1E, C2 okay, but not C3

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 docs/misc/xen-command-line.markdown |  8 +++++++-
 xen/arch/x86/acpi/cpu_idle.c        | 20 +++++++++++++++++---
 xen/arch/x86/cpu/mwait-idle.c       |  4 +++-
 xen/include/xen/acpi.h              | 16 ++++++++++++----
 4 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a7ac53d..1122f2c 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -692,7 +692,13 @@ so the crash kernel may find find them.  Should be used in combination
 with **crashinfo_maxaddr**.
 
 ### max\_cstate
-> `= <integer>`
+> `= <cstate>[,<substate>]`
+
+`cstate` is an integer which limits the maximum C-state that Xen uses.
+
+`substate` is an integer which limits the maximum sub C-state that Xen
+uses.  The limit only applies to the highest legal C-state.
+
 
 ### max\_gsi\_irqs
 > `= <integer>`
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index b05fb39..a16c0b5 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -105,7 +105,16 @@ static uint64_t (*__read_mostly tick_to_ns)(uint64_t) = acpi_pm_tick_to_ns;
 
 void (*__read_mostly pm_idle_save)(void);
 unsigned int max_cstate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
-integer_param("max_cstate", max_cstate);
+unsigned int max_csubstate __read_mostly = UINT_MAX;
+
+static void __init parse_cstate(const char *s)
+{
+    max_cstate = simple_strtoul(s, &s, 0);
+    if ( *s == ',' )
+        max_csubstate = simple_strtoul(s + 1, &s, 0);
+}
+custom_param("max_cstate", parse_cstate);
+
 static bool_t __read_mostly local_apic_timer_c2_ok;
 boolean_param("lapic_timer_c2_ok", local_apic_timer_c2_ok);
 
@@ -240,6 +249,7 @@ static void print_acpi_power(uint32_t cpu, struct acpi_processor_power *power)
     last_state_idx = power->last_state ? power->last_state->idx : -1;
     printk("active state:\t\tC%d\n", last_state_idx);
     printk("max_cstate:\t\tC%d\n", max_cstate);
+    printk("max_csubstate:\t\t%u\n", max_csubstate);
     printk("states:\n");
     
     for ( i = 1; i < power->count; i++ )
@@ -484,8 +494,12 @@ static void acpi_processor_idle(void)
         if ( cx->type == ACPI_STATE_C3 && power->flags.bm_check &&
              acpi_idle_bm_check() )
             cx = power->safe_state;
-        if ( cx->idx > max_cstate )
-            cx = &power->states[max_cstate];
+        while ( ( cx->idx > max_cstate ||
+                ( cx->entry_method == ACPI_CSTATE_EM_FFH &&
+                  cx->idx == max_cstate &&
+                  ( cx->address & MWAIT_SUBSTATE_MASK ) > max_csubstate ) ) &&
+               next_state-- );
+            cx = &power->states[next_state];
         menu_get_trace_data(&exp, &pred);
     }
     if ( !cx )
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 38172e5..3bad6d8 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -330,7 +330,9 @@ static void mwait_idle(void)
 	    (next_state = cpuidle_current_governor->select(power)) > 0) {
 		do {
 			cx = &power->states[next_state];
-		} while (cx->type > max_cstate && --next_state);
+		} while ((cx->type > max_cstate || (cx->type == max_cstate &&
+			  MWAIT_HINT2SUBSTATE(cx->address) > max_csubstate)) &&
+			 --next_state);
 		if (!next_state)
 			cx = NULL;
 		menu_get_trace_data(&exp, &pred);
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index aedec65..c3925bc 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -123,13 +123,21 @@ void acpi_unregister_gsi (u32 gsi);
 
 #ifdef	CONFIG_ACPI_CSTATE
 /*
- * Set highest legal C-state
- * 0: C0 okay, but not C1
- * 1: C1 okay, but not C2
- * 2: C2 okay, but not C3 etc.
+ * max_cstate sets the highest legal C-state.
+ * max_cstate = 0: C0 okay, but not C1
+ * max_cstate = 1: C1 okay, but not C2
+ * max_cstate = 2: C2 okay, but not C3 etc.
+
+ * max_csubstate sets the highest legal C-state sub-state. Only applies to the
+ * highest legal C-state.
+ * max_cstate = 1, max_csubstate = 0 ==> C0, C1 okay, but not C1E
+ * max_cstate = 1, max_csubstate = 1 ==> C0, C1 and C1E okay, but not C2
+ * max_cstate = 2, max_csubstate = 0 ==> C0, C1, C1E, C2 okay, but not C3
+ * max_cstate = 2, max_csubstate = 1 ==> C0, C1, C1E, C2 okay, but not C3
  */
 
 extern unsigned int max_cstate;
+extern unsigned int max_csubstate;
 
 static inline unsigned int acpi_get_cstate_limit(void)
 {
-- 
1.9.3

Re: [PATCH v2 2/4] x86: Allow limiting the max C-state sub-state

[Jan Beulich]

>>> On 19.06.14 at 13:16, <ross.lagerwall@citrix.com> wrote:
> @@ -484,8 +494,12 @@ static void acpi_processor_idle(void)
>          if ( cx->type == ACPI_STATE_C3 && power->flags.bm_check &&
>               acpi_idle_bm_check() )
>              cx = power->safe_state;
> -        if ( cx->idx > max_cstate )
> -            cx = &power->states[max_cstate];
> +        while ( ( cx->idx > max_cstate ||
> +                ( cx->entry_method == ACPI_CSTATE_EM_FFH &&
> +                  cx->idx == max_cstate &&
> +                  ( cx->address & MWAIT_SUBSTATE_MASK ) > max_csubstate ) ) &&
> +               next_state-- );
> +            cx = &power->states[next_state];

Wouldn't this need switching to using cx->type instead of cx->idx?

Also please put spaces after opening/before closing parentheses
only when those are the outer ones following a keyword.

Jan

[PATCH v2 3/4] tools/libxc: Alow controlling the max C-state sub-state

[Ross Lagerwall]

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 tools/libxc/xc_pm.c       | 28 ++++++++++++++++++++++++----
 tools/libxc/xenctrl.h     |  3 +++
 xen/drivers/acpi/pmstat.c |  8 ++++++--
 xen/include/xen/acpi.h    | 13 +++++++++++++
 4 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index e4e0fb9..9631d99 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -386,7 +386,7 @@ int xc_get_vcpu_migration_delay(xc_interface *xch, uint32_t *value)
    return rc;
 }
 
-int xc_get_cpuidle_max_cstate(xc_interface *xch, uint32_t *value)
+static int get_max_cstate(xc_interface *xch, uint32_t *value, uint32_t type)
 {
     int rc;
     DECLARE_SYSCTL;
@@ -396,7 +396,7 @@ int xc_get_cpuidle_max_cstate(xc_interface *xch, uint32_t *value)
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = XEN_SYSCTL_pm_op_get_max_cstate;
-    sysctl.u.pm_op.cpuid = 0;
+    sysctl.u.pm_op.cpuid = type;
     sysctl.u.pm_op.u.get_max_cstate = 0;
     rc = do_sysctl(xch, &sysctl);
     *value = sysctl.u.pm_op.u.get_max_cstate;
@@ -404,7 +404,17 @@ int xc_get_cpuidle_max_cstate(xc_interface *xch, uint32_t *value)
     return rc;
 }
 
-int xc_set_cpuidle_max_cstate(xc_interface *xch, uint32_t value)
+int xc_get_cpuidle_max_cstate(xc_interface *xch, uint32_t *value)
+{
+    return get_max_cstate(xch, value, 0);
+}
+
+int xc_get_cpuidle_max_csubstate(xc_interface *xch, uint32_t *value)
+{
+    return get_max_cstate(xch, value, 1);
+}
+
+static int set_max_cstate(xc_interface *xch, uint32_t value, uint32_t type)
 {
     DECLARE_SYSCTL;
 
@@ -413,12 +423,22 @@ int xc_set_cpuidle_max_cstate(xc_interface *xch, uint32_t value)
 
     sysctl.cmd = XEN_SYSCTL_pm_op;
     sysctl.u.pm_op.cmd = XEN_SYSCTL_pm_op_set_max_cstate;
-    sysctl.u.pm_op.cpuid = 0;
+    sysctl.u.pm_op.cpuid = type;
     sysctl.u.pm_op.u.set_max_cstate = value;
 
     return do_sysctl(xch, &sysctl);
 }
 
+int xc_set_cpuidle_max_cstate(xc_interface *xch, uint32_t value)
+{
+    return set_max_cstate(xch, value, 0);
+}
+
+int xc_set_cpuidle_max_csubstate(xc_interface *xch, uint32_t value)
+{
+    return set_max_cstate(xch, value, 1);
+}
+
 int xc_enable_turbo(xc_interface *xch, int cpuid)
 {
     DECLARE_SYSCTL;
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 02129f7..5e90c9b 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1992,6 +1992,9 @@ int xc_get_vcpu_migration_delay(xc_interface *xch, uint32_t *value);
 int xc_get_cpuidle_max_cstate(xc_interface *xch, uint32_t *value);
 int xc_set_cpuidle_max_cstate(xc_interface *xch, uint32_t value);
 
+int xc_get_cpuidle_max_csubstate(xc_interface *xch, uint32_t *value);
+int xc_set_cpuidle_max_csubstate(xc_interface *xch, uint32_t value);
+
 int xc_enable_turbo(xc_interface *xch, int cpuid);
 int xc_disable_turbo(xc_interface *xch, int cpuid);
 /**
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index daac2da..75538d2 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -465,13 +465,17 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
 
     case XEN_SYSCTL_pm_op_get_max_cstate:
     {
-        op->u.get_max_cstate = acpi_get_cstate_limit();
+        op->u.get_max_cstate = op->cpuid == 0 ?
+            acpi_get_cstate_limit() : acpi_get_csubstate_limit();
         break;
     }
 
     case XEN_SYSCTL_pm_op_set_max_cstate:
     {
-        acpi_set_cstate_limit(op->u.set_max_cstate);
+        if ( op->cpuid == 0 )
+            acpi_set_cstate_limit(op->u.set_max_cstate);
+        else
+            acpi_set_csubstate_limit(op->u.set_max_cstate);
         break;
     }
 
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index c3925bc..7ae9bf0 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -148,9 +148,22 @@ static inline void acpi_set_cstate_limit(unsigned int new_limit)
 	max_cstate = new_limit;
 	return;
 }
+
+static inline unsigned int acpi_get_csubstate_limit(void)
+{
+	return max_csubstate;
+}
+
+static inline void acpi_set_csubstate_limit(unsigned int new_limit)
+{
+	max_csubstate = new_limit;
+}
+
 #else
 static inline unsigned int acpi_get_cstate_limit(void) { return 0; }
 static inline void acpi_set_cstate_limit(unsigned int new_limit) { return; }
+static inline unsigned int acpi_get_csubstate_limit(void) { return 0; }
+static inline void acpi_set_csubstate_limit(unsigned int new_limit) { return; }
 #endif
 
 #ifdef XEN_GUEST_HANDLE_PARAM
-- 
1.9.3

Re: [PATCH v2 3/4] tools/libxc: Alow controlling the max C-state sub-state

[Jan Beulich]

>>> On 19.06.14 at 13:16, <ross.lagerwall@citrix.com> wrote:
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -465,13 +465,17 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>  
>      case XEN_SYSCTL_pm_op_get_max_cstate:
>      {
> -        op->u.get_max_cstate = acpi_get_cstate_limit();
> +        op->u.get_max_cstate = op->cpuid == 0 ?
> +            acpi_get_cstate_limit() : acpi_get_csubstate_limit();
>          break;
>      }
>  
>      case XEN_SYSCTL_pm_op_set_max_cstate:
>      {
> -        acpi_set_cstate_limit(op->u.set_max_cstate);
> +        if ( op->cpuid == 0 )
> +            acpi_set_cstate_limit(op->u.set_max_cstate);
> +        else
> +            acpi_set_csubstate_limit(op->u.set_max_cstate);
>          break;
>      }
>  

Please properly distinguish ->cpuid being zero, one, or anything else
(so that other values can be given a meaning later). Also with 0 and
1 now being valid, I think you need to adjust handling further up in
the function, so that even when running with just a single CPU (which
admittedly should be rare these days) things work correctly. And
finally, I guess you ought to still add a note to the public header
briefly describing the new behavior.

Jan

[PATCH v2 4/4] xenpm: Allow controlling the max C-state sub-state

[Ross Lagerwall]

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 tools/misc/xenpm.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index e43924c..9b52b00 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -64,6 +64,7 @@ void show_help(void)
             " 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-csubstate     <num>         set the C-State sub-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"
@@ -188,7 +189,19 @@ static int show_max_cstate(xc_interface *xc_handle)
     if ( (ret = xc_get_cpuidle_max_cstate(xc_handle, &value)) )
         return ret;
 
-    printf("Max possible C-state: C%d\n\n", value);
+    printf("Max possible C-state: C%d\n", value);
+    return 0;
+}
+
+static int show_max_csubstate(xc_interface *xc_handle)
+{
+    int ret = 0;
+    uint32_t value;
+
+    if ( (ret = xc_get_cpuidle_max_csubstate(xc_handle, &value)) )
+        return ret;
+
+    printf("Max possible C-state sub-state: %u\n\n", value);
     return 0;
 }
 
@@ -223,6 +236,7 @@ void cxstat_func(int argc, char *argv[])
         parse_cpuid(argv[0], &cpuid);
 
     show_max_cstate(xc_handle);
+    show_max_csubstate(xc_handle);
 
     if ( cpuid < 0 )
     {
@@ -1088,6 +1102,23 @@ void set_max_cstate_func(int argc, char *argv[])
                 value, errno, strerror(errno));
 }
 
+void set_max_csubstate_func(int argc, char *argv[])
+{
+    uint32_t value;
+
+    if ( argc != 1 || sscanf(argv[0], "%u", &value) != 1 )
+    {
+        fprintf(stderr, "Missing or invalid argument(s)\n");
+        exit(EINVAL);
+    }
+
+    if ( !xc_set_cpuidle_max_csubstate(xc_handle, value) )
+        printf("set max_csubstate to %u succeeded\n", value);
+    else
+        fprintf(stderr, "set max_csubstate to %u failed (%d - %s)\n",
+                value, errno, strerror(errno));
+}
+
 void enable_turbo_mode(int argc, char *argv[])
 {
     int cpuid = -1;
@@ -1154,6 +1185,7 @@ struct {
     { "get-vcpu-migration-delay", get_vcpu_migration_delay_func},
     { "set-vcpu-migration-delay", set_vcpu_migration_delay_func},
     { "set-max-cstate", set_max_cstate_func},
+    { "set-max-csubstate", set_max_csubstate_func},
     { "enable-turbo-mode", enable_turbo_mode },
     { "disable-turbo-mode", disable_turbo_mode },
 };
-- 
1.9.3

Re: [PATCH v2 0/4] Support controlling the max C-state sub-state

[Jan Beulich]

>>> Ross Lagerwall <ross.lagerwall@citrix.com> 06/19/14 1:17 PM >>>

Can you do something about your email setup please - this and patch 2 ended in
6 copies in my inbox (3 direct deliveries and 3 via xen-devel). The previous version
had similar, but not as bad (4 copies of two or three of the series constituents)
behavior too.

Thanks, Jan

Re: [PATCH v2 0/4] Support controlling the max C-state sub-state

[Ross Lagerwall]

On 06/19/2014 05:20 PM, Jan Beulich wrote:
>>>> Ross Lagerwall <ross.lagerwall@citrix.com> 06/19/14 1:17 PM >>>
>
> Can you do something about your email setup please - this and patch 2 ended in
> 6 copies in my inbox (3 direct deliveries and 3 via xen-devel). The previous version
> had similar, but not as bad (4 copies of two or three of the series constituents)
> behavior too.
>

Sorry about that, I think my sendmail is misconfigured.  I will fix it.

Thanks,
-- 
Ross Lagerwall