[PATCH 0/4] Support controlling the maximum sub C-State

[PATCH 0/4] Support controlling the maximum sub C-State

[Ross Lagerwall]

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

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

 tools/libxc/xc_pm.c           | 33 +++++++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h         |  3 +++
 tools/misc/xenpm.c            | 34 +++++++++++++++++++++++++++++++++-
 xen/arch/x86/acpi/cpu_idle.c  |  3 +++
 xen/arch/x86/cpu/mwait-idle.c |  8 +++++---
 xen/drivers/acpi/pmstat.c     | 12 ++++++++++++
 xen/include/public/sysctl.h   |  6 ++++++
 xen/include/xen/acpi.h        | 22 ++++++++++++++++++++++
 8 files changed, 117 insertions(+), 4 deletions(-)

-- 
1.9.3

[PATCH 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 2/4] x86/mwait_idle: Allow limiting the sub C-state

[Ross Lagerwall]

Allow limiting the sub C-state using a new command-line parameter, max_substate.
The limit only applies to the highest legal C-state. For example:
 max_cstate = 1, max_substate = 0 ==> C0, C1 okay, but not C1E
 max_cstate = 1, max_substate = 1 ==> C0, C1 and C1E okay, but not C2
 max_cstate = 2, max_substate = 0 ==> C0, C1, C1E, C2 okay, but not C3
 max_cstate = 2, max_substate = 1 ==> C0, C1, C1E, C2 okay, but not C3

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/acpi/cpu_idle.c  | 3 +++
 xen/arch/x86/cpu/mwait-idle.c | 4 +++-
 xen/include/xen/acpi.h        | 2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index b05fb39..96909b3 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -106,6 +106,8 @@ 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_substate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
+integer_param("max_substate", max_substate);
 static bool_t __read_mostly local_apic_timer_c2_ok;
 boolean_param("lapic_timer_c2_ok", local_apic_timer_c2_ok);
 
@@ -240,6 +242,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_substate:\t\t%d\n", max_substate);
     printk("states:\n");
     
     for ( i = 1; i < power->count; i++ )
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 38172e5..90c8aea 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_substate)) &&
+			 --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..b49c4fc 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -140,6 +140,8 @@ static inline void acpi_set_cstate_limit(unsigned int new_limit)
 	max_cstate = new_limit;
 	return;
 }
+
+extern unsigned int max_substate;
 #else
 static inline unsigned int acpi_get_cstate_limit(void) { return 0; }
 static inline void acpi_set_cstate_limit(unsigned int new_limit) { return; }
-- 
1.9.3

[PATCH 3/4] tools/libxc: Alow getting and setting the max sub C-State

[Ross Lagerwall]

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 tools/libxc/xc_pm.c         | 33 +++++++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h       |  3 +++
 xen/drivers/acpi/pmstat.c   | 12 ++++++++++++
 xen/include/public/sysctl.h |  6 ++++++
 xen/include/xen/acpi.h      | 20 ++++++++++++++++++++
 5 files changed, 74 insertions(+)

diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index e4e0fb9..ef72f02 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -419,6 +419,39 @@ int xc_set_cpuidle_max_cstate(xc_interface *xch, uint32_t value)
     return do_sysctl(xch, &sysctl);
 }
 
+int xc_get_cpuidle_max_substate(xc_interface *xch, uint32_t *value)
+{
+    int rc;
+    DECLARE_SYSCTL;
+
+    if ( !xch || !value )
+        return -EINVAL;
+
+    sysctl.cmd = XEN_SYSCTL_pm_op;
+    sysctl.u.pm_op.cmd = XEN_SYSCTL_pm_op_get_max_substate;
+    sysctl.u.pm_op.cpuid = 0;
+    sysctl.u.pm_op.u.get_max_substate = 0;
+    rc = do_sysctl(xch, &sysctl);
+    *value = sysctl.u.pm_op.u.get_max_substate;
+
+    return rc;
+}
+
+int xc_set_cpuidle_max_substate(xc_interface *xch, uint32_t value)
+{
+    DECLARE_SYSCTL;
+
+    if ( !xch )
+        return -EINVAL;
+
+    sysctl.cmd = XEN_SYSCTL_pm_op;
+    sysctl.u.pm_op.cmd = XEN_SYSCTL_pm_op_set_max_substate;
+    sysctl.u.pm_op.cpuid = 0;
+    sysctl.u.pm_op.u.set_max_substate = value;
+
+    return do_sysctl(xch, &sysctl);
+}
+
 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..d518812 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_substate(xc_interface *xch, uint32_t *value);
+int xc_set_cpuidle_max_substate(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..44598aa 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -475,6 +475,18 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
         break;
     }
 
+    case XEN_SYSCTL_pm_op_get_max_substate:
+    {
+        op->u.get_max_substate = acpi_get_substate_limit();
+        break;
+    }
+
+    case XEN_SYSCTL_pm_op_set_max_substate:
+    {
+        acpi_set_substate_limit(op->u.set_max_substate);
+        break;
+    }
+
     case XEN_SYSCTL_pm_op_enable_turbo:
     {
         ret = cpufreq_update_turbo(op->cpuid, CPUFREQ_TURBO_ENABLED);
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 3588698..1bb3767 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -366,6 +366,10 @@ struct xen_sysctl_pm_op {
     #define XEN_SYSCTL_pm_op_enable_turbo               0x26
     #define XEN_SYSCTL_pm_op_disable_turbo              0x27
 
+    /* cpuidle max_substate access command */
+    #define XEN_SYSCTL_pm_op_get_max_substate     0x28
+    #define XEN_SYSCTL_pm_op_set_max_substate     0x29
+
     uint32_t cmd;
     uint32_t cpuid;
     union {
@@ -376,6 +380,8 @@ struct xen_sysctl_pm_op {
         uint32_t                    set_sched_opt_smt;
         uint32_t                    get_max_cstate;
         uint32_t                    set_max_cstate;
+        uint32_t                    get_max_substate;
+        uint32_t                    set_max_substate;
         uint32_t                    get_vcpu_migration_delay;
         uint32_t                    set_vcpu_migration_delay;
     } u;
diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index b49c4fc..52e9074 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -141,10 +141,30 @@ static inline void acpi_set_cstate_limit(unsigned int new_limit)
 	return;
 }
 
+/*
+ * Set the highest legal sub C-state. Only applies to the highest legal C-state
+ * max_cstate = 1, max_substate = 0 ==> C0, C1 okay, but not C1E
+ * max_cstate = 1, max_substate = 1 ==> C0, C1 and C1E okay, but not C2
+ * max_cstate = 2, max_substate = 0 ==> C0, C1, C1E, C2 okay, but not C3
+ * max_cstate = 2, max_substate = 1 ==> C0, C1, C1E, C2 okay, but not C3
+ */
+
 extern unsigned int max_substate;
+
+static inline unsigned int acpi_get_substate_limit(void)
+{
+	return max_substate;
+}
+static inline void acpi_set_substate_limit(unsigned int new_limit)
+{
+	max_substate = new_limit;
+	return;
+}
 #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_substate_limit(void) { return 0; }
+static inline void acpi_set_substate_limit(unsigned int new_limit) { return; }
 #endif
 
 #ifdef XEN_GUEST_HANDLE_PARAM
-- 
1.9.3

[PATCH 4/4] xenpm: Alow getting and setting the max sub C-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..25d8613 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-substate      <num>         set the sub 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"
@@ -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_substate(xc_interface *xc_handle)
+{
+    int ret = 0;
+    uint32_t value;
+
+    if ( (ret = xc_get_cpuidle_max_substate(xc_handle, &value)) )
+        return ret;
+
+    printf("Max possible sub C-state: %d\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_substate(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_substate_func(int argc, char *argv[])
+{
+    int value;
+
+    if ( argc != 1 || sscanf(argv[0], "%d", &value) != 1 || value < 0 )
+    {
+        fprintf(stderr, "Missing or invalid argument(s)\n");
+        exit(EINVAL);
+    }
+
+    if ( !xc_set_cpuidle_max_substate(xc_handle, (uint32_t)value) )
+        printf("set max_substate to %d succeeded\n", value);
+    else
+        fprintf(stderr, "set max_substate to %d 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-substate", set_max_substate_func},
     { "enable-turbo-mode", enable_turbo_mode },
     { "disable-turbo-mode", disable_turbo_mode },
 };
-- 
1.9.3

Re: [PATCH 3/4] tools/libxc: Alow getting and setting the max sub C-State

[Andrew Cooper]

On 18/06/14 16:04, Ross Lagerwall wrote:
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  tools/libxc/xc_pm.c         | 33 +++++++++++++++++++++++++++++++++
>  tools/libxc/xenctrl.h       |  3 +++
>  xen/drivers/acpi/pmstat.c   | 12 ++++++++++++
>  xen/include/public/sysctl.h |  6 ++++++
>  xen/include/xen/acpi.h      | 20 ++++++++++++++++++++
>  5 files changed, 74 insertions(+)
>
>
> diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
> index b49c4fc..52e9074 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -141,10 +141,30 @@ static inline void acpi_set_cstate_limit(unsigned int new_limit)
>  	return;
>  }
>  
> +/*
> + * Set the highest legal sub C-state. Only applies to the highest legal C-state
> + * max_cstate = 1, max_substate = 0 ==> C0, C1 okay, but not C1E
> + * max_cstate = 1, max_substate = 1 ==> C0, C1 and C1E okay, but not C2
> + * max_cstate = 2, max_substate = 0 ==> C0, C1, C1E, C2 okay, but not C3
> + * max_cstate = 2, max_substate = 1 ==> C0, C1, C1E, C2 okay, but not C3
> + */
> +

This comment belongs in the previous patch.

>  extern unsigned int max_substate;
> +
> +static inline unsigned int acpi_get_substate_limit(void)
> +{
> +	return max_substate;
> +}

Newline in here please.

> +static inline void acpi_set_substate_limit(unsigned int new_limit)
> +{
> +	max_substate = new_limit;
> +	return;

Drop this return.

> +}

And a newline here please.

>  #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_substate_limit(void) { return 0; }
> +static inline void acpi_set_substate_limit(unsigned int new_limit) { return; }
>  #endif
>  
>  #ifdef XEN_GUEST_HANDLE_PARAM

Re: [PATCH 0/4] Support controlling the maximum sub C-State

[Konrad Rzeszutek Wilk]

On Wed, Jun 18, 2014 at 04:04:11PM +0100, Ross Lagerwall wrote:
> As discussed previously on the list, here is a patch series to allow
> controlling the maximum sub C-State. It doesn't fix the output of
> xenpm to correctly show the sub C-States (that can be for later).
> 
> Ross Lagerwall (4):
>   x86/mwait_idle: Fix trace output
>   x86/mwait_idle: Allow limiting the sub C-state
>   tools/libxc: Alow getting and setting the max sub C-State
>   xenpm: Alow getting and setting the max sub C-State
> 
>  tools/libxc/xc_pm.c           | 33 +++++++++++++++++++++++++++++++++
>  tools/libxc/xenctrl.h         |  3 +++
>  tools/misc/xenpm.c            | 34 +++++++++++++++++++++++++++++++++-
>  xen/arch/x86/acpi/cpu_idle.c  |  3 +++
>  xen/arch/x86/cpu/mwait-idle.c |  8 +++++---
>  xen/drivers/acpi/pmstat.c     | 12 ++++++++++++
>  xen/include/public/sysctl.h   |  6 ++++++
>  xen/include/xen/acpi.h        | 22 ++++++++++++++++++++++

I think you also need to update the manpage and/or docs directory?
>  8 files changed, 117 insertions(+), 4 deletions(-)
> 
> -- 
> 1.9.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Andrew Cooper]

On 18/06/14 16:04, Ross Lagerwall wrote:
> Allow limiting the sub C-state using a new command-line parameter, max_substate.
> The limit only applies to the highest legal C-state. For example:
>  max_cstate = 1, max_substate = 0 ==> C0, C1 okay, but not C1E
>  max_cstate = 1, max_substate = 1 ==> C0, C1 and C1E okay, but not C2
>  max_cstate = 2, max_substate = 0 ==> C0, C1, C1E, C2 okay, but not C3
>  max_cstate = 2, max_substate = 1 ==> C0, C1, C1E, C2 okay, but not C3
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

At the very least, please patch docs/misc/xen-command-line.markdown

However, the name "max_substate" is far too generic on its own.

"max_csubstate" would be better, but still not as easily identifiable on
its own as "max_cstate".

> ---
>  xen/arch/x86/acpi/cpu_idle.c  | 3 +++
>  xen/arch/x86/cpu/mwait-idle.c | 4 +++-
>  xen/include/xen/acpi.h        | 2 ++
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> index b05fb39..96909b3 100644
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -106,6 +106,8 @@ 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_substate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
> +integer_param("max_substate", max_substate);
>  static bool_t __read_mostly local_apic_timer_c2_ok;
>  boolean_param("lapic_timer_c2_ok", local_apic_timer_c2_ok);
>  
> @@ -240,6 +242,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_substate:\t\t%d\n", max_substate);

%u

The preceding line is also wrong with its formatting parameter.

>      printk("states:\n");
>      
>      for ( i = 1; i < power->count; i++ )
> diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
> index 38172e5..90c8aea 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_substate)) &&
> +			 --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..b49c4fc 100644
> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -140,6 +140,8 @@ static inline void acpi_set_cstate_limit(unsigned int new_limit)
>  	max_cstate = new_limit;
>  	return;
>  }
> +
> +extern unsigned int max_substate;

This should live next to the extern for max_cstate.

~Andrew

>  #else
>  static inline unsigned int acpi_get_cstate_limit(void) { return 0; }
>  static inline void acpi_set_cstate_limit(unsigned int new_limit) { return; }

Re: [PATCH 4/4] xenpm: Alow getting and setting the max sub C-State

[Andrew Cooper]

On 18/06/14 16:04, Ross Lagerwall wrote:
> 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..25d8613 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-substate      <num>         set the sub 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"
> @@ -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_substate(xc_interface *xc_handle)
> +{
> +    int ret = 0;
> +    uint32_t value;
> +
> +    if ( (ret = xc_get_cpuidle_max_substate(xc_handle, &value)) )
> +        return ret;
> +
> +    printf("Max possible sub C-state: %d\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_substate(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_substate_func(int argc, char *argv[])
> +{
> +    int value;
> +
> +    if ( argc != 1 || sscanf(argv[0], "%d", &value) != 1 || value < 0 )
> +    {
> +        fprintf(stderr, "Missing or invalid argument(s)\n");
> +        exit(EINVAL);
> +    }
> +
> +    if ( !xc_set_cpuidle_max_substate(xc_handle, (uint32_t)value) )
> +        printf("set max_substate to %d succeeded\n", value);
> +    else
> +        fprintf(stderr, "set max_substate to %d failed (%d - %s)\n",
> +                value, errno, strerror(errno));
> +}
> +

Please don't inherit the brokenness of set_max_cstate_func() for the
sake of copying it.

"value "should be unsigned all the way through, and you should use %u in
place of %d everywhere.

~Andrew

>  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-substate", set_max_substate_func},
>      { "enable-turbo-mode", enable_turbo_mode },
>      { "disable-turbo-mode", disable_turbo_mode },
>  };

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

[Jan Beulich]

>>> On 18.06.14 at 17:04, <ross.lagerwall@citrix.com> wrote:
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -106,6 +106,8 @@ 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_substate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;

For max_cstate this same initializer makes sense, but for the maximum
sub-state it doesn't. I supposed this should simply be UINT_MAX.

> +integer_param("max_substate", max_substate);

As said before, I'd much prefer staying with just max_cstate. Simply
make this a custom_param() and have the sub-state be specified
after an optional comma.

Furthermore you then only alter mwait-idle.c to obey this, while you
leave the ACPI-based driver alone.

Jan

Re: [PATCH 3/4] tools/libxc: Alow getting and setting the max sub C-State

[Jan Beulich]

>>> On 18.06.14 at 17:04, <ross.lagerwall@citrix.com> wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -366,6 +366,10 @@ struct xen_sysctl_pm_op {
>      #define XEN_SYSCTL_pm_op_enable_turbo               0x26
>      #define XEN_SYSCTL_pm_op_disable_turbo              0x27
>  
> +    /* cpuidle max_substate access command */
> +    #define XEN_SYSCTL_pm_op_get_max_substate     0x28
> +    #define XEN_SYSCTL_pm_op_set_max_substate     0x29
> +
>      uint32_t cmd;
>      uint32_t cpuid;
>      union {
> @@ -376,6 +380,8 @@ struct xen_sysctl_pm_op {
>          uint32_t                    set_sched_opt_smt;
>          uint32_t                    get_max_cstate;
>          uint32_t                    set_max_cstate;
> +        uint32_t                    get_max_substate;
> +        uint32_t                    set_max_substate;
>          uint32_t                    get_vcpu_migration_delay;
>          uint32_t                    set_vcpu_migration_delay;
>      } u;

I'm really uncertain about adding whole new sub-ops and union fields
for this. For simplicity's sake I'd simply re-use the ops and fields we
already have (with cpuid, which is unused anyway, distinguishing
between main and sub-states).

But then again now that I look at this from an interface perspective
I wonder whether sooner or later we wouldn't want to be able to
specify maximum sub-states per main state. That surely would
require separate new sub-ops (as it would likely require handles to
be passed in).

Jan