[PATCH 0/5] x86/mwait-idle: updates from Linux

[PATCH 0/5] x86/mwait-idle: updates from Linux

[Jan Beulich]

Before the code freeze I thought I'd check the original driver again,
and indeed there were a few changes we want to inherit.

1: mention assumption that WBINVD is not needed
2: add SnowRidge C-state table
3: update ICX C6 data
4: add Icelake-D support
5: adjust the SKX C6 parameters if PC6 is disabled

Jan

[PATCH 1/5] x86/mwait-idle: mention assumption that WBINVD is not needed

[Jan Beulich]

From: Alexander Monakov <amonakov@ispras.ru>

Intel SDM does not explicitly say that entering a C-state via MWAIT will
implicitly flush CPU caches as appropriate for that C-state. However,
documentation for individual Intel CPU generations does mention this
behavior.

Since intel_idle binds to any Intel CPU with MWAIT, list this assumption
of MWAIT behavior.

In passing, reword opening comment to make it clear that the driver can
load on any old and future Intel CPU with MWAIT.

Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[Linux commit: 8bb2e2a887afdf8a39e68fa0dccf82a168aae655]

Dropped "reword opending comment" part - this doesn't apply to our code:
First thing mwait_idle_probe() does is call x86_match_cpu(); we do not
have a 2nd such call looking for just MWAIT (in order to the use _CST
data directly, which we can't get our hands at _CST at this point yet).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -31,6 +31,10 @@
  *
  * Chipset BM_STS (bus master status) bit is a NOP
  *	for preventing entry into deep C-states
+ *
+ * CPU will flush caches as needed when entering a C-state via MWAIT
+ *	(in contrast to entering ACPI C3, in which case the WBINVD
+ *	instruction needs to be executed to flush the caches)
  */
 
 /*

[PATCH 2/5] x86/mwait-idle: add SnowRidge C-state table

[Jan Beulich]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Add C-state table for the SnowRidge SoC which is found on Intel Jacobsville
platforms.

The following has been changed.

 1. C1E latency changed from 10us to 15us. It was measured using the
    open source "wult" tool (the "nic" method, 15us is the 99.99th
    percentile).

 2. C1E power break even changed from 20us to 25us, which may result
    in less C1E residency in some workloads.

 3. C6 latency changed from 50us to 130us. Measured the same way as C1E.

The C6 C-state is supported only by some SnowRidge revisions, so add a C-state
table commentary about this.

On SnowRidge, C6 support is enumerated via the usual mechanism: "mwait" leaf of
the "cpuid" instruction. The 'intel_idle' driver does check this leaf, so even
though C6 is present in the table, the driver will only use it if the CPU does
support it.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[Linux commit: 9cf93f056f783f986c19f40d5304d1bcffa0fc0d]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -742,6 +742,32 @@ static const struct cpuidle_state dnv_cs
 	{}
 };
 
+/*
+ * Note, depending on HW and FW revision, SnowRidge SoC may or may not support
+ * C6, and this is indicated in the CPUID mwait leaf.
+ */
+static const struct cpuidle_state snr_cstates[] = {
+	{
+		.name = "C1",
+		.flags = MWAIT2flg(0x00),
+		.exit_latency = 2,
+		.target_residency = 2,
+	},
+	{
+		.name = "C1E",
+		.flags = MWAIT2flg(0x01),
+		.exit_latency = 15,
+		.target_residency = 25,
+	},
+	{
+		.name = "C6",
+		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
+		.exit_latency = 130,
+		.target_residency = 500,
+	},
+	{}
+};
+
 static void mwait_idle(void)
 {
 	unsigned int cpu = smp_processor_id();
@@ -954,6 +980,11 @@ static const struct idle_cpu idle_cpu_dn
 	.disable_promotion_to_c1e = 1,
 };
 
+static const struct idle_cpu idle_cpu_snr = {
+	.state_table = snr_cstates,
+	.disable_promotion_to_c1e = true,
+};
+
 #define ICPU(model, cpu) \
 	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ALWAYS, &idle_cpu_##cpu}
 
@@ -995,7 +1026,7 @@ static const struct x86_cpu_id intel_idl
 	ICPU(0x5c, bxt),
 	ICPU(0x7a, bxt),
 	ICPU(0x5f, dnv),
-	ICPU(0x86, dnv),
+	ICPU(0x86, snr),
 	{}
 };
 

[PATCH 3/5] x86/mwait-idle: update ICX C6 data

[Jan Beulich]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Change IceLake Xeon C6 latency from 128 us to 170 us. The latency
was measured with the "wult" tool and corresponds to the 99.99th
percentile when measuring with the "nic" method. Note, the 128 us
figure correspond to the median latency, but in intel_idle we use
the "worst case" latency figure instead.

C6 target residency was increased from 384 us to 600 us, which may
result in less C6 residency in some workloads. This value was tested
and compared to values 384, and 1000. Value 600 is a reasonable
tradeoff between power and performance.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Acked-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[Linux commit: d484b8bfc6fa71a088e4ac85d9ce11aa0385867e]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -574,8 +574,8 @@ static const struct cpuidle_state icx_cs
        {
                .name = "C6-ICX",
                .flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
-               .exit_latency = 128,
-               .target_residency = 384,
+               .exit_latency = 170,
+               .target_residency = 600,
        },
        {}
 };

[PATCH 4/5] x86/mwait-idle: add Icelake-D support

[Jan Beulich]

This patch adds Icelake Xeon D support to the intel_idle driver.

Since Icelake D and Icelake SP C-state characteristics the same,
we use Icelake SP C-states table for Icelake D as well.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Acked-by: Chen Yu <yu.c.chen@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[Linux commit: 22141d5f411895bb1b0df2a6b05f702e11e63918]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -1021,6 +1021,7 @@ static const struct x86_cpu_id intel_idl
 	ICPU(0x9e, skl),
 	ICPU(0x55, skx),
 	ICPU(0x6a, icx),
+	ICPU(0x6c, icx),
 	ICPU(0x57, knl),
 	ICPU(0x85, knl),
 	ICPU(0x5c, bxt),

[PATCH 5/5] x86/mwait-idle: adjust the SKX C6 parameters if PC6 is disabled

[Jan Beulich]

From: Chen Yu <yu.c.chen@intel.com>

Because cpuidle assumes worst-case C-state parameters, PC6 parameters
are used for describing C6, which is worst-case for requesting CC6.
When PC6 is enabled, this is appropriate. But if PC6 is disabled
in the BIOS, the exit latency and target residency should be adjusted
accordingly.

Exit latency:
Previously the C6 exit latency was measured as the PC6 exit latency.
With PC6 disabled, the C6 exit latency should be the one of CC6.

Target residency:
With PC6 disabled, the idle duration within [CC6, PC6) would make the
idle governor choose C1E over C6. This would cause low energy-efficiency.
We should lower the bar to request C6 when PC6 is disabled.

To fill this gap, check if PC6 is disabled in the BIOS in the
MSR_PKG_CST_CONFIG_CONTROL(0xe2) register. If so, use the CC6 exit latency
for C6 and set target_residency to 3 times of the new exit latency. [This
is consistent with how intel_idle driver uses _CST to calculate the
target_residency.] As a result, the OS would be more likely to choose C6
over C1E when PC6 is disabled, which is reasonable, because if C6 is
enabled, it implies that the user cares about energy, so choosing C6 more
frequently makes sense.

The new CC6 exit latency of 92us was measured with wult[1] on SKX via NIC
wakeup as the 99.99th percentile. Also CLX and CPX both have the same CPU
model number as SkX, but their CC6 exit latencies are similar to the SKX
one, 96us and 89us respectively, so reuse the SKX value for them.

There is a concern that it might be better to use a more generic approach
instead of optimizing every platform. However, if the required code
complexity and different PC6 bit interpretation on different platforms
are taken into account, tuning the code per platform seems to be an
acceptable tradeoff.

Link: https://intel.github.io/wult/ # [1]
Suggested-by: Len Brown <len.brown@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
Reviewed-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
[ rjw: Subject and changelog edits ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[Linux commit: 64233338499126c5c31e07165735ab5441c7e45a]

Pull in Linux'es MSR_PKG_CST_CONFIG_CONTROL. Alongside the dropping of
"const" from skx_cstates[] add __read_mostly, and extend that to other
similar non-const tables.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -484,7 +484,7 @@ static const struct cpuidle_state bdw_cs
 	{}
 };
 
-static struct cpuidle_state skl_cstates[] = {
+static struct cpuidle_state __read_mostly skl_cstates[] = {
 	{
 		.name = "C1-SKL",
 		.flags = MWAIT2flg(0x00),
@@ -536,7 +536,7 @@ static struct cpuidle_state skl_cstates[
 	{}
 };
 
-static const struct cpuidle_state skx_cstates[] = {
+static struct cpuidle_state __read_mostly skx_cstates[] = {
 	{
 		.name = "C1-SKX",
 		.flags = MWAIT2flg(0x00),
@@ -674,7 +674,7 @@ static const struct cpuidle_state knl_cs
 	{}
 };
 
-static struct cpuidle_state bxt_cstates[] = {
+static struct cpuidle_state __read_mostly bxt_cstates[] = {
 	{
 		.name = "C1-BXT",
 		.flags = MWAIT2flg(0x00),
@@ -870,9 +870,9 @@ static void auto_demotion_disable(void *
 {
 	u64 msr_bits;
 
-	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
 	msr_bits &= ~(icpu->auto_demotion_disable_flags);
-	wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
 }
 
 static void byt_auto_demotion_disable(void *dummy)
@@ -1141,7 +1141,7 @@ static void __init sklh_idle_state_table
 	if ((mwait_substates & (MWAIT_CSTATE_MASK << 28)) == 0)
 		return;
 
-	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr);
+	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
 
 	/* PC10 is not enabled in PKG C-state limit */
 	if ((msr & 0xF) != 8)
@@ -1161,6 +1161,36 @@ static void __init sklh_idle_state_table
 }
 
 /*
+ * skx_idle_state_table_update - Adjust the Sky Lake/Cascade Lake
+ * idle states table.
+ */
+static void __init skx_idle_state_table_update(void)
+{
+	unsigned long long msr;
+
+	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
+
+	/*
+	 * 000b: C0/C1 (no package C-state support)
+	 * 001b: C2
+	 * 010b: C6 (non-retention)
+	 * 011b: C6 (retention)
+	 * 111b: No Package C state limits.
+	 */
+	if ((msr & 0x7) < 2) {
+		/*
+		 * Uses the CC6 + PC0 latency and 3 times of
+		 * latency for target_residency if the PC6
+		 * is disabled in BIOS. This is consistent
+		 * with how intel_idle driver uses _CST
+		 * to set the target_residency.
+		 */
+		skx_cstates[2].exit_latency = 92;
+		skx_cstates[2].target_residency = 276;
+	}
+}
+
+/*
  * mwait_idle_state_table_update()
  *
  * Update the default state_table for this CPU-id
@@ -1178,6 +1208,9 @@ static void __init mwait_idle_state_tabl
 	case 0x5e: /* SKL-H */
 		sklh_idle_state_table_update();
 		break;
+	case 0x55: /* SKL-X */
+		skx_idle_state_table_update();
+		break;
  	}
 }
 
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -45,6 +45,13 @@
 #define MSR_CORE_CAPABILITIES               0x000000cf
 #define  CORE_CAPS_SPLITLOCK_DETECT         (_AC(1, ULL) <<  5)
 
+#define MSR_PKG_CST_CONFIG_CONTROL          0x000000e2
+#define  NHM_C3_AUTO_DEMOTE                 (_AC(1, ULL) << 25)
+#define  NHM_C1_AUTO_DEMOTE                 (_AC(1, ULL) << 26)
+#define  ATM_LNC_C6_AUTO_DEMOTE             (_AC(1, ULL) << 25)
+#define  SNB_C3_AUTO_UNDEMOTE               (_AC(1, ULL) << 27)
+#define  SNB_C1_AUTO_UNDEMOTE               (_AC(1, ULL) << 28)
+
 #define MSR_ARCH_CAPABILITIES               0x0000010a
 #define  ARCH_CAPS_RDCL_NO                  (_AC(1, ULL) <<  0)
 #define  ARCH_CAPS_IBRS_ALL                 (_AC(1, ULL) <<  1)
@@ -175,11 +182,6 @@
 #define MSR_IA32_A_PERFCTR0		0x000004c1
 #define MSR_FSB_FREQ			0x000000cd
 
-#define MSR_NHM_SNB_PKG_CST_CFG_CTL	0x000000e2
-#define NHM_C3_AUTO_DEMOTE		(1UL << 25)
-#define NHM_C1_AUTO_DEMOTE		(1UL << 26)
-#define ATM_LNC_C6_AUTO_DEMOTE		(1UL << 25)
-
 #define MSR_MTRRcap			0x000000fe
 #define MTRRcap_VCNT			0x000000ff
 

Ping: [PATCH 0/5] x86/mwait-idle: updates from Linux

[Jan Beulich]

On 06.09.2021 14:58, Jan Beulich wrote:
> Before the code freeze I thought I'd check the original driver again,
> and indeed there were a few changes we want to inherit.
> 
> 1: mention assumption that WBINVD is not needed
> 2: add SnowRidge C-state table
> 3: update ICX C6 data
> 4: add Icelake-D support
> 5: adjust the SKX C6 parameters if PC6 is disabled

Ping?

Thanks, Jan

Ping: [PATCH 0/5] x86/mwait-idle: updates from Linux

[Jan Beulich]

On 06.09.2021 14:58, Jan Beulich wrote:
> Before the code freeze I thought I'd check the original driver again,
> and indeed there were a few changes we want to inherit.
> 
> 1: mention assumption that WBINVD is not needed
> 2: add SnowRidge C-state table
> 3: update ICX C6 data
> 4: add Icelake-D support
> 5: adjust the SKX C6 parameters if PC6 is disabled

May I ask for an ack or otherwise? It's sad enough that this didn't
make 4.16.

Thanks, Jan

Re: [PATCH 2/5] x86/mwait-idle: add SnowRidge C-state table

[Roger Pau Monné]

On Mon, Sep 06, 2021 at 03:00:46PM +0200, Jan Beulich wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Add C-state table for the SnowRidge SoC which is found on Intel Jacobsville
> platforms.
> 
> The following has been changed.
> 
>  1. C1E latency changed from 10us to 15us. It was measured using the
>     open source "wult" tool (the "nic" method, 15us is the 99.99th
>     percentile).
> 
>  2. C1E power break even changed from 20us to 25us, which may result
>     in less C1E residency in some workloads.
> 
>  3. C6 latency changed from 50us to 130us. Measured the same way as C1E.
> 
> The C6 C-state is supported only by some SnowRidge revisions, so add a C-state
> table commentary about this.
> 
> On SnowRidge, C6 support is enumerated via the usual mechanism: "mwait" leaf of
> the "cpuid" instruction. The 'intel_idle' driver does check this leaf, so even
> though C6 is present in the table, the driver will only use it if the CPU does
> support it.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> [Linux commit: 9cf93f056f783f986c19f40d5304d1bcffa0fc0d]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -742,6 +742,32 @@ static const struct cpuidle_state dnv_cs
>  	{}
>  };
>  
> +/*
> + * Note, depending on HW and FW revision, SnowRidge SoC may or may not support
> + * C6, and this is indicated in the CPUID mwait leaf.
> + */
> +static const struct cpuidle_state snr_cstates[] = {
> +	{
> +		.name = "C1",

We usually use names like "C1-SNR" or similar in other cpuidle_state
structs. Is using plain "C1" intentional here?

> +		.flags = MWAIT2flg(0x00),
> +		.exit_latency = 2,
> +		.target_residency = 2,
> +	},
> +	{
> +		.name = "C1E",
> +		.flags = MWAIT2flg(0x01),
> +		.exit_latency = 15,
> +		.target_residency = 25,
> +	},
> +	{
> +		.name = "C6",
> +		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
> +		.exit_latency = 130,
> +		.target_residency = 500,
> +	},
> +	{}
> +};
> +
>  static void mwait_idle(void)
>  {
>  	unsigned int cpu = smp_processor_id();
> @@ -954,6 +980,11 @@ static const struct idle_cpu idle_cpu_dn
>  	.disable_promotion_to_c1e = 1,
>  };
>  
> +static const struct idle_cpu idle_cpu_snr = {
> +	.state_table = snr_cstates,
> +	.disable_promotion_to_c1e = true,

This likely wants to be 1 because the type is bool_t.

Thanks, Roger.

Re: [PATCH 1/5] x86/mwait-idle: mention assumption that WBINVD is not needed

[Roger Pau Monné]

On Mon, Sep 06, 2021 at 02:59:46PM +0200, Jan Beulich wrote:
> From: Alexander Monakov <amonakov@ispras.ru>
> 
> Intel SDM does not explicitly say that entering a C-state via MWAIT will
> implicitly flush CPU caches as appropriate for that C-state. However,
> documentation for individual Intel CPU generations does mention this
> behavior.
> 
> Since intel_idle binds to any Intel CPU with MWAIT, list this assumption
> of MWAIT behavior.
> 
> In passing, reword opening comment to make it clear that the driver can
> load on any old and future Intel CPU with MWAIT.
> 
> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> [Linux commit: 8bb2e2a887afdf8a39e68fa0dccf82a168aae655]
> 
> Dropped "reword opending comment" part - this doesn't apply to our code:
> First thing mwait_idle_probe() does is call x86_match_cpu(); we do not
> have a 2nd such call looking for just MWAIT (in order to the use _CST
> data directly, which we can't get our hands at _CST at this point yet).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 3/5] x86/mwait-idle: update ICX C6 data

[Roger Pau Monné]

On Mon, Sep 06, 2021 at 03:01:12PM +0200, Jan Beulich wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Change IceLake Xeon C6 latency from 128 us to 170 us. The latency
> was measured with the "wult" tool and corresponds to the 99.99th
> percentile when measuring with the "nic" method. Note, the 128 us
> figure correspond to the median latency, but in intel_idle we use
> the "worst case" latency figure instead.
> 
> C6 target residency was increased from 384 us to 600 us, which may
> result in less C6 residency in some workloads. This value was tested
> and compared to values 384, and 1000. Value 600 is a reasonable
> tradeoff between power and performance.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Acked-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> [Linux commit: d484b8bfc6fa71a088e4ac85d9ce11aa0385867e]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

AFAICT those values are all from measurements, and not in any manual
or specification?

Thanks, Roger.

Re: [PATCH 4/5] x86/mwait-idle: add Icelake-D support

[Roger Pau Monné]

On Mon, Sep 06, 2021 at 03:01:46PM +0200, Jan Beulich wrote:
> This patch adds Icelake Xeon D support to the intel_idle driver.
> 
> Since Icelake D and Icelake SP C-state characteristics the same,
> we use Icelake SP C-states table for Icelake D as well.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Acked-by: Chen Yu <yu.c.chen@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> [Linux commit: 22141d5f411895bb1b0df2a6b05f702e11e63918]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 5/5] x86/mwait-idle: adjust the SKX C6 parameters if PC6 is disabled

[Roger Pau Monné]

On Mon, Sep 06, 2021 at 03:02:12PM +0200, Jan Beulich wrote:
> From: Chen Yu <yu.c.chen@intel.com>
> 
> Because cpuidle assumes worst-case C-state parameters, PC6 parameters
> are used for describing C6, which is worst-case for requesting CC6.
> When PC6 is enabled, this is appropriate. But if PC6 is disabled
> in the BIOS, the exit latency and target residency should be adjusted
> accordingly.
> 
> Exit latency:
> Previously the C6 exit latency was measured as the PC6 exit latency.
> With PC6 disabled, the C6 exit latency should be the one of CC6.
> 
> Target residency:
> With PC6 disabled, the idle duration within [CC6, PC6) would make the
> idle governor choose C1E over C6. This would cause low energy-efficiency.
> We should lower the bar to request C6 when PC6 is disabled.
> 
> To fill this gap, check if PC6 is disabled in the BIOS in the
> MSR_PKG_CST_CONFIG_CONTROL(0xe2) register. If so, use the CC6 exit latency
> for C6 and set target_residency to 3 times of the new exit latency. [This
> is consistent with how intel_idle driver uses _CST to calculate the
> target_residency.] As a result, the OS would be more likely to choose C6
> over C1E when PC6 is disabled, which is reasonable, because if C6 is
> enabled, it implies that the user cares about energy, so choosing C6 more
> frequently makes sense.
> 
> The new CC6 exit latency of 92us was measured with wult[1] on SKX via NIC
> wakeup as the 99.99th percentile. Also CLX and CPX both have the same CPU
> model number as SkX, but their CC6 exit latencies are similar to the SKX
> one, 96us and 89us respectively, so reuse the SKX value for them.
> 
> There is a concern that it might be better to use a more generic approach
> instead of optimizing every platform. However, if the required code
> complexity and different PC6 bit interpretation on different platforms
> are taken into account, tuning the code per platform seems to be an
> acceptable tradeoff.
> 
> Link: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fintel.github.io%2Fwult%2F&amp;data=04%7C01%7Croger.pau%40citrix.com%7Ccdf115e71eb14429868408d97136a902%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637665301851513469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=D9sIoe%2BFAvia3OuRsT19VAxkTqrlnPHnKqTSKiW6pRM%3D&amp;reserved=0 # [1]
> Suggested-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> Reviewed-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> [ rjw: Subject and changelog edits ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> [Linux commit: 64233338499126c5c31e07165735ab5441c7e45a]
> 
> Pull in Linux'es MSR_PKG_CST_CONFIG_CONTROL. Alongside the dropping of
> "const" from skx_cstates[] add __read_mostly, and extend that to other
> similar non-const tables.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> --- a/xen/arch/x86/cpu/mwait-idle.c
> +++ b/xen/arch/x86/cpu/mwait-idle.c
> @@ -484,7 +484,7 @@ static const struct cpuidle_state bdw_cs
>  	{}
>  };
>  
> -static struct cpuidle_state skl_cstates[] = {
> +static struct cpuidle_state __read_mostly skl_cstates[] = {
>  	{
>  		.name = "C1-SKL",
>  		.flags = MWAIT2flg(0x00),
> @@ -536,7 +536,7 @@ static struct cpuidle_state skl_cstates[
>  	{}
>  };
>  
> -static const struct cpuidle_state skx_cstates[] = {
> +static struct cpuidle_state __read_mostly skx_cstates[] = {
>  	{
>  		.name = "C1-SKX",
>  		.flags = MWAIT2flg(0x00),
> @@ -674,7 +674,7 @@ static const struct cpuidle_state knl_cs
>  	{}
>  };
>  
> -static struct cpuidle_state bxt_cstates[] = {
> +static struct cpuidle_state __read_mostly bxt_cstates[] = {
>  	{
>  		.name = "C1-BXT",
>  		.flags = MWAIT2flg(0x00),
> @@ -870,9 +870,9 @@ static void auto_demotion_disable(void *
>  {
>  	u64 msr_bits;
>  
> -	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
> +	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
>  	msr_bits &= ~(icpu->auto_demotion_disable_flags);
> -	wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
> +	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
>  }
>  
>  static void byt_auto_demotion_disable(void *dummy)
> @@ -1141,7 +1141,7 @@ static void __init sklh_idle_state_table
>  	if ((mwait_substates & (MWAIT_CSTATE_MASK << 28)) == 0)
>  		return;
>  
> -	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr);
> +	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
>  
>  	/* PC10 is not enabled in PKG C-state limit */
>  	if ((msr & 0xF) != 8)
> @@ -1161,6 +1161,36 @@ static void __init sklh_idle_state_table
>  }
>  
>  /*
> + * skx_idle_state_table_update - Adjust the Sky Lake/Cascade Lake
> + * idle states table.
> + */
> +static void __init skx_idle_state_table_update(void)
> +{
> +	unsigned long long msr;
> +
> +	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
> +
> +	/*
> +	 * 000b: C0/C1 (no package C-state support)
> +	 * 001b: C2
> +	 * 010b: C6 (non-retention)
> +	 * 011b: C6 (retention)
> +	 * 111b: No Package C state limits.
> +	 */
> +	if ((msr & 0x7) < 2) {

I wouldn't mind adding this mask field to msr-index.h.

Thanks, Roger.

Re: [PATCH 2/5] x86/mwait-idle: add SnowRidge C-state table

[Jan Beulich]

On 18.01.2022 11:17, Roger Pau Monné wrote:
> On Mon, Sep 06, 2021 at 03:00:46PM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/mwait-idle.c
>> +++ b/xen/arch/x86/cpu/mwait-idle.c
>> @@ -742,6 +742,32 @@ static const struct cpuidle_state dnv_cs
>>  	{}
>>  };
>>  
>> +/*
>> + * Note, depending on HW and FW revision, SnowRidge SoC may or may not support
>> + * C6, and this is indicated in the CPUID mwait leaf.
>> + */
>> +static const struct cpuidle_state snr_cstates[] = {
>> +	{
>> +		.name = "C1",
> 
> We usually use names like "C1-SNR" or similar in other cpuidle_state
> structs. Is using plain "C1" intentional here?

I don't know. We're simply following the Linux side change. If we're
unhappy with their naming (it indeed looks inconsistent), then we
ought to make a separate patch on top (and maybe submit that also to
Linux).

>> @@ -954,6 +980,11 @@ static const struct idle_cpu idle_cpu_dn
>>  	.disable_promotion_to_c1e = 1,
>>  };
>>  
>> +static const struct idle_cpu idle_cpu_snr = {
>> +	.state_table = snr_cstates,
>> +	.disable_promotion_to_c1e = true,
> 
> This likely wants to be 1 because the type is bool_t.

bool_t is just an alias of bool, so "true" ought to be fine. We may
want to follow Linux in switching to bool altogether - iirc we didn't
have bool yet at the time the driver (or the first commit needing it)
was ported. I guess I'll make a patch ...

Jan

Re: [PATCH 3/5] x86/mwait-idle: update ICX C6 data

[Jan Beulich]

On 18.01.2022 11:20, Roger Pau Monné wrote:
> On Mon, Sep 06, 2021 at 03:01:12PM +0200, Jan Beulich wrote:
>> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>>
>> Change IceLake Xeon C6 latency from 128 us to 170 us. The latency
>> was measured with the "wult" tool and corresponds to the 99.99th
>> percentile when measuring with the "nic" method. Note, the 128 us
>> figure correspond to the median latency, but in intel_idle we use
>> the "worst case" latency figure instead.
>>
>> C6 target residency was increased from 384 us to 600 us, which may
>> result in less C6 residency in some workloads. This value was tested
>> and compared to values 384, and 1000. Value 600 is a reasonable
>> tradeoff between power and performance.
>>
>> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> Acked-by: Zhang Rui <rui.zhang@intel.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> [Linux commit: d484b8bfc6fa71a088e4ac85d9ce11aa0385867e]
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> AFAICT those values are all from measurements, and not in any manual
> or specification?

That's my understanding of the description, yes.

Jan

Re: [PATCH 5/5] x86/mwait-idle: adjust the SKX C6 parameters if PC6 is disabled

[Jan Beulich]

On 18.01.2022 11:48, Roger Pau Monné wrote:
> On Mon, Sep 06, 2021 at 03:02:12PM +0200, Jan Beulich wrote:
>> From: Chen Yu <yu.c.chen@intel.com>
>>
>> Because cpuidle assumes worst-case C-state parameters, PC6 parameters
>> are used for describing C6, which is worst-case for requesting CC6.
>> When PC6 is enabled, this is appropriate. But if PC6 is disabled
>> in the BIOS, the exit latency and target residency should be adjusted
>> accordingly.
>>
>> Exit latency:
>> Previously the C6 exit latency was measured as the PC6 exit latency.
>> With PC6 disabled, the C6 exit latency should be the one of CC6.
>>
>> Target residency:
>> With PC6 disabled, the idle duration within [CC6, PC6) would make the
>> idle governor choose C1E over C6. This would cause low energy-efficiency.
>> We should lower the bar to request C6 when PC6 is disabled.
>>
>> To fill this gap, check if PC6 is disabled in the BIOS in the
>> MSR_PKG_CST_CONFIG_CONTROL(0xe2) register. If so, use the CC6 exit latency
>> for C6 and set target_residency to 3 times of the new exit latency. [This
>> is consistent with how intel_idle driver uses _CST to calculate the
>> target_residency.] As a result, the OS would be more likely to choose C6
>> over C1E when PC6 is disabled, which is reasonable, because if C6 is
>> enabled, it implies that the user cares about energy, so choosing C6 more
>> frequently makes sense.
>>
>> The new CC6 exit latency of 92us was measured with wult[1] on SKX via NIC
>> wakeup as the 99.99th percentile. Also CLX and CPX both have the same CPU
>> model number as SkX, but their CC6 exit latencies are similar to the SKX
>> one, 96us and 89us respectively, so reuse the SKX value for them.
>>
>> There is a concern that it might be better to use a more generic approach
>> instead of optimizing every platform. However, if the required code
>> complexity and different PC6 bit interpretation on different platforms
>> are taken into account, tuning the code per platform seems to be an
>> acceptable tradeoff.
>>
>> Link: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fintel.github.io%2Fwult%2F&amp;data=04%7C01%7Croger.pau%40citrix.com%7Ccdf115e71eb14429868408d97136a902%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637665301851513469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=D9sIoe%2BFAvia3OuRsT19VAxkTqrlnPHnKqTSKiW6pRM%3D&amp;reserved=0 # [1]
>> Suggested-by: Len Brown <len.brown@intel.com>
>> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
>> Reviewed-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> [ rjw: Subject and changelog edits ]
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> [Linux commit: 64233338499126c5c31e07165735ab5441c7e45a]
>>
>> Pull in Linux'es MSR_PKG_CST_CONFIG_CONTROL. Alongside the dropping of
>> "const" from skx_cstates[] add __read_mostly, and extend that to other
>> similar non-const tables.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> @@ -1161,6 +1161,36 @@ static void __init sklh_idle_state_table
>>  }
>>  
>>  /*
>> + * skx_idle_state_table_update - Adjust the Sky Lake/Cascade Lake
>> + * idle states table.
>> + */
>> +static void __init skx_idle_state_table_update(void)
>> +{
>> +	unsigned long long msr;
>> +
>> +	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
>> +
>> +	/*
>> +	 * 000b: C0/C1 (no package C-state support)
>> +	 * 001b: C2
>> +	 * 010b: C6 (non-retention)
>> +	 * 011b: C6 (retention)
>> +	 * 111b: No Package C state limits.
>> +	 */
>> +	if ((msr & 0x7) < 2) {
> 
> I wouldn't mind adding this mask field to msr-index.h.

This wouldn't buy us much since - as per the original Linux change - the
manifest constant then still wouldn't be used here.

Jan

Re: [PATCH 2/5] x86/mwait-idle: add SnowRidge C-state table

[Roger Pau Monné]

On Tue, Jan 18, 2022 at 03:05:54PM +0100, Jan Beulich wrote:
> On 18.01.2022 11:17, Roger Pau Monné wrote:
> > On Mon, Sep 06, 2021 at 03:00:46PM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/cpu/mwait-idle.c
> >> +++ b/xen/arch/x86/cpu/mwait-idle.c
> >> @@ -742,6 +742,32 @@ static const struct cpuidle_state dnv_cs
> >>  	{}
> >>  };
> >>  
> >> +/*
> >> + * Note, depending on HW and FW revision, SnowRidge SoC may or may not support
> >> + * C6, and this is indicated in the CPUID mwait leaf.
> >> + */
> >> +static const struct cpuidle_state snr_cstates[] = {
> >> +	{
> >> +		.name = "C1",
> > 
> > We usually use names like "C1-SNR" or similar in other cpuidle_state
> > structs. Is using plain "C1" intentional here?
> 
> I don't know. We're simply following the Linux side change. If we're
> unhappy with their naming (it indeed looks inconsistent), then we
> ought to make a separate patch on top (and maybe submit that also to
> Linux).

Looks like at some point Linux dropped the '-SNR' and similar suffixes
from the state names, so we should likely import that patch as a
pre-req for consistency? It's commit:

de09cdd09fa1 intel_idle: stop exposing platform acronyms in sysfs

> 
> >> @@ -954,6 +980,11 @@ static const struct idle_cpu idle_cpu_dn
> >>  	.disable_promotion_to_c1e = 1,
> >>  };
> >>  
> >> +static const struct idle_cpu idle_cpu_snr = {
> >> +	.state_table = snr_cstates,
> >> +	.disable_promotion_to_c1e = true,
> > 
> > This likely wants to be 1 because the type is bool_t.
> 
> bool_t is just an alias of bool, so "true" ought to be fine. We may
> want to follow Linux in switching to bool altogether - iirc we didn't
> have bool yet at the time the driver (or the first commit needing it)
> was ported. I guess I'll make a patch ...

OK, thanks!

I guess for the patch itself then:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Would be nice to get those things fixed for consistency.

Roger.