[PATCH v2 0/3] mwait support for AMD processors

[PATCH v2 0/3] mwait support for AMD processors

[Woods, Brian]

This patch series add support and enablement for mwait on AMD Naples
and Rome processors.  Newer AMD processors support mwait, but only for
c1, and for c2 halt is used.  The mwait-idle driver is modified to be
able to use both mwait and halt for idling.

Brian Woods (3):
  mwait-idle: add support for using halt
  mwait-idle: add support for AMD processors
  mwait-idle: add enablement for AMD Naples and Rome

 xen/arch/x86/acpi/cpu_idle.c  |  2 +-
 xen/arch/x86/cpu/mwait-idle.c | 62 ++++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/cpuidle.h |  1 +
 3 files changed, 57 insertions(+), 8 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/3] mwait-idle: add support for using halt

[Woods, Brian]

From: Brian Woods <brian.woods@amd.com>

Some AMD processors can use a mixture of mwait and halt for accessing
various c-states.  In preparation for adding support for AMD processors,
update the mwait-idle driver to optionally use halt.

Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/acpi/cpu_idle.c  |  2 +-
 xen/arch/x86/cpu/mwait-idle.c | 19 +++++++++++++------
 xen/include/asm-x86/cpuidle.h |  1 +
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 654de24f40..b45824d343 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -439,7 +439,7 @@ static void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
     mwait_idle_with_hints(cx->address, MWAIT_ECX_INTERRUPT_BREAK);
 }
 
-static void acpi_idle_do_entry(struct acpi_processor_cx *cx)
+void acpi_idle_do_entry(struct acpi_processor_cx *cx)
 {
     struct cpu_info *info = get_cpu_info();
 
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index f89c52f256..b9c7f75882 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -103,6 +103,11 @@ static const struct cpuidle_state {
 
 #define CPUIDLE_FLAG_DISABLED		0x1
 /*
+ * On certain AMD families that support mwait, only c1 can be reached by
+ * mwait and to reach c2, halt has to be used.
+ */
+#define CPUIDLE_FLAG_USE_HALT		0x2
+/*
  * Set this flag for states where the HW flushes the TLB for us
  * and so we don't need cross-calls to keep it consistent.
  * If this flag is set, SW flushes the TLB, so even if the
@@ -784,7 +789,7 @@ static void mwait_idle(void)
 	update_last_cx_stat(power, cx, before);
 
 	if (cpu_is_haltable(cpu))
-		mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
+		acpi_idle_do_entry(cx);
 
 	after = cpuidle_get_tick();
 
@@ -1184,8 +1189,9 @@ static int mwait_idle_cpu_init(struct notifier_block *nfb,
 	for (cstate = 0; cpuidle_state_table[cstate].target_residency; ++cstate) {
 		unsigned int num_substates, hint, state;
 		struct acpi_processor_cx *cx;
+		const unsigned int cflags = cpuidle_state_table[cstate].flags;
 
-		hint = flg2MWAIT(cpuidle_state_table[cstate].flags);
+		hint = flg2MWAIT(cflags);
 		state = MWAIT_HINT2CSTATE(hint) + 1;
 
 		if (state > max_cstate) {
@@ -1196,13 +1202,13 @@ static int mwait_idle_cpu_init(struct notifier_block *nfb,
 		/* Number of sub-states for this state in CPUID.MWAIT. */
 		num_substates = (mwait_substates >> (state * 4))
 		                & MWAIT_SUBSTATE_MASK;
+
 		/* If NO sub-states for this state in CPUID, skip it. */
-		if (num_substates == 0)
+		if (num_substates == 0 && !(cflags & CPUIDLE_FLAG_USE_HALT))
 			continue;
 
 		/* if state marked as disabled, skip it */
-		if (cpuidle_state_table[cstate].flags &
-		    CPUIDLE_FLAG_DISABLED) {
+		if (cflags & CPUIDLE_FLAG_DISABLED) {
 			printk(XENLOG_DEBUG PREFIX "state %s is disabled",
 			       cpuidle_state_table[cstate].name);
 			continue;
@@ -1221,7 +1227,8 @@ static int mwait_idle_cpu_init(struct notifier_block *nfb,
 		cx = dev->states + dev->count;
 		cx->type = state;
 		cx->address = hint;
-		cx->entry_method = ACPI_CSTATE_EM_FFH;
+		cx->entry_method = cflags & CPUIDLE_FLAG_USE_HALT ?
+		                   ACPI_CSTATE_EM_HALT : ACPI_CSTATE_EM_FFH;
 		cx->latency = cpuidle_state_table[cstate].exit_latency;
 		cx->target_residency =
 			cpuidle_state_table[cstate].target_residency;
diff --git a/xen/include/asm-x86/cpuidle.h b/xen/include/asm-x86/cpuidle.h
index 08da01803f..33c8cf1593 100644
--- a/xen/include/asm-x86/cpuidle.h
+++ b/xen/include/asm-x86/cpuidle.h
@@ -18,6 +18,7 @@ extern uint64_t (*cpuidle_get_tick)(void);
 
 int mwait_idle_init(struct notifier_block *);
 int cpuidle_init_cpu(unsigned int cpu);
+void acpi_idle_do_entry(struct acpi_processor_cx *cx);
 void default_dead_idle(void);
 void acpi_dead_idle(void);
 void trace_exit_reason(u32 *irq_traced);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/3] mwait-idle: add support for AMD processors

[Woods, Brian]

From: Brian Woods <brian.woods@amd.com>

Newer AMD processors (F17h) have mwait support which is compatible with
Intel.  Add some checks to make sure vendor specific code is run
correctly and some infrastructure to facilitate adding AMD processors.

This is done so that Xen will not be reliant on dom0 passing the parsed
ACPI tables back since Xen doesn't have an AML interpreter. This can be
unreliable or broken in some cases.

Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/cpu/mwait-idle.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index b9c7f75882..58629f1c29 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -964,6 +964,13 @@ static const struct x86_cpu_id intel_idle_ids[] __initconstrel = {
 	{}
 };
 
+#define ACPU(family, model, cpu) \
+	{ X86_VENDOR_AMD, family, model, X86_FEATURE_ALWAYS, &idle_cpu_##cpu}
+
+static const struct x86_cpu_id amd_idle_ids[] __initconstrel = {
+	{}
+};
+
 /*
  * ivt_idle_state_table_update(void)
  *
@@ -1100,6 +1107,9 @@ static void __init sklh_idle_state_table_update(void)
  */
 static void __init mwait_idle_state_table_update(void)
 {
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
+		return;
+
 	switch (boot_cpu_data.x86_model) {
 	case 0x3e: /* IVT */
 		ivt_idle_state_table_update();
@@ -1117,7 +1127,16 @@ static void __init mwait_idle_state_table_update(void)
 static int __init mwait_idle_probe(void)
 {
 	unsigned int eax, ebx, ecx;
-	const struct x86_cpu_id *id = x86_match_cpu(intel_idle_ids);
+	const struct x86_cpu_id *id = NULL;
+
+	switch (boot_cpu_data.x86_vendor) {
+	case X86_VENDOR_INTEL:
+		id = x86_match_cpu(intel_idle_ids);
+		break;
+	case X86_VENDOR_AMD:
+		id = x86_match_cpu(amd_idle_ids);
+		break;
+	}
 
 	if (!id) {
 		pr_debug(PREFIX "does not run on family %d model %d\n",
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 3/3] mwait-idle: add enablement for AMD Naples and Rome

[Woods, Brian]

From: Brian Woods <brian.woods@amd.com>

Add the needed data structures for enabling Naples (F17h M01h).  Since
Rome (F17h M31h) has the same c-state latencies and entry methods, the
c-state information can be used for Rome as well.  For both Naples and
Rome, mwait is used for c1 (cc1) and halt is functionally the same as
c2 (cc6).  If c2 (cc6) is disabled in BIOS, then halt functions similar
to c1 (cc1).

Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/cpu/mwait-idle.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 58629f1c29..0d5d4caa4d 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -720,6 +720,22 @@ static const struct cpuidle_state dnv_cstates[] = {
 	{}
 };
 
+static const struct cpuidle_state naples_cstates[] = {
+	{
+		.name = "CC1",
+		.flags = MWAIT2flg(0x00),
+		.exit_latency = 1,
+		.target_residency = 2,
+	},
+	{
+		.name = "CC6",
+		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_USE_HALT,
+		.exit_latency = 400,
+		.target_residency = 1000,
+	},
+	{}
+};
+
 static void mwait_idle(void)
 {
 	unsigned int cpu = smp_processor_id();
@@ -964,10 +980,16 @@ static const struct x86_cpu_id intel_idle_ids[] __initconstrel = {
 	{}
 };
 
+static const struct idle_cpu idle_cpu_naples = {
+	.state_table = naples_cstates,
+};
+
 #define ACPU(family, model, cpu) \
 	{ X86_VENDOR_AMD, family, model, X86_FEATURE_ALWAYS, &idle_cpu_##cpu}
 
 static const struct x86_cpu_id amd_idle_ids[] __initconstrel = {
+	ACPU(0x17, 0x01, naples),
+	ACPU(0x17, 0x31, naples), /* Rome shares the same c-state config */
 	{}
 };
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 0/3] mwait support for AMD processors

[Woods, Brian]

On 3/28/19 10:04 AM, Woods, Brian wrote:
> This patch series add support and enablement for mwait on AMD Naples
> and Rome processors.  Newer AMD processors support mwait, but only for
> c1, and for c2 halt is used.  The mwait-idle driver is modified to be
> able to use both mwait and halt for idling.
> 
> Brian Woods (3):
>    mwait-idle: add support for using halt
>    mwait-idle: add support for AMD processors
>    mwait-idle: add enablement for AMD Naples and Rome
> 
>   xen/arch/x86/acpi/cpu_idle.c  |  2 +-
>   xen/arch/x86/cpu/mwait-idle.c | 62 ++++++++++++++++++++++++++++++++++++++-----
>   xen/include/asm-x86/cpuidle.h |  1 +
>   3 files changed, 57 insertions(+), 8 deletions(-)
> 

Just a ping to hopefully start the discussion that mentioned in the 
community call.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 0/3] mwait support for AMD processors

[Woods, Brian]

On Thu, Mar 28, 2019 at 03:04:32PM +0000, Brian Woods wrote:
> This patch series add support and enablement for mwait on AMD Naples
> and Rome processors.  Newer AMD processors support mwait, but only for
> c1, and for c2 halt is used.  The mwait-idle driver is modified to be
> able to use both mwait and halt for idling.
> 
> Brian Woods (3):
>   mwait-idle: add support for using halt
>   mwait-idle: add support for AMD processors
>   mwait-idle: add enablement for AMD Naples and Rome
> 
>  xen/arch/x86/acpi/cpu_idle.c  |  2 +-
>  xen/arch/x86/cpu/mwait-idle.c | 62 ++++++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-x86/cpuidle.h |  1 +
>  3 files changed, 57 insertions(+), 8 deletions(-)
> 
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Ping for Andy.

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 0/3] mwait support for AMD processors

[Woods, Brian]

On Thu, May 09, 2019 at 05:00:07PM -0500, Brian Woods wrote:
> On Thu, Mar 28, 2019 at 03:04:32PM +0000, Brian Woods wrote:
> > This patch series add support and enablement for mwait on AMD Naples
> > and Rome processors.  Newer AMD processors support mwait, but only for
> > c1, and for c2 halt is used.  The mwait-idle driver is modified to be
> > able to use both mwait and halt for idling.
> > 
> > Brian Woods (3):
> >   mwait-idle: add support for using halt
> >   mwait-idle: add support for AMD processors
> >   mwait-idle: add enablement for AMD Naples and Rome
> > 
> >  xen/arch/x86/acpi/cpu_idle.c  |  2 +-
> >  xen/arch/x86/cpu/mwait-idle.c | 62 ++++++++++++++++++++++++++++++++++++++-----
> >  xen/include/asm-x86/cpuidle.h |  1 +
> >  3 files changed, 57 insertions(+), 8 deletions(-)
> > 
> > -- 
> > 2.11.0
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xenproject.org
> > https://lists.xenproject.org/mailman/listinfo/xen-devel
> 
> Ping for Andy.
> 
> -- 
> Brian Woods

Ping in general...

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 0/3] mwait support for AMD processors

[Rich Persaud]

[-- Attachment #1.1: Type: text/plain, Size: 3101 bytes --]

> On Mar 28, 2019, at 11:04, Woods, Brian <Brian.Woods@amd.com> wrote:
> 
> This patch series add support and enablement for mwait on AMD Naples
> and Rome processors.  Newer AMD processors support mwait, but only for
> c1, and for c2 halt is used.  The mwait-idle driver is modified to be
> able to use both mwait and halt for idling.

Would you mind if I create a Xen Project JIRA ticket, or a wiki page, to track requirements and implementations related to this patch series?

From the initial thread [1]:
>>> On certain AMD families that support mwait, only c1 can be reached by
>>> + * mwait and to reach c2, halt has to be used.
>>> + */
>>> +#define CPUIDLE_FLAG_USE_HALT        0x2
>> 
>> Could you point us at where in the manuals this behavior is described?
>> While PM Vol 2 has a chapter talking about P-states, I can't seem to
>> find any mention of C-states there.
> 
> IIRC it's in the NDA PPR and internally it's in some other documents. 
> We don't have support to use mwait while in CC6 due to caches being 
> turned off etc.  If we did have mwait suport for CC6, we'd use that here 
> (basically mirroring Intel).  Sadly I don't think we have any public 
> information directly detailing this information.  

Can this be documented in the patch comment, or an AMD-specific page on wiki.xenproject.org?  It's a requirement/input to all possible implementations.  

From a comment in the April 2018 Linux patch by Yazen [2]:
> x86/smpboot: Don't use mwait_play_dead() on AMD systems
> Recent AMD systems support using MWAIT for C1 state. However, MWAIT will
> not allow deeper cstates than C1 on current systems.
> 
> play_dead() expects to use the deepest state available.  The deepest state
> available on AMD systems is reached through SystemIO or HALT. If MWAIT is
> available, it is preferred over the other methods, so the CPU never reaches
> the deepest possible state.
> 
> Don't try to use MWAIT to play_dead() on AMD systems. Instead, use CPUIDLE
> to enter the deepest state advertised by firmware. If CPUIDLE is not
> available then fallback to HALT.

For the ticket/wiki: what are the expected benefits of the proposed Xen change?  Would it reduce idle power consumption on Ryzen 1000/2000/3000? Epyc 3000/7000? Any sample data available for idle power before/after the v2 patch?

From a thread [3] posted by Jan this week, "x86/AMD: make C-state handling independent of Dom0":
> The 3rd patch is my counterproposal to Brian's intended abuse (as I would call it) of the mwait-idle driver. 

Do we need a new, patch-independent, thread for convergence of candidate implementations which address the requirements (documented in ticket/wiki)?  Should discussion move from the initial thread [1] to the counter-proposal thread [3]?  Or this thread?

Rich

[1] https://lists.gt.net/xen/devel/545688

[2] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86-urgent-for-linus&id=da6fa7ef67f07108a1b0cb9fd9e7fcaabd39c051

[3] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01894.html


[-- Attachment #1.2: Type: text/html, Size: 6525 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel