[PATCH 0/3] mwait support for AMD processors

[PATCH 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/cpu/mwait-idle.c | 87 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 78 insertions(+), 9 deletions(-)

-- 
2.11.0


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

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

[Woods, Brian]

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/cpu/mwait-idle.c | 40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index f89c52f256..a063e39d60 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
@@ -783,8 +788,23 @@ 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);
+	if (cpu_is_haltable(cpu)) {
+		struct cpu_info *info;
+		switch (cx->entry_method) {
+		case ACPI_CSTATE_EM_FFH:
+			mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
+			break;
+		case ACPI_CSTATE_EM_HALT:
+			info = get_cpu_info();
+			spec_ctrl_enter_idle(info);
+			safe_halt();
+			spec_ctrl_exit_idle(info);
+			local_irq_disable();
+			break;
+		default:
+			printk(XENLOG_ERR PREFIX "unknown entry method %d\n", cx->entry_method);
+		}
+	}
 
 	after = cpuidle_get_tick();
 
@@ -1184,8 +1204,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 flags = cpuidle_state_table[cstate].flags;
 
-		hint = flg2MWAIT(cpuidle_state_table[cstate].flags);
+		hint = flg2MWAIT(flags);
 		state = MWAIT_HINT2CSTATE(hint) + 1;
 
 		if (state > max_cstate) {
@@ -1196,13 +1217,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 && !(flags & CPUIDLE_FLAG_USE_HALT))
 			continue;
 
 		/* if state marked as disabled, skip it */
-		if (cpuidle_state_table[cstate].flags &
-		    CPUIDLE_FLAG_DISABLED) {
+		if (flags & CPUIDLE_FLAG_DISABLED) {
 			printk(XENLOG_DEBUG PREFIX "state %s is disabled",
 			       cpuidle_state_table[cstate].name);
 			continue;
@@ -1221,7 +1242,12 @@ 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;
+
+		if (flags & CPUIDLE_FLAG_USE_HALT)
+			cx->entry_method = ACPI_CSTATE_EM_HALT;
+		 else
+			cx->entry_method = ACPI_CSTATE_EM_FFH;
+
 		cx->latency = cpuidle_state_table[cstate].exit_latency;
 		cx->target_residency =
 			cpuidle_state_table[cstate].target_residency;
-- 
2.11.0


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

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

[Woods, Brian]

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

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

diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index a063e39d60..1036c8b101 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -979,6 +979,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)
  *
@@ -1115,6 +1122,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_AMD)
+		return;
+
 	switch (boot_cpu_data.x86_model) {
 	case 0x3e: /* IVT */
 		ivt_idle_state_table_update();
@@ -1126,13 +1136,24 @@ static void __init mwait_idle_state_table_update(void)
 	case 0x5e: /* SKL-H */
 		sklh_idle_state_table_update();
 		break;
- 	}
+	}
 }
 
 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;
+
+	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;
+	default:
+		id = NULL;
+	}
 
 	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 3/3] mwait-idle: add enablement for AMD Naples and Rome

[Woods, Brian]

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 1036c8b101..d63ec650e0 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();
@@ -979,10 +995,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 0/3] mwait support for AMD processors

[Jan Beulich]

>>> On 25.02.19 at 21:23, <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.

I recall you saying so elsewhere, but I continue to be confused. Afaik
HLT is specified to mean C1. Without having looked at the patches,
I'm also not happy to see you say you make the driver capable of using
HLT. That's not its purpose, and I think the ACPI driver should instead
be used for that.

It is my understanding that the driver is there solely to overcome
recurring issues with BIOSes not providing optimal (or even correct)
ACPI tables. Since for C1 we don't even need any ACPI tables (we
enter C1 [through HLT] whenever no other C states are defined),
I'm having trouble seeing what problem would be addressed here.
Are there really no deeper C states than C2 supported by your CPUs?

Jan



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

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

[Woods, Brian]

On 2/26/19 4:49 AM, Jan Beulich wrote:
>>>> On 25.02.19 at 21:23, <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.
> 
> I recall you saying so elsewhere, but I continue to be confused. Afaik
> HLT is specified to mean C1. Without having looked at the patches,
> I'm also not happy to see you say you make the driver capable of using
> HLT. That's not its purpose, and I think the ACPI driver should instead
> be used for that.
> 
> It is my understanding that the driver is there solely to overcome
> recurring issues with BIOSes not providing optimal (or even correct)
> ACPI tables. Since for C1 we don't even need any ACPI tables (we
> enter C1 [through HLT] whenever no other C states are defined),
> I'm having trouble seeing what problem would be addressed here.
> Are there really no deeper C states than C2 supported by your CPUs?
> 
> Jan
> 

On Naples/Rome HLT can mean different things depending on how the 
HW/BIOS is set up.  If C2/CC6 is enabled, HLT (after going through some 
checks etc), will put the system in a C2/CC6 state.  If C2/CC6 is 
disabled in BIOS, then HLT will act as C1/CC1. It may not be completely 
ideal but the system will function just fine.  It's the best that can be 
done without reading the tables (that I can think of).

Correct me if I'm wrong, but the Xen's acpi-idle implementation is 
dependent on dom0 using a AML interpreter and then giving that data back 
to Xen.  I've heard that this doesn't always work correctly on PV dom0s 
and doesn't work at all on PVH dom0s.

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

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

[Jan Beulich]

>>> On 26.02.19 at 17:25, <Brian.Woods@amd.com> wrote:
> Correct me if I'm wrong, but the Xen's acpi-idle implementation is 
> dependent on dom0 using a AML interpreter and then giving that data back 
> to Xen.  I've heard that this doesn't always work correctly on PV dom0s 
> and doesn't work at all on PVH dom0s.

For C2 and deeper (using entering methods other than HLT) - yes.
The use of HLT is the default with the assumption that this will put
the system in C1 (i.e. with a pretty low wakeup latency); see
default_idle(), cpuidle_init_cpu(), and acpi_idle_do_entry().

Jan



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

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

[Woods, Brian]

On 2/26/19 10:37 AM, Jan Beulich wrote:
>>>> On 26.02.19 at 17:25, <Brian.Woods@amd.com> wrote:
>> Correct me if I'm wrong, but the Xen's acpi-idle implementation is
>> dependent on dom0 using a AML interpreter and then giving that data back
>> to Xen.  I've heard that this doesn't always work correctly on PV dom0s
>> and doesn't work at all on PVH dom0s.
> 
> For C2 and deeper (using entering methods other than HLT) - yes.
> The use of HLT is the default with the assumption that this will put
> the system in C1 (i.e. with a pretty low wakeup latency); see
> default_idle(), cpuidle_init_cpu(), and acpi_idle_do_entry().
> 
> Jan
> 

Well, assuming C2 is enabled (which I was assume is the default case), 
HLT roughly puts the processor in C2 rather than C1.  On my test system, 
the debug console output for the cx tables only output HLT for C1 (which 
is wrong).

Rather than depending on dom0, which is shaky, and not having an AML 
interpreter, it seems the best solution is to hardcode the values in 
like Intel does.  If Xen had an AML interpreter, I'd agree doing things 
differently (reading in the ACPI tables) would be best.  But given the 
resources Xen has at the moment, this seems like the safest solution and 
is better than using HLT (which is C2 assuming it's enabled) as the 
default idle method like Xen is using now.

It comes down to sometimes (when C2 is diabled in BIOS) using C1 
thinking it's C2, or without the patches in the common case using C2 
thinking it's C1.

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

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

[Jan Beulich]

>>> On 26.02.19 at 17:54, <Brian.Woods@amd.com> wrote:
> On 2/26/19 10:37 AM, Jan Beulich wrote:
>>>>> On 26.02.19 at 17:25, <Brian.Woods@amd.com> wrote:
>>> Correct me if I'm wrong, but the Xen's acpi-idle implementation is
>>> dependent on dom0 using a AML interpreter and then giving that data back
>>> to Xen.  I've heard that this doesn't always work correctly on PV dom0s
>>> and doesn't work at all on PVH dom0s.
>> 
>> For C2 and deeper (using entering methods other than HLT) - yes.
>> The use of HLT is the default with the assumption that this will put
>> the system in C1 (i.e. with a pretty low wakeup latency); see
>> default_idle(), cpuidle_init_cpu(), and acpi_idle_do_entry().
> 
> Well, assuming C2 is enabled (which I was assume is the default case), 
> HLT roughly puts the processor in C2 rather than C1.  On my test system, 
> the debug console output for the cx tables only output HLT for C1 (which 
> is wrong).
> 
> Rather than depending on dom0, which is shaky, and not having an AML 
> interpreter, it seems the best solution is to hardcode the values in 
> like Intel does.  If Xen had an AML interpreter, I'd agree doing things 
> differently (reading in the ACPI tables) would be best.  But given the 
> resources Xen has at the moment, this seems like the safest solution and 
> is better than using HLT (which is C2 assuming it's enabled) as the 
> default idle method like Xen is using now.
> 
> It comes down to sometimes (when C2 is diabled in BIOS) using C1 
> thinking it's C2, or without the patches in the common case using C2 
> thinking it's C1.

So in one of our idle routines, how would one go about entering
C1 or C2 depending on wakeup latency requirements? I'm having a
hard time seeing how HLT can be used for both (without a reboot
cycle and a BIOS option change in between). Yet if there's only
one state that can be entered, then it's merely cosmetic whether
it gets called C1 or C2 in the debug output.

Anyway - I guess we need to continue this discussion (if necessary)
once I got around to actually look at the patches.

Jan



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

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

[Wei Liu]

On Mon, Feb 25, 2019 at 08:23:58PM +0000, Woods, Brian wrote:
> 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/cpu/mwait-idle.c | 40 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
> index f89c52f256..a063e39d60 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
> @@ -783,8 +788,23 @@ 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);
> +	if (cpu_is_haltable(cpu)) {
> +		struct cpu_info *info;
> +		switch (cx->entry_method) {
> +		case ACPI_CSTATE_EM_FFH:
> +			mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
> +			break;
> +		case ACPI_CSTATE_EM_HALT:

> +			info = get_cpu_info();
> +			spec_ctrl_enter_idle(info);
> +			safe_halt();
> +			spec_ctrl_exit_idle(info);

May I suggest you make this snippet a function? The same code snippet
appears a few lines above.

Wei.

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

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

[Woods, Brian]

On 2/27/19 2:51 AM, Jan Beulich wrote:
>>>> On 26.02.19 at 17:54, <Brian.Woods@amd.com> wrote:
>> On 2/26/19 10:37 AM, Jan Beulich wrote:
>>>>>> On 26.02.19 at 17:25, <Brian.Woods@amd.com> wrote:
>>>> Correct me if I'm wrong, but the Xen's acpi-idle implementation is
>>>> dependent on dom0 using a AML interpreter and then giving that data back
>>>> to Xen.  I've heard that this doesn't always work correctly on PV dom0s
>>>> and doesn't work at all on PVH dom0s.
>>>
>>> For C2 and deeper (using entering methods other than HLT) - yes.
>>> The use of HLT is the default with the assumption that this will put
>>> the system in C1 (i.e. with a pretty low wakeup latency); see
>>> default_idle(), cpuidle_init_cpu(), and acpi_idle_do_entry().
>>
>> Well, assuming C2 is enabled (which I was assume is the default case),
>> HLT roughly puts the processor in C2 rather than C1.  On my test system,
>> the debug console output for the cx tables only output HLT for C1 (which
>> is wrong).
>>
>> Rather than depending on dom0, which is shaky, and not having an AML
>> interpreter, it seems the best solution is to hardcode the values in
>> like Intel does.  If Xen had an AML interpreter, I'd agree doing things
>> differently (reading in the ACPI tables) would be best.  But given the
>> resources Xen has at the moment, this seems like the safest solution and
>> is better than using HLT (which is C2 assuming it's enabled) as the
>> default idle method like Xen is using now.
>>
>> It comes down to sometimes (when C2 is diabled in BIOS) using C1
>> thinking it's C2, or without the patches in the common case using C2
>> thinking it's C1.
> 
> So in one of our idle routines, how would one go about entering
> C1 or C2 depending on wakeup latency requirements? I'm having a
> hard time seeing how HLT can be used for both (without a reboot
> cycle and a BIOS option change in between). Yet if there's only
> one state that can be entered, then it's merely cosmetic whether
> it gets called C1 or C2 in the debug output.
> 
> Anyway - I guess we need to continue this discussion (if necessary)
> once I got around to actually look at the patches.
> 
> Jan
> 
> Does this answer the questions?

C2/CC6 enabled (default our internal MBs [and in general I'd assume])
HLT -> C2/CC6*
mwait -> C1/CC1

C2/CC6 disabled in BIOS
HLT -> C1/CC1
mwait -> C1/CC1

* HLT doesn't directly call C2/CC6 but it has a small timer then flushes 
the caches and puts it in C2/CC6.  Effectively it's the same but not 
exactly.

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

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

[Woods, Brian]

On 2/27/19 7:47 AM, Wei Liu wrote:
> On Mon, Feb 25, 2019 at 08:23:58PM +0000, Woods, Brian wrote:
>> 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/cpu/mwait-idle.c | 40 +++++++++++++++++++++++++++++++++-------
>>   1 file changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
>> index f89c52f256..a063e39d60 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
>> @@ -783,8 +788,23 @@ 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);
>> +	if (cpu_is_haltable(cpu)) {
>> +		struct cpu_info *info;
>> +		switch (cx->entry_method) {
>> +		case ACPI_CSTATE_EM_FFH:
>> +			mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
>> +			break;
>> +		case ACPI_CSTATE_EM_HALT:
> 
>> +			info = get_cpu_info();
>> +			spec_ctrl_enter_idle(info);
>> +			safe_halt();
>> +			spec_ctrl_exit_idle(info);
> 
> May I suggest you make this snippet a function? The same code snippet
> appears a few lines above.
> 
> Wei.
> 
It's used in various other places as well (cpu_idle.c, x86/domain.c), 
would a function like:

void safe_halt_with_spec(cpu_info *info)
{
     if (!info)
         info = get_cpu_info();

     spec_ctrl_enter_idle(info);
     safe_halt();
     spec_ctrl_exit_idle(info);
}

work since that way it could be used in other places where info is 
already defined?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Wei Liu]

On Wed, Feb 27, 2019 at 06:23:35PM +0000, Woods, Brian wrote:
> On 2/27/19 7:47 AM, Wei Liu wrote:
> > On Mon, Feb 25, 2019 at 08:23:58PM +0000, Woods, Brian wrote:
> >> 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/cpu/mwait-idle.c | 40 +++++++++++++++++++++++++++++++++-------
> >>   1 file changed, 33 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
> >> index f89c52f256..a063e39d60 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
> >> @@ -783,8 +788,23 @@ 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);
> >> +	if (cpu_is_haltable(cpu)) {
> >> +		struct cpu_info *info;
> >> +		switch (cx->entry_method) {
> >> +		case ACPI_CSTATE_EM_FFH:
> >> +			mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
> >> +			break;
> >> +		case ACPI_CSTATE_EM_HALT:
> > 
> >> +			info = get_cpu_info();
> >> +			spec_ctrl_enter_idle(info);
> >> +			safe_halt();
> >> +			spec_ctrl_exit_idle(info);
> > 
> > May I suggest you make this snippet a function? The same code snippet
> > appears a few lines above.
> > 
> > Wei.
> > 
> It's used in various other places as well (cpu_idle.c, x86/domain.c), 
> would a function like:
> 
> void safe_halt_with_spec(cpu_info *info)
> {
>      if (!info)
>          info = get_cpu_info();
> 
>      spec_ctrl_enter_idle(info);
>      safe_halt();
>      spec_ctrl_exit_idle(info);
> }
> 
> work since that way it could be used in other places where info is 
> already defined?

Looks reasonable. But I will leave that to Andrew and Jan to decide what
suits them best.

Wei.

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

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

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

[Woods, Brian]

On 3/5/19 11:12 AM, Wei Liu wrote:
> On Wed, Feb 27, 2019 at 06:23:35PM +0000, Woods, Brian wrote:
>> On 2/27/19 7:47 AM, Wei Liu wrote:
>>> On Mon, Feb 25, 2019 at 08:23:58PM +0000, Woods, Brian wrote:
>>>> 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/cpu/mwait-idle.c | 40 +++++++++++++++++++++++++++++++++-------
>>>>    1 file changed, 33 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
>>>> index f89c52f256..a063e39d60 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
>>>> @@ -783,8 +788,23 @@ 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);
>>>> +	if (cpu_is_haltable(cpu)) {
>>>> +		struct cpu_info *info;
>>>> +		switch (cx->entry_method) {
>>>> +		case ACPI_CSTATE_EM_FFH:
>>>> +			mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
>>>> +			break;
>>>> +		case ACPI_CSTATE_EM_HALT:
>>>
>>>> +			info = get_cpu_info();
>>>> +			spec_ctrl_enter_idle(info);
>>>> +			safe_halt();
>>>> +			spec_ctrl_exit_idle(info);
>>>
>>> May I suggest you make this snippet a function? The same code snippet
>>> appears a few lines above.
>>>
>>> Wei.
>>>
>> It's used in various other places as well (cpu_idle.c, x86/domain.c),
>> would a function like:
>>
>> void safe_halt_with_spec(cpu_info *info)
>> {
>>       if (!info)
>>           info = get_cpu_info();
>>
>>       spec_ctrl_enter_idle(info);
>>       safe_halt();
>>       spec_ctrl_exit_idle(info);
>> }
>>
>> work since that way it could be used in other places where info is
>> already defined?
> 
> Looks reasonable. But I will leave that to Andrew and Jan to decide what
> suits them best.
> 
> Wei.
> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xenproject.org
>> https://lists.xenproject.org/mailman/listinfo/xen-devel

Ping for Andy and Jan for this and the patches in general?

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

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

[Jan Beulich]

>>> On 25.02.19 at 21:23, <Brian.Woods@amd.com> wrote:
> --- 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

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.

> @@ -783,8 +788,23 @@ 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);
> +	if (cpu_is_haltable(cpu)) {
> +		struct cpu_info *info;
> +		switch (cx->entry_method) {

Blank line between declaration(s) and statement(s) please. And
it would seem better to move the declaration right here (inside
the switch()) anyway. Then again ...

> +		case ACPI_CSTATE_EM_FFH:
> +			mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
> +			break;
> +		case ACPI_CSTATE_EM_HALT:
> +			info = get_cpu_info();
> +			spec_ctrl_enter_idle(info);
> +			safe_halt();
> +			spec_ctrl_exit_idle(info);

... wouldn't it be better to avoid the redundancy with default_idle(),
by introducing a new helper function, e.g. spec_ctrl_safe_halt()?

> +			local_irq_disable();
> +			break;
> +		default:
> +			printk(XENLOG_ERR PREFIX "unknown entry method %d\n", cx->entry_method);
> +		}

Overly long line and missing break statement.

> @@ -1184,8 +1204,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 flags = cpuidle_state_table[cstate].flags;

May I suggest to name the variable slightly differently, e.g. cflags,
to avoid any risk of it being mistaken for what we commonly use
with e.g. spin_lock_irqsave()?

> @@ -1221,7 +1242,12 @@ 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;
> +
> +		if (flags & CPUIDLE_FLAG_USE_HALT)
> +			cx->entry_method = ACPI_CSTATE_EM_HALT;
> +		 else
> +			cx->entry_method = ACPI_CSTATE_EM_FFH;

I'd prefer if you used a conditional expression here. One of the goals for
any changes to this file should be to limit the delta to its Linux original, in
order to increase the chances of patches coming from there to apply
reasonably cleanly here.

Doing so would also save me from complaining about the stray blank
ahead of "else".

Jan



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

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

[Jan Beulich]

>>> On 25.02.19 at 21:23, <Brian.Woods@amd.com> wrote:
> Newer AMD processors (F17h) have mwait support.  Add some checks to make
> sure vendor specific code is run correctly and some infrastructure to
> facilitate adding AMD processors.

Both my Fam15 and my Fam10 system have CPUID[1].ECX[3] set - why
the reference to Fam17 here?

> @@ -1115,6 +1122,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_AMD)
> +		return;

Please use != INTEL here.

> @@ -1126,13 +1136,24 @@ static void __init mwait_idle_state_table_update(void)
>  	case 0x5e: /* SKL-H */
>  		sklh_idle_state_table_update();
>  		break;
> - 	}
> +	}
>  }
>  
>  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;
> +
> +	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;
> +	default:
> +		id = NULL;
> +	}

Missing break statement again, but perhaps even better here to drop
the default: and make NULL the variable's initializer.

Jan



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

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

[Jan Beulich]

>>> On 25.02.19 at 21:24, <Brian.Woods@amd.com> wrote:
> 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).

But your code does not detect this situation, and does hence not update
the table used accordingly. Why is this? Is entering C1 cheaper one way
or the other in this situation (in which case the cheaper approach should
always be used)?

Jan



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

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

[Woods, Brian]

On 3/13/19 4:35 AM, Jan Beulich wrote:
>>>> On 25.02.19 at 21:23, <Brian.Woods@amd.com> wrote:
>> --- 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
> 
> 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.  If you'd like, I can 
look further into it.

>> @@ -783,8 +788,23 @@ 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);
>> +	if (cpu_is_haltable(cpu)) {
>> +		struct cpu_info *info;
>> +		switch (cx->entry_method) {
> 
> Blank line between declaration(s) and statement(s) please. And
> it would seem better to move the declaration right here (inside
> the switch()) anyway. Then again ...
> 
>> +		case ACPI_CSTATE_EM_FFH:
>> +			mwait_idle_with_hints(eax, MWAIT_ECX_INTERRUPT_BREAK);
>> +			break;
>> +		case ACPI_CSTATE_EM_HALT:
>> +			info = get_cpu_info();
>> +			spec_ctrl_enter_idle(info);
>> +			safe_halt();
>> +			spec_ctrl_exit_idle(info);
> 
> ... wouldn't it be better to avoid the redundancy with default_idle(),
> by introducing a new helper function, e.g. spec_ctrl_safe_halt()?
> 
See my email with Wei about this.


>> +			local_irq_disable();
>> +			break;
>> +		default:
>> +			printk(XENLOG_ERR PREFIX "unknown entry method %d\n", cx->entry_method);
>> +		}
> 
> Overly long line and missing break statement.
> 
>> @@ -1184,8 +1204,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 flags = cpuidle_state_table[cstate].flags;
> 
> May I suggest to name the variable slightly differently, e.g. cflags,
> to avoid any risk of it being mistaken for what we commonly use
> with e.g. spin_lock_irqsave()?
> 
>> @@ -1221,7 +1242,12 @@ 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;
>> +
>> +		if (flags & CPUIDLE_FLAG_USE_HALT)
>> +			cx->entry_method = ACPI_CSTATE_EM_HALT;
>> +		 else
>> +			cx->entry_method = ACPI_CSTATE_EM_FFH;
> 
> I'd prefer if you used a conditional expression here. One of the goals for
> any changes to this file should be to limit the delta to its Linux original, in
> order to increase the chances of patches coming from there to apply
> reasonably cleanly here.
> 
> Doing so would also save me from complaining about the stray blank
> ahead of "else".
> 
> Jan
> 

By conditional statement you mean ternary?  If so, that'll be easy enough.

Also, noted for things I didn't directly to.

Brian

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

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

[Woods, Brian]

On 3/13/19 4:42 AM, Jan Beulich wrote:
>>>> On 25.02.19 at 21:23, <Brian.Woods@amd.com> wrote:
>> Newer AMD processors (F17h) have mwait support.  Add some checks to make
>> sure vendor specific code is run correctly and some infrastructure to
>> facilitate adding AMD processors.
> 
> Both my Fam15 and my Fam10 system have CPUID[1].ECX[3] set - why
> the reference to Fam17 here?

We added CPUID_Fn00000005_EDX to match Intel starting with F17h M01h 
(Naples).  Therefore going forward, we're just enabling with Naples and 
further processors.

Noted about other comments.

Brian

>> @@ -1115,6 +1122,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_AMD)
>> +		return;
> 
> Please use != INTEL here.
> 
>> @@ -1126,13 +1136,24 @@ static void __init mwait_idle_state_table_update(void)
>>   	case 0x5e: /* SKL-H */
>>   		sklh_idle_state_table_update();
>>   		break;
>> - 	}
>> +	}
>>   }
>>   
>>   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;
>> +
>> +	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;
>> +	default:
>> +		id = NULL;
>> +	}
> 
> Missing break statement again, but perhaps even better here to drop
> the default: and make NULL the variable's initializer.
> 
> Jan
> 
> 

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

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

[Woods, Brian]

On 3/13/19 4:51 AM, Jan Beulich wrote:
>>>> On 25.02.19 at 21:24, <Brian.Woods@amd.com> wrote:
>> 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).
> 
> But your code does not detect this situation, and does hence not update
> the table used accordingly. Why is this? Is entering C1 cheaper one way
> or the other in this situation (in which case the cheaper approach should
> always be used)?
> 
> Jan
> 
> 

Well, if Xen had an AML interrupter, we could use the ACPI tables like 
we do in Linux, but Xen doesn't (which is why we're hard coding it). 
mwait has the CPUID_Fn00000005_EDX MSR but since we don't have a mwait 
support for CC6, we can't use that.  There's another register we _might_ 
be able to use, but support for CC6 is AND'd with that and another 
another register (we don't have access to). The register we'd read is 
also RW.  So I'm not sure I trust it.

The worst case (no CC6 when we think we have it), is that halt uses cc1 
rather than cc6.  I don't see a negative downside to this other than 
delay.  Although, if the idle scheduler is expecting a longer delay that 
what it is, I don't see this as a huge issue.  It still can use mwait 
for smaller idle periods.  Although, if all c states are turned off, 
mwait should even be available and this code is never enabled (and the 
default halt is used).

I agree it isn't optimal, but it's the best solution I can think of.

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

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

[Jan Beulich]

>>> On 14.03.19 at 20:00, <Brian.Woods@amd.com> wrote:
> On 3/13/19 4:35 AM, Jan Beulich wrote:
>>>>> On 25.02.19 at 21:23, <Brian.Woods@amd.com> wrote:
>>> --- 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
>> 
>> 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.  If you'd like, I can 
> look further into it.

Ah yes, I found it. But the text suggests to use SystemIO, not
HLT for entering C2 (CC6). An important difference looks to be
the state of EFLAGS.IF as to whether the core wakes up again.
The SystemIO approach would better match the FFixedHW one,
as we require and use MWAIT_ECX_INTERRUPT_BREAK.

Furthermore I'm then once again wondering what the gain is
over using the ACPI driver: The suggested _CST looks to exactly
match the data you enter into the table in the later patch. IOW
my fundamental concern didn't go away yet: As per the name
of the driver, it shouldn't really need to support HLT (or anything
other than MWAIT) as an entry method. Hence I think that at
the very least you need to extend the description of the change
quite a bit to explain why the ACPI driver is not suitable.

Depending on how this comes out, it may then still be a matter
of discussing whether, rather than fiddling with mwait-idle, it
wouldn't be better to have an AMD-specific driver instead. Are
there any thoughts in similar directions for Linux?

>>> +		case ACPI_CSTATE_EM_HALT:
>>> +			info = get_cpu_info();
>>> +			spec_ctrl_enter_idle(info);
>>> +			safe_halt();
>>> +			spec_ctrl_exit_idle(info);
>> 
>> ... wouldn't it be better to avoid the redundancy with default_idle(),
>> by introducing a new helper function, e.g. spec_ctrl_safe_halt()?
>> 
> See my email with Wei about this.

There you've basically settled on making a helper function, to
be used in pre-existing places as well as here.

I've also just noticed that there's another safe_halt() invocation
a few lines up from here, as a fallback. It doesn't come with any
of the statistics though, so would probably be unsuitable to
funnel into.

>>> @@ -1221,7 +1242,12 @@ 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;
>>> +
>>> +		if (flags & CPUIDLE_FLAG_USE_HALT)
>>> +			cx->entry_method = ACPI_CSTATE_EM_HALT;
>>> +		 else
>>> +			cx->entry_method = ACPI_CSTATE_EM_FFH;
>> 
>> I'd prefer if you used a conditional expression here. One of the goals for
>> any changes to this file should be to limit the delta to its Linux original, in
>> order to increase the chances of patches coming from there to apply
>> reasonably cleanly here.
>> 
>> Doing so would also save me from complaining about the stray blank
>> ahead of "else".
> 
> By conditional statement you mean ternary?  If so, that'll be easy enough.

Yes.

Jan



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

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

[Jan Beulich]

>>> On 14.03.19 at 20:29, <Brian.Woods@amd.com> wrote:
> On 3/13/19 4:51 AM, Jan Beulich wrote:
>>>>> On 25.02.19 at 21:24, <Brian.Woods@amd.com> wrote:
>>> 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).
>> 
>> But your code does not detect this situation, and does hence not update
>> the table used accordingly. Why is this? Is entering C1 cheaper one way
>> or the other in this situation (in which case the cheaper approach should
>> always be used)?
> 
> Well, if Xen had an AML interrupter, we could use the ACPI tables like 
> we do in Linux, but Xen doesn't (which is why we're hard coding it). 

But the necessary data gets uploaded by the ACPI code in Dom0. Or
else there wouldn't be a point to have an ACPI idle driver in Xen in the
first place.

We should add custom (vendor specific) code to Xen only if there
are clear advantages over the ACPI based approach, and so far
the patch descriptions don't make clear what advantages there
are (besides becoming independent of Dom0, which I'd consider
marginal).

> mwait has the CPUID_Fn00000005_EDX MSR but since we don't have a mwait 
> support for CC6, we can't use that.  There's another register we _might_ 
> be able to use, but support for CC6 is AND'd with that and another 
> another register (we don't have access to). The register we'd read is 
> also RW.  So I'm not sure I trust it.

It's hard to believe that one can't find out whether HLT would enter
only CC1 or eventually also CC6.

Jan



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

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

[Woods, Brian]

On 3/15/19 3:54 AM, Jan Beulich wrote:
>>>> On 14.03.19 at 20:29, <Brian.Woods@amd.com> wrote:
>> On 3/13/19 4:51 AM, Jan Beulich wrote:
>>>>>> On 25.02.19 at 21:24, <Brian.Woods@amd.com> wrote:
>>>> 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).
>>>
>>> But your code does not detect this situation, and does hence not update
>>> the table used accordingly. Why is this? Is entering C1 cheaper one way
>>> or the other in this situation (in which case the cheaper approach should
>>> always be used)?
>>
>> Well, if Xen had an AML interrupter, we could use the ACPI tables like
>> we do in Linux, but Xen doesn't (which is why we're hard coding it).
> 
> But the necessary data gets uploaded by the ACPI code in Dom0. Or
> else there wouldn't be a point to have an ACPI idle driver in Xen in the
> first place.
> 
> We should add custom (vendor specific) code to Xen only if there
> are clear advantages over the ACPI based approach, and so far
> the patch descriptions don't make clear what advantages there
> are (besides becoming independent of Dom0, which I'd consider
> marginal).
> 

I'll update the patches to explain why this is needed (aka, unreliable 
(PV dom0) or no passing (PVH dom0) of the ACPI tables back to Xen.

>> mwait has the CPUID_Fn00000005_EDX MSR but since we don't have a mwait
>> support for CC6, we can't use that.  There's another register we _might_
>> be able to use, but support for CC6 is AND'd with that and another
>> another register (we don't have access to). The register we'd read is
>> also RW.  So I'm not sure I trust it.
> 
> It's hard to believe that one can't find out whether HLT would enter
> only CC1 or eventually also CC6.
> 
> Jan
> 

There's a register, but it's AND'd with firmware for if C6 is enabled. 
Assuming it isn't touched, it should be able to determine if C6 is 
enabled or not by BIOS.  That leads to more code and the negative of not 
checking is system thinking it's using CC6 when it's really using CC1. 
It's also NDA'd so I'd have to get approval to use it (and then also put 
it in the public PPR).

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

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

[Woods, Brian]

On 3/15/19 3:37 AM, Jan Beulich wrote:
>>>> On 14.03.19 at 20:00, <Brian.Woods@amd.com> wrote:
>> On 3/13/19 4:35 AM, Jan Beulich wrote:
>>>>>> On 25.02.19 at 21:23, <Brian.Woods@amd.com> wrote:
>>>> --- 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
>>>
>>> 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.  If you'd like, I can
>> look further into it.
> 
> Ah yes, I found it. But the text suggests to use SystemIO, not
> HLT for entering C2 (CC6). An important difference looks to be
> the state of EFLAGS.IF as to whether the core wakes up again.
> The SystemIO approach would better match the FFixedHW one,
> as we require and use MWAIT_ECX_INTERRUPT_BREAK.
> 
> Furthermore I'm then once again wondering what the gain is
> over using the ACPI driver: The suggested _CST looks to exactly
> match the data you enter into the table in the later patch. IOW
> my fundamental concern didn't go away yet: As per the name
> of the driver, it shouldn't really need to support HLT (or anything
> other than MWAIT) as an entry method. Hence I think that at
> the very least you need to extend the description of the change
> quite a bit to explain why the ACPI driver is not suitable.
> 
> Depending on how this comes out, it may then still be a matter
> of discussing whether, rather than fiddling with mwait-idle, it
> wouldn't be better to have an AMD-specific driver instead. Are
> there any thoughts in similar directions for Linux?

I can make it use sysIO rather than HLT if there's a need or strong 
desire for it.  I used HLT mainly because I thought it would be more 
robust (like in the case of CC6 being disabled).

Because:
#1 getting the ACPI tables from dom0 is either unreliable (PV dom0) or 
not possible (PVH dom0).
#2 the changes to the Intel code are minimal.
#3 worse case, Xen thinks it's using CC6 when it's using CC1.  Not 
perfect but far from fatal or breaking.

In Linux, they have a working AML interrupter so they just read the ACPI 
tables.  If Xen had a working AML interrupter, I'd suggest just reading 
the ACPI tables as well.  As far as a completely different driver for 
AMD, it would mostly just be the Intel drive with the small changes and 
some code removed.  With the minimal changes needed, I don't see a 
reason, but that's just me.

>>>> +		case ACPI_CSTATE_EM_HALT:
>>>> +			info = get_cpu_info();
>>>> +			spec_ctrl_enter_idle(info);
>>>> +			safe_halt();
>>>> +			spec_ctrl_exit_idle(info);
>>>
>>> ... wouldn't it be better to avoid the redundancy with default_idle(),
>>> by introducing a new helper function, e.g. spec_ctrl_safe_halt()?
>>>
>> See my email with Wei about this.
> 
> There you've basically settled on making a helper function, to
> be used in pre-existing places as well as here.
> 
> I've also just noticed that there's another safe_halt() invocation
> a few lines up from here, as a fallback. It doesn't come with any
> of the statistics though, so would probably be unsuitable to
> funnel into.

It does use follow the pattern of:
	spec_ctrl_enter_idle(info);
	safe_halt();
	spec_ctrl_exit_idle(info);
though.  I'm pretty sure out would work with what I suggested or am I 
missing something?

>>>> @@ -1221,7 +1242,12 @@ 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;
>>>> +
>>>> +		if (flags & CPUIDLE_FLAG_USE_HALT)
>>>> +			cx->entry_method = ACPI_CSTATE_EM_HALT;
>>>> +		 else
>>>> +			cx->entry_method = ACPI_CSTATE_EM_FFH;
>>>
>>> I'd prefer if you used a conditional expression here. One of the goals for
>>> any changes to this file should be to limit the delta to its Linux original, in
>>> order to increase the chances of patches coming from there to apply
>>> reasonably cleanly here.
>>>
>>> Doing so would also save me from complaining about the stray blank
>>> ahead of "else".
>>
>> By conditional statement you mean ternary?  If so, that'll be easy enough.
> 
> Yes.
> 
> Jan
> 
> 
Noted.

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

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

[Jan Beulich]

>>> On 19.03.19 at 17:12, <Brian.Woods@amd.com> wrote:
> On 3/15/19 3:37 AM, Jan Beulich wrote:
>>>>> On 14.03.19 at 20:00, <Brian.Woods@amd.com> wrote:
>>> On 3/13/19 4:35 AM, Jan Beulich wrote:
>>>>>>> On 25.02.19 at 21:23, <Brian.Woods@amd.com> wrote:
>>>>> --- 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
>>>>
>>>> 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.  If you'd like, I can
>>> look further into it.
>> 
>> Ah yes, I found it. But the text suggests to use SystemIO, not
>> HLT for entering C2 (CC6). An important difference looks to be
>> the state of EFLAGS.IF as to whether the core wakes up again.
>> The SystemIO approach would better match the FFixedHW one,
>> as we require and use MWAIT_ECX_INTERRUPT_BREAK.
>> 
>> Furthermore I'm then once again wondering what the gain is
>> over using the ACPI driver: The suggested _CST looks to exactly
>> match the data you enter into the table in the later patch. IOW
>> my fundamental concern didn't go away yet: As per the name
>> of the driver, it shouldn't really need to support HLT (or anything
>> other than MWAIT) as an entry method. Hence I think that at
>> the very least you need to extend the description of the change
>> quite a bit to explain why the ACPI driver is not suitable.
>> 
>> Depending on how this comes out, it may then still be a matter
>> of discussing whether, rather than fiddling with mwait-idle, it
>> wouldn't be better to have an AMD-specific driver instead. Are
>> there any thoughts in similar directions for Linux?
> 
> I can make it use sysIO rather than HLT if there's a need or strong 
> desire for it.  I used HLT mainly because I thought it would be more 
> robust (like in the case of CC6 being disabled).

Well, you know whether to trust your documentation.

> Because:
> #1 getting the ACPI tables from dom0 is either unreliable (PV dom0) or 
> not possible (PVH dom0).

Why unreliable? And PVH Dom0 is still WIP.

> #2 the changes to the Intel code are minimal.

But they go against the purpose of the file, which is to make use of
MWAIT.

> #3 worse case, Xen thinks it's using CC6 when it's using CC1.  Not 
> perfect but far from fatal or breaking.

Yes, that's a minor aspect.

> In Linux, they have a working AML interrupter so they just read the ACPI 
> tables.  If Xen had a working AML interrupter, I'd suggest just reading 
> the ACPI tables as well.  As far as a completely different driver for 
> AMD, it would mostly just be the Intel drive with the small changes and 
> some code removed.  With the minimal changes needed, I don't see a 
> reason, but that's just me.

Well, I've put this up for discussion, and specifically raised the
question of what Linux is doing here.

>>>>> +		case ACPI_CSTATE_EM_HALT:
>>>>> +			info = get_cpu_info();
>>>>> +			spec_ctrl_enter_idle(info);
>>>>> +			safe_halt();
>>>>> +			spec_ctrl_exit_idle(info);
>>>>
>>>> ... wouldn't it be better to avoid the redundancy with default_idle(),
>>>> by introducing a new helper function, e.g. spec_ctrl_safe_halt()?
>>>>
>>> See my email with Wei about this.
>> 
>> There you've basically settled on making a helper function, to
>> be used in pre-existing places as well as here.
>> 
>> I've also just noticed that there's another safe_halt() invocation
>> a few lines up from here, as a fallback. It doesn't come with any
>> of the statistics though, so would probably be unsuitable to
>> funnel into.
> 
> It does use follow the pattern of:
> 	spec_ctrl_enter_idle(info);
> 	safe_halt();
> 	spec_ctrl_exit_idle(info);
> though.  I'm pretty sure out would work with what I suggested or am I 
> missing something?

I'm afraid I'm not understanding the question in this context. Perhaps
I'm largely confused by the use of "out" in the middle of the sentence.
Is this referring to the OUT instruction in context of the SysIO aspect
discussed further up? If so - I don't understand what you're trying to
get at in the context here. If not, I'm lost altogether.

Jan


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

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

[Jan Beulich]

>>> On 19.03.19 at 16:59, <Brian.Woods@amd.com> wrote:
> On 3/15/19 3:54 AM, Jan Beulich wrote:
>>>>> On 14.03.19 at 20:29, <Brian.Woods@amd.com> wrote:
>>> There's another register we _might_
>>> be able to use, but support for CC6 is AND'd with that and another
>>> another register (we don't have access to). The register we'd read is
>>> also RW.  So I'm not sure I trust it.
>> 
>> It's hard to believe that one can't find out whether HLT would enter
>> only CC1 or eventually also CC6.
> 
> There's a register, but it's AND'd with firmware for if C6 is enabled. 
> Assuming it isn't touched, it should be able to determine if C6 is 
> enabled or not by BIOS.  That leads to more code and the negative of not 
> checking is system thinking it's using CC6 when it's really using CC1. 
> It's also NDA'd so I'd have to get approval to use it (and then also put 
> it in the public PPR).

Well, no need to go through too many hoops for this. It would
just have been nice to tell the two apart, not the least because
of their quite different wakeup latencies.

Jan



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

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

[Jan Beulich]

>>> On 19.03.19 at 17:12, <Brian.Woods@amd.com> wrote:
> On 3/15/19 3:37 AM, Jan Beulich wrote:
>> Furthermore I'm then once again wondering what the gain is
>> over using the ACPI driver: The suggested _CST looks to exactly
>> match the data you enter into the table in the later patch. IOW
>> my fundamental concern didn't go away yet: As per the name
>> of the driver, it shouldn't really need to support HLT (or anything
>> other than MWAIT) as an entry method. Hence I think that at
>> the very least you need to extend the description of the change
>> quite a bit to explain why the ACPI driver is not suitable.
>> 
>> Depending on how this comes out, it may then still be a matter
>> of discussing whether, rather than fiddling with mwait-idle, it
>> wouldn't be better to have an AMD-specific driver instead. Are
>> there any thoughts in similar directions for Linux?
> 
> Because:
> #1 getting the ACPI tables from dom0 is either unreliable (PV dom0) or 
> not possible (PVH dom0).
> #2 the changes to the Intel code are minimal.
> #3 worse case, Xen thinks it's using CC6 when it's using CC1.  Not 
> perfect but far from fatal or breaking.

Having thought about this some more, I agree that an AMD-specific
driver would likely go too far. However, that's still no reason to fiddle
with the mwait-idle one - I think you could as well populate the data
as necessary for the ACPI driver to use, removing the dependency
on Dom0. After all that driver already knows of all the entry methods
you may want/need to use (see acpi_idle_do_entry()).

Jan



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

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

[Woods, Brian]

On 3/26/19 10:54 AM, Jan Beulich wrote:
>>>> On 19.03.19 at 17:12, <Brian.Woods@amd.com> wrote:
>> On 3/15/19 3:37 AM, Jan Beulich wrote:
>>> Furthermore I'm then once again wondering what the gain is
>>> over using the ACPI driver: The suggested _CST looks to exactly
>>> match the data you enter into the table in the later patch. IOW
>>> my fundamental concern didn't go away yet: As per the name
>>> of the driver, it shouldn't really need to support HLT (or anything
>>> other than MWAIT) as an entry method. Hence I think that at
>>> the very least you need to extend the description of the change
>>> quite a bit to explain why the ACPI driver is not suitable.
>>>
>>> Depending on how this comes out, it may then still be a matter
>>> of discussing whether, rather than fiddling with mwait-idle, it
>>> wouldn't be better to have an AMD-specific driver instead. Are
>>> there any thoughts in similar directions for Linux?
>>
>> Because:
>> #1 getting the ACPI tables from dom0 is either unreliable (PV dom0) or
>> not possible (PVH dom0).
>> #2 the changes to the Intel code are minimal.
>> #3 worse case, Xen thinks it's using CC6 when it's using CC1.  Not
>> perfect but far from fatal or breaking.
> 
> Having thought about this some more, I agree that an AMD-specific
> driver would likely go too far. However, that's still no reason to fiddle
> with the mwait-idle one - I think you could as well populate the data
> as necessary for the ACPI driver to use, removing the dependency
> on Dom0. After all that driver already knows of all the entry methods
> you may want/need to use (see acpi_idle_do_entry()).
> 
> Jan
> 
> 
I did a rough example of how that might work and lines of code changed 
for adding it to cpu_idle was roughly 125.  Seeing as this doesn't 
compile and doesn't even have comments, I'd say at least 140 lines of 
code/change (most of those are additive too), a lot of is functionally 
copied from mwait-idle and how it reads data out of the structures, 
checks, and populates the cx structures.  The first set of mwait patches 
is 87 lines changed total.

I _could_ try and refactor some of the code and get it down from 
125-140, but that would most likely make porting changes even harder for 
mwait-idle.

What are your thoughts?

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

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

[Jan Beulich]

>>> On 26.03.19 at 22:56, <Brian.Woods@amd.com> wrote:
> On 3/26/19 10:54 AM, Jan Beulich wrote:
>>>>> On 19.03.19 at 17:12, <Brian.Woods@amd.com> wrote:
>>> On 3/15/19 3:37 AM, Jan Beulich wrote:
>>>> Furthermore I'm then once again wondering what the gain is
>>>> over using the ACPI driver: The suggested _CST looks to exactly
>>>> match the data you enter into the table in the later patch. IOW
>>>> my fundamental concern didn't go away yet: As per the name
>>>> of the driver, it shouldn't really need to support HLT (or anything
>>>> other than MWAIT) as an entry method. Hence I think that at
>>>> the very least you need to extend the description of the change
>>>> quite a bit to explain why the ACPI driver is not suitable.
>>>>
>>>> Depending on how this comes out, it may then still be a matter
>>>> of discussing whether, rather than fiddling with mwait-idle, it
>>>> wouldn't be better to have an AMD-specific driver instead. Are
>>>> there any thoughts in similar directions for Linux?
>>>
>>> Because:
>>> #1 getting the ACPI tables from dom0 is either unreliable (PV dom0) or
>>> not possible (PVH dom0).
>>> #2 the changes to the Intel code are minimal.
>>> #3 worse case, Xen thinks it's using CC6 when it's using CC1.  Not
>>> perfect but far from fatal or breaking.
>> 
>> Having thought about this some more, I agree that an AMD-specific
>> driver would likely go too far. However, that's still no reason to fiddle
>> with the mwait-idle one - I think you could as well populate the data
>> as necessary for the ACPI driver to use, removing the dependency
>> on Dom0. After all that driver already knows of all the entry methods
>> you may want/need to use (see acpi_idle_do_entry()).
>> 
> I did a rough example of how that might work and lines of code changed 
> for adding it to cpu_idle was roughly 125.  Seeing as this doesn't 
> compile and doesn't even have comments, I'd say at least 140 lines of 
> code/change (most of those are additive too), a lot of is functionally 
> copied from mwait-idle and how it reads data out of the structures, 
> checks, and populates the cx structures.  The first set of mwait patches 
> is 87 lines changed total.
> 
> I _could_ try and refactor some of the code and get it down from 
> 125-140, but that would most likely make porting changes even harder for 
> mwait-idle.

Well, I was rather thinking about something like the change below,
taking slightly over 100 lines of new code, and not touching
mwait-idle.c at all. Otoh there are a couple of TBDs in there which
may cause the patch to further grow once addressed.

Note that this goes on top of
https://lists.xenproject.org/archives/html/xen-devel/2018-08/msg00089.html
which sadly there still wasn't sufficient feedback on to decide where
to go with the series; all I know is that Andrew (understandably)
doesn't want to see the last patch go in without vendor confirmation
(and I'd be fine to drop that last patch if need be, but this shouldn't
block the earlier patches in the series).

Jan

x86/AMD: make C-state handling independent of Dom0

At least for more recent CPUs, following what BKDG / PPR suggest for the
BIOS to surface via ACPI we can make ourselves independent of Dom0
uploading respective data.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Can we set local_apic_timer_c2_ok to true? I can't seem to find any
     statement in the BKDG / PPR as to whether the LAPIC timer continues
     running in CC6.
TBD: We may want to verify that HLT indeed is configured to enter CC6.
TBD: I guess we could extend this to families older then Fam15 as well.

--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -120,6 +120,8 @@ boolean_param("lapic_timer_c2_ok", local
 
 struct acpi_processor_power *__read_mostly processor_powers[NR_CPUS];
 
+static int8_t __read_mostly vendor_override;
+
 struct hw_residencies
 {
     uint64_t mc0;
@@ -1220,6 +1222,9 @@ long set_cx_pminfo(uint32_t acpi_id, str
     if ( pm_idle_save && pm_idle != acpi_processor_idle )
         return 0;
 
+    if ( vendor_override > 0 )
+        return 0;
+
     print_cx_pminfo(acpi_id, power);
 
     cpu_id = get_cpu_id(acpi_id);
@@ -1292,6 +1297,98 @@ long set_cx_pminfo(uint32_t acpi_id, str
     return 0;
 }
 
+static void amd_cpuidle_init(struct acpi_processor_power *power)
+{
+    unsigned int i, nr = 0;
+    const struct cpuinfo_x86 *c = &current_cpu_data;
+    const unsigned int ecx_req = CPUID5_ECX_EXTENSIONS_SUPPORTED |
+                                 CPUID5_ECX_INTERRUPT_BREAK;
+    const struct acpi_processor_cx *cx = NULL;
+    static const struct acpi_processor_cx fam17[] = {
+        {
+            .type = ACPI_STATE_C1,
+            .entry_method = ACPI_CSTATE_EM_FFH,
+            .address = 0,
+            .latency = 1,
+        },
+        {
+            .type = ACPI_STATE_C2,
+            .entry_method = ACPI_CSTATE_EM_HALT,
+            .latency = 400,
+        },
+    };
+
+    if ( pm_idle_save && pm_idle != acpi_processor_idle )
+        return;
+
+    if ( vendor_override < 0 )
+        return;
+
+    switch ( c->x86 )
+    {
+    case 0x17:
+        if ( cpu_has_monitor && c->cpuid_level >= CPUID_MWAIT_LEAF &&
+             (cpuid_ecx(CPUID_MWAIT_LEAF) & ecx_req) == ecx_req )
+        {
+            cx = fam17;
+            nr = ARRAY_SIZE(fam17);
+            break;
+        }
+        /* fall through */
+    case 0x15:
+    case 0x16:
+        cx = &fam17[1];
+        nr = ARRAY_SIZE(fam17) - 1;
+        break;
+
+    default:
+        vendor_override = -1;
+        return;
+    }
+
+    power->flags.has_cst = true;
+
+    for ( i = 0; i < nr; ++i )
+    {
+        if ( cx[i].type > max_cstate )
+            break;
+        power->states[i + 1] = cx[i];
+        power->states[i + 1].idx = i + 1;
+        power->states[i + 1].target_residency = cx[i].latency * latency_factor;
+    }
+
+    if ( i )
+    {
+        power->count = i + 1;
+        power->safe_state = &power->states[i];
+
+        if ( !vendor_override )
+        {
+            if ( !boot_cpu_has(X86_FEATURE_ARAT) )
+                hpet_broadcast_init();
+
+            if ( !lapic_timer_init() )
+            {
+                vendor_override = -1;
+                cpuidle_init_cpu(power->cpu);
+                return;
+            }
+
+            if ( !pm_idle_save )
+            {
+                pm_idle_save = pm_idle;
+                pm_idle = acpi_processor_idle;
+            }
+
+            dead_idle = acpi_dead_idle;
+
+            vendor_override = 1;
+        }
+    }
+    else
+        vendor_override = -1;
+}
+
 uint32_t pmstat_get_cx_nr(uint32_t cpuid)
 {
     return processor_powers[cpuid] ? processor_powers[cpuid]->count : 0;
@@ -1437,8 +1534,8 @@ static int cpu_callback(
     int rc = 0;
 
     /*
-     * Only hook on CPU_UP_PREPARE because a dead cpu may utilize the info
-     * to enter deep C-state.
+     * Only hook on CPU_UP_PREPARE / CPU_ONLINE because a dead cpu may utilize
+     * the info to enter deep C-state.
      */
     switch ( action )
     {
@@ -1447,6 +1544,12 @@ static int cpu_callback(
         if ( !rc && cpuidle_current_governor->enable )
             rc = cpuidle_current_governor->enable(processor_powers[cpu]);
         break;
+
+    case CPU_ONLINE:
+        if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
+             processor_powers[cpu] )
+            amd_cpuidle_init(processor_powers[cpu]);
+        break;
     }
 
     return !rc ? NOTIFY_DONE : notifier_from_errno(rc);



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

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

[Woods, Brian]

On 3/27/19 9:48 AM, Jan Beulich wrote:
>>>> On 26.03.19 at 22:56, <Brian.Woods@amd.com> wrote:
>> On 3/26/19 10:54 AM, Jan Beulich wrote:
>>>>>> On 19.03.19 at 17:12, <Brian.Woods@amd.com> wrote:
>>>> On 3/15/19 3:37 AM, Jan Beulich wrote:
>>>>> Furthermore I'm then once again wondering what the gain is
>>>>> over using the ACPI driver: The suggested _CST looks to exactly
>>>>> match the data you enter into the table in the later patch. IOW
>>>>> my fundamental concern didn't go away yet: As per the name
>>>>> of the driver, it shouldn't really need to support HLT (or anything
>>>>> other than MWAIT) as an entry method. Hence I think that at
>>>>> the very least you need to extend the description of the change
>>>>> quite a bit to explain why the ACPI driver is not suitable.
>>>>>
>>>>> Depending on how this comes out, it may then still be a matter
>>>>> of discussing whether, rather than fiddling with mwait-idle, it
>>>>> wouldn't be better to have an AMD-specific driver instead. Are
>>>>> there any thoughts in similar directions for Linux?
>>>>
>>>> Because:
>>>> #1 getting the ACPI tables from dom0 is either unreliable (PV dom0) or
>>>> not possible (PVH dom0).
>>>> #2 the changes to the Intel code are minimal.
>>>> #3 worse case, Xen thinks it's using CC6 when it's using CC1.  Not
>>>> perfect but far from fatal or breaking.
>>>
>>> Having thought about this some more, I agree that an AMD-specific
>>> driver would likely go too far. However, that's still no reason to fiddle
>>> with the mwait-idle one - I think you could as well populate the data
>>> as necessary for the ACPI driver to use, removing the dependency
>>> on Dom0. After all that driver already knows of all the entry methods
>>> you may want/need to use (see acpi_idle_do_entry()).
>>>
>> I did a rough example of how that might work and lines of code changed
>> for adding it to cpu_idle was roughly 125.  Seeing as this doesn't
>> compile and doesn't even have comments, I'd say at least 140 lines of
>> code/change (most of those are additive too), a lot of is functionally
>> copied from mwait-idle and how it reads data out of the structures,
>> checks, and populates the cx structures.  The first set of mwait patches
>> is 87 lines changed total.
>>
>> I _could_ try and refactor some of the code and get it down from
>> 125-140, but that would most likely make porting changes even harder for
>> mwait-idle.
> 
> Well, I was rather thinking about something like the change below,
> taking slightly over 100 lines of new code, and not touching
> mwait-idle.c at all. Otoh there are a couple of TBDs in there which
> may cause the patch to further grow once addressed.
> 
> Note that this goes on top of
> https://lists.xenproject.org/archives/html/xen-devel/2018-08/msg00089.html
> which sadly there still wasn't sufficient feedback on to decide where
> to go with the series; all I know is that Andrew (understandably)
> doesn't want to see the last patch go in without vendor confirmation
> (and I'd be fine to drop that last patch if need be, but this shouldn't
> block the earlier patches in the series).
> 
> Jan
> 
> x86/AMD: make C-state handling independent of Dom0
> 
> At least for more recent CPUs, following what BKDG / PPR suggest for the
> BIOS to surface via ACPI we can make ourselves independent of Dom0
> uploading respective data.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: Can we set local_apic_timer_c2_ok to true? I can't seem to find any
>       statement in the BKDG / PPR as to whether the LAPIC timer continues
>       running in CC6.
> TBD: We may want to verify that HLT indeed is configured to enter CC6.
> TBD: I guess we could extend this to families older then Fam15 as well.
> 
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -120,6 +120,8 @@ boolean_param("lapic_timer_c2_ok", local
>   
>   struct acpi_processor_power *__read_mostly processor_powers[NR_CPUS];
>   
> +static int8_t __read_mostly vendor_override;
> +
>   struct hw_residencies
>   {
>       uint64_t mc0;
> @@ -1220,6 +1222,9 @@ long set_cx_pminfo(uint32_t acpi_id, str
>       if ( pm_idle_save && pm_idle != acpi_processor_idle )
>           return 0;
>   
> +    if ( vendor_override > 0 )
> +        return 0;
> +
>       print_cx_pminfo(acpi_id, power);
>   
>       cpu_id = get_cpu_id(acpi_id);
> @@ -1292,6 +1297,98 @@ long set_cx_pminfo(uint32_t acpi_id, str
>       return 0;
>   }
>   
> +static void amd_cpuidle_init(struct acpi_processor_power *power)
> +{
> +    unsigned int i, nr = 0;
> +    const struct cpuinfo_x86 *c = &current_cpu_data;
> +    const unsigned int ecx_req = CPUID5_ECX_EXTENSIONS_SUPPORTED |
> +                                 CPUID5_ECX_INTERRUPT_BREAK;
> +    const struct acpi_processor_cx *cx = NULL;
> +    static const struct acpi_processor_cx fam17[] = {
> +        {
> +            .type = ACPI_STATE_C1,
> +            .entry_method = ACPI_CSTATE_EM_FFH,
> +            .address = 0,
> +            .latency = 1,
> +        },
> +        {
> +            .type = ACPI_STATE_C2,
> +            .entry_method = ACPI_CSTATE_EM_HALT,
> +            .latency = 400,
> +        },
> +    };
> +
> +    if ( pm_idle_save && pm_idle != acpi_processor_idle )
> +        return;
> +
> +    if ( vendor_override < 0 )
> +        return;
> +
> +    switch ( c->x86 )
> +    {
> +    case 0x17:
> +        if ( cpu_has_monitor && c->cpuid_level >= CPUID_MWAIT_LEAF &&
> +             (cpuid_ecx(CPUID_MWAIT_LEAF) & ecx_req) == ecx_req )
> +        {
> +            cx = fam17;
> +            nr = ARRAY_SIZE(fam17);
> +            break;
> +        }
> +        /* fall through */
> +    case 0x15:
> +    case 0x16:
> +        cx = &fam17[1];
> +        nr = ARRAY_SIZE(fam17) - 1;
> +        break;
> +
> +    default:
> +        vendor_override = -1;
> +        return;
> +    }
> +
> +    power->flags.has_cst = true;
> +
> +    for ( i = 0; i < nr; ++i )
> +    {
> +        if ( cx[i].type > max_cstate )
> +            break;
> +        power->states[i + 1] = cx[i];
> +        power->states[i + 1].idx = i + 1;
> +        power->states[i + 1].target_residency = cx[i].latency * latency_factor;
> +    }
> +
> +    if ( i )
> +    {
> +        power->count = i + 1;
> +        power->safe_state = &power->states[i];
> +
> +        if ( !vendor_override )
> +        {
> +            if ( !boot_cpu_has(X86_FEATURE_ARAT) )
> +                hpet_broadcast_init();
> +
> +            if ( !lapic_timer_init() )
> +            {
> +                vendor_override = -1;
> +                cpuidle_init_cpu(power->cpu);
> +                return;
> +            }
> +
> +            if ( !pm_idle_save )
> +            {
> +                pm_idle_save = pm_idle;
> +                pm_idle = acpi_processor_idle;
> +            }
> +
> +            dead_idle = acpi_dead_idle;
> +
> +            vendor_override = 1;
> +        }
> +    }
> +    else
> +        vendor_override = -1;
> +}
> +
>   uint32_t pmstat_get_cx_nr(uint32_t cpuid)
>   {
>       return processor_powers[cpuid] ? processor_powers[cpuid]->count : 0;
> @@ -1437,8 +1534,8 @@ static int cpu_callback(
>       int rc = 0;
>   
>       /*
> -     * Only hook on CPU_UP_PREPARE because a dead cpu may utilize the info
> -     * to enter deep C-state.
> +     * Only hook on CPU_UP_PREPARE / CPU_ONLINE because a dead cpu may utilize
> +     * the info to enter deep C-state.
>        */
>       switch ( action )
>       {
> @@ -1447,6 +1544,12 @@ static int cpu_callback(
>           if ( !rc && cpuidle_current_governor->enable )
>               rc = cpuidle_current_governor->enable(processor_powers[cpu]);
>           break;
> +
> +    case CPU_ONLINE:
> +        if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> +             processor_powers[cpu] )
> +            amd_cpuidle_init(processor_powers[cpu]);
> +        break;
>       }
>   
>       return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
> 
> 

This also lacks some of the features of mwait-idle has and duplicates 
the limited functionality.  There's also a lack of comments which may or 
may not be needed.  So that would add to the line change count if you 
care about that.

I'm not sure why you're so adverse to the mwait-idle patches.  We're 
hard coding values in and using mwait (just like Intel is), but the only 
real change we need is using halt for one c-state.

Rather than having both drivers have the ability to read in data from 
structures and duplicate functionality and code, why not just have a 
single driver for hard coded values (mwait-idle) and one for getting the 
values in from dom0 (acpi-idle)?

I think you're getting hung up on the fact that we just don't use mwait 
rather than seeing the larger picture.  Which is, one driver uses hard 
coded values and the other is reading them from Dom0.  We still use 
mwait in the mwait driver, but we just happen to use halt for one 
c-state.  It doesn't require a huge rework of the driver or reducing the 
functionality of it for Intel.  Rather, we're limiting the functionality 
of using hard coded values to one driver that support both vendors with 
minimal changes.

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

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

[Jan Beulich]

>>> On 27.03.19 at 18:28, <Brian.Woods@amd.com> wrote:
> This also lacks some of the features of mwait-idle has and duplicates 
> the limited functionality.

Would you mind clarifying the lack-of-features aspect? The
only difference to your patches that I can spot is that you set
.target_residency in the static tables. If the value wanted
for CC6 is really 1000 instead of the 800 the default
calculation would produce, then this would be easy enough
to arrange for in my variant of the patch as well.

The mwait-idle driver would not have been needed at all if all
BIOSes surfaced complete data via ACPI. Therefore, by
suitably populating tables, it ought to be possible in theory to
use just that one driver. It's just that for Intel CPUs we've
decided to follow what Linux does, hence the separate
driver. There's no Linux precedent for AMD (afaict).

>  There's also a lack of comments which may or 
> may not be needed.  So that would add to the line change count if you 
> care about that.
> 
> I'm not sure why you're so adverse to the mwait-idle patches.  We're 
> hard coding values in and using mwait (just like Intel is), but the only 
> real change we need is using halt for one c-state.

But that's precisely what I dislike, as getting us further away
from the Linux driver. And btw, if we were to go that route,
then I think we'd better call acpi_idle_do_entry() than to
duplicate further parts of it. But that would also remove some
of that other part of the benefits of mwait_idle() over
acpi_processor_idle(): The former is much more streamlined,
due to not having to care about anything other than MWAIT.

As an aside, despite having followed the HLT approach in my
draft patch, I continue to be unconvinced that this is what we
actually want. There's a respective TBD remark there.

Jan



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

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

[Woods, Brian]

On 3/28/19 3:26 AM, Jan Beulich wrote:
>>>> On 27.03.19 at 18:28, <Brian.Woods@amd.com> wrote:
>> This also lacks some of the features of mwait-idle has and duplicates
>> the limited functionality.
> 
> Would you mind clarifying the lack-of-features aspect? The
> only difference to your patches that I can spot is that you set
> .target_residency in the static tables. If the value wanted
> for CC6 is really 1000 instead of the 800 the default
> calculation would produce, then this would be easy enough
> to arrange for in my variant of the patch as well.
> 
> The mwait-idle driver would not have been needed at all if all
> BIOSes surfaced complete data via ACPI. Therefore, by
> suitably populating tables, it ought to be possible in theory to
> use just that one driver. It's just that for Intel CPUs we've
> decided to follow what Linux does, hence the separate
> driver. There's no Linux precedent for AMD (afaict).

target_residency and some of the checks IIRC.

Yes, but that's Linux and this is Xen.  Linux has an AML interpreter and 
Xen does not.  That's an apple to oranges comparison.  You can't compare 
Xen to Linux for this because the features they have and how they work 
are different.

>>   There's also a lack of comments which may or
>> may not be needed.  So that would add to the line change count if you
>> care about that.
>>
>> I'm not sure why you're so adverse to the mwait-idle patches.  We're
>> hard coding values in and using mwait (just like Intel is), but the only
>> real change we need is using halt for one c-state.
> 
> But that's precisely what I dislike, as getting us further away
> from the Linux driver. And btw, if we were to go that route,
> then I think we'd better call acpi_idle_do_entry() than to
> duplicate further parts of it. But that would also remove some
> of that other part of the benefits of mwait_idle() over
> acpi_processor_idle(): The former is much more streamlined,
> due to not having to care about anything other than MWAIT.
> 
> As an aside, despite having followed the HLT approach in my
> draft patch, I continue to be unconvinced that this is what we
> actually want. There's a respective TBD remark there.
> 
> Jan
> 
> 

The changes needed are small though... most of the changes are 
non-intrusive.  It's just a couple of lines here and then and then 
something where it calls what entry_method.  Although, I think using 
acpi_idle_do_entry() is perfectly fine. With the acpi_idle_do_entry 
change, the line change count of the mwait-idle patches is 65 lines. A 
lot of that is the structures (28 lines), which isn't any _real_ code 
change.

One function call and one switch statement isn't going to change the 
performance or how streamlined it is.  The only other change is when 
it's initialized which is only at start up.

Functionally, it should go in mwait-idle.  The changes are small, the 
functionally the same, there's no duplication of functionality or code.

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

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

[Jan Beulich]

>>> On 28.03.19 at 16:02, <Brian.Woods@amd.com> wrote:
> On 3/28/19 3:26 AM, Jan Beulich wrote:
>>>>> On 27.03.19 at 18:28, <Brian.Woods@amd.com> wrote:
>>> This also lacks some of the features of mwait-idle has and duplicates
>>> the limited functionality.
>> 
>> Would you mind clarifying the lack-of-features aspect? The
>> only difference to your patches that I can spot is that you set
>> .target_residency in the static tables. If the value wanted
>> for CC6 is really 1000 instead of the 800 the default
>> calculation would produce, then this would be easy enough
>> to arrange for in my variant of the patch as well.
>> 
>> The mwait-idle driver would not have been needed at all if all
>> BIOSes surfaced complete data via ACPI. Therefore, by
>> suitably populating tables, it ought to be possible in theory to
>> use just that one driver. It's just that for Intel CPUs we've
>> decided to follow what Linux does, hence the separate
>> driver. There's no Linux precedent for AMD (afaict).
> 
> target_residency and some of the checks IIRC.

Could you be more specific what checks you mean?

> Yes, but that's Linux and this is Xen.  Linux has an AML interpreter and 
> Xen does not.  That's an apple to oranges comparison.  You can't compare 
> Xen to Linux for this because the features they have and how they work 
> are different.

It's not a direct comparison, sure. But lack of suitable ACPI data
(known to happen in practice) would put Linux into exactly the
same position. If Linux accepted changes to the driver to use
entry methods other than MWAIT, I'd not be as opposed (but I'd
still question their reasoning then).

> Functionally, it should go in mwait-idle.

That's what I continue to question, seeing the HLT additions you're
making. Plus older families (which you didn't cover at all so far)
apparently would want HLT alone spelled out, which is even less
suitable for a driver with this name.

Jan



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

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

[Woods, Brian]

On 3/28/19 10:50 AM, Jan Beulich wrote:
>>>> On 28.03.19 at 16:02, <Brian.Woods@amd.com> wrote:
>> On 3/28/19 3:26 AM, Jan Beulich wrote:
>>>>>> On 27.03.19 at 18:28, <Brian.Woods@amd.com> wrote:
>>>> This also lacks some of the features of mwait-idle has and duplicates
>>>> the limited functionality.
>>>
>>> Would you mind clarifying the lack-of-features aspect? The
>>> only difference to your patches that I can spot is that you set
>>> .target_residency in the static tables. If the value wanted
>>> for CC6 is really 1000 instead of the 800 the default
>>> calculation would produce, then this would be easy enough
>>> to arrange for in my variant of the patch as well.
>>>
>>> The mwait-idle driver would not have been needed at all if all
>>> BIOSes surfaced complete data via ACPI. Therefore, by
>>> suitably populating tables, it ought to be possible in theory to
>>> use just that one driver. It's just that for Intel CPUs we've
>>> decided to follow what Linux does, hence the separate
>>> driver. There's no Linux precedent for AMD (afaict).
>>
>> target_residency and some of the checks IIRC.
> 
> Could you be more specific what checks you mean?
> 
>> Yes, but that's Linux and this is Xen.  Linux has an AML interpreter and
>> Xen does not.  That's an apple to oranges comparison.  You can't compare
>> Xen to Linux for this because the features they have and how they work
>> are different.
> 
> It's not a direct comparison, sure. But lack of suitable ACPI data
> (known to happen in practice) would put Linux into exactly the
> same position. If Linux accepted changes to the driver to use
> entry methods other than MWAIT, I'd not be as opposed (but I'd
> still question their reasoning then).

Xen doesn't have an AML interpreter though and you can't reliably get 
the ACPI data from Dom0 in my experience.  You can't dictate what 
happens in Xen by what happens in Linux when the systems function 
completely different.  It's an apples and oranges situation.

>> Functionally, it should go in mwait-idle.
> 
> That's what I continue to question, seeing the HLT additions you're
> making. Plus older families (which you didn't cover at all so far)
> apparently would want HLT alone spelled out, which is even less
> suitable for a driver with this name.
> 
> Jan

Older families aren't compatible with Intel's mwait and we don't have 
any interest in enabling mwait on older families.  Older families won't 
be using this driver (mwait-idle) at all, but rather the acpi cpu_idle 
driver.  That's why there's only talk of F17h in the commits and code.

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