[PATCH]ACPI: re-enable mwait for xen cpuidle

[PATCH]ACPI: re-enable mwait for xen cpuidle

[Wei, Gang]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

I suggest including this patch in Xen 4.0 release. It is quite straightforward, and make sure the better and widely equipped cpu idle entry method MWAIT is used by xen if available.

BTW, can we consider re-open MWAIT freature for dom0 in xen hypervisor? We can discuss it post 4.0. 

Jimmy

ACPI: re-enable mwait for xen cpuidle

Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel start to check this feature while initializing _PDC object. Bypass such check in pv-ops case to re-enable mwait for xen cpuidle.

Signed-off-by: Wei Gang <gang.wei@intel.com>
Signed-off-by: Yu Ke <ke.yu@intel.com>

diff --git a/arch/x86/kernel/acpi/processor.c b/arch/x86/kernel/acpi/processor.c
index 8c9526d..d88866c 100644
--- a/arch/x86/kernel/acpi/processor.c
+++ b/arch/x86/kernel/acpi/processor.c
@@ -60,7 +60,7 @@ static void init_intel_pdc(struct acpi_processor *pr, struct cpuinfo_x86 *c)
 	/*
 	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
 	 */
-	if (!cpu_has(c, X86_FEATURE_MWAIT))
+	if (!cpu_has(c, X86_FEATURE_MWAIT) && !xen_initial_domain())
 		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
 
 	obj->type = ACPI_TYPE_BUFFER;
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 22baf4c..0a81637 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -419,7 +419,8 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 				cx.entry_method = ACPI_CSTATE_HALT;
 				snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI HLT");
 			} else {
-				continue;
+				if (!xen_initial_domain())
+					continue;
 			}
 			if (cx.type == ACPI_STATE_C1 &&
 					(idle_halt || idle_nomwait)) {

[-- Attachment #2: re-enable-mwait_pv-ops-dom0.patch --]
[-- Type: application/octet-stream, Size: 1382 bytes --]

ACPI: re-enable mwait for xen cpuidle

Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel start to check this feature while initializing _PDC object. Bypass such check in pv-ops case to re-enable mwait for xen cpuidle.

Signed-off-by: Wei Gang <gang.wei@intel.com>
Signed-off-by: Yu Ke <ke.yu@intel.com>

diff --git a/arch/x86/kernel/acpi/processor.c b/arch/x86/kernel/acpi/processor.c
index 8c9526d..d88866c 100644
--- a/arch/x86/kernel/acpi/processor.c
+++ b/arch/x86/kernel/acpi/processor.c
@@ -60,7 +60,7 @@ static void init_intel_pdc(struct acpi_processor *pr, struct cpuinfo_x86 *c)
 	/*
 	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
 	 */
-	if (!cpu_has(c, X86_FEATURE_MWAIT))
+	if (!cpu_has(c, X86_FEATURE_MWAIT) && !xen_initial_domain())
 		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
 
 	obj->type = ACPI_TYPE_BUFFER;
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 22baf4c..0a81637 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -419,7 +419,8 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 				cx.entry_method = ACPI_CSTATE_HALT;
 				snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI HLT");
 			} else {
-				continue;
+				if (!xen_initial_domain())
+					continue;
 			}
 			if (cx.type == ACPI_STATE_C1 &&
 					(idle_halt || idle_nomwait)) {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Jeremy Fitzhardinge]

On 04/01/2010 07:38 PM, Wei, Gang wrote:
> I suggest including this patch in Xen 4.0 release. It is quite straightforward, and make sure the better and widely equipped cpu idle entry method MWAIT is used by xen if available.
>
> BTW, can we consider re-open MWAIT freature for dom0 in xen hypervisor? We can discuss it post 4.0.
>
> Jimmy
>
> ACPI: re-enable mwait for xen cpuidle
>
> Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel start to check this feature while initializing _PDC object. Bypass such check in pv-ops case to re-enable mwait for xen cpuidle.
>    

Why not just set or clear the MWAIT feature flag in xen_cpuid?

     J

> Signed-off-by: Wei Gang<gang.wei@intel.com>
> Signed-off-by: Yu Ke<ke.yu@intel.com>
>
> diff --git a/arch/x86/kernel/acpi/processor.c b/arch/x86/kernel/acpi/processor.c
> index 8c9526d..d88866c 100644
> --- a/arch/x86/kernel/acpi/processor.c
> +++ b/arch/x86/kernel/acpi/processor.c
> @@ -60,7 +60,7 @@ static void init_intel_pdc(struct acpi_processor *pr, struct cpuinfo_x86 *c)
>   	/*
>   	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
>   	 */
> -	if (!cpu_has(c, X86_FEATURE_MWAIT))
> +	if (!cpu_has(c, X86_FEATURE_MWAIT)&&  !xen_initial_domain())
>   		buf[2]&= ~(ACPI_PDC_C_C2C3_FFH);
>
>   	obj->type = ACPI_TYPE_BUFFER;
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 22baf4c..0a81637 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -419,7 +419,8 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
>   				cx.entry_method = ACPI_CSTATE_HALT;
>   				snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI HLT");
>   			} else {
> -				continue;
> +				if (!xen_initial_domain())
> +					continue;
>   			}
>   			if (cx.type == ACPI_STATE_C1&&
>   					(idle_halt || idle_nomwait)) {
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>    

RE: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Wei, Gang]

On Friday, 2010-4-2 1:41 PM, Jeremy Fitzhardinge wrote:
> On 04/01/2010 07:38 PM, Wei, Gang wrote:
>> I suggest including this patch in Xen 4.0 release. It is quite
>> straightforward, and make sure the better and widely equipped cpu
>> idle entry method MWAIT is used by xen if available.  
>> 
>> BTW, can we consider re-open MWAIT freature for dom0 in xen
>> hypervisor? We can discuss it post 4.0. 
>> 
>> Jimmy
>> 
>> ACPI: re-enable mwait for xen cpuidle
>> 
>> Xen hypervisor doesn't export mwait feature to dom0, but latest
>> Linux kernel start to check this feature while initializing _PDC
>> object. Bypass such check in pv-ops case to re-enable mwait for xen
>> cpuidle.   
>> 
> 
> Why not just set or clear the MWAIT feature flag in xen_cpuid?

Yes, you pointed out another approach in dom0 side which has similar effect as re-open MWAIT feature for dom0 in xen. I am not sure whether exposing the MWAIT feature to whole dom0 kernel is ok. My way is just to narrow the impact.

Jimmy

RE: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Wei, Gang]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

Here is the alternative patch you expected. 

ACPI: re-enable mwait for xen cpuidle

Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel start to check this feature while initializing _PDC object. Modify xen_cpuid to re-enable mwait for xen cpuidle.

Signed-off-by: Wei Gang <gang.wei@intel.com>

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index c3e8bff..565244f 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -189,6 +189,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	unsigned maskebx = ~0;
 	unsigned maskecx = ~0;
 	unsigned maskedx = ~0;
+	unsigned setecx  =  0;
+	unsigned setedx  =  0;
 
 	/*
 	 * Mask out inconvenient features, to try and disable as many
@@ -198,6 +200,12 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	case 0x1:
 		maskecx = cpuid_leaf1_ecx_mask;
 		maskedx = cpuid_leaf1_edx_mask;
+		setecx  = 1 << (X86_FEATURE_MWAIT % 32);
+		break;
+
+	case 0x5: /* MWAIT INFO */
+		setecx  = 0x3; /* EXTENSIONS_SUPPORTED | INTERRUPT_BREAK */
+		setedx  = ~0;
 		break;
 
 	case 0xb:
@@ -220,6 +228,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	*bx &= maskebx;
 	*cx &= maskecx;
 	*dx &= maskedx;
+	*cx |= setecx;
+	*dx |= setedx;
 }
 
 static __init void xen_init_cpuid_mask(void)

On Friday, 2010-4-2 2:33 PM, Wei, Gang wrote:
> On Friday, 2010-4-2 1:41 PM, Jeremy Fitzhardinge wrote:
>> On 04/01/2010 07:38 PM, Wei, Gang wrote:
>>> I suggest including this patch in Xen 4.0 release. It is quite
>>> straightforward, and make sure the better and widely equipped cpu
>>> idle entry method MWAIT is used by xen if available.
>>> 
>>> BTW, can we consider re-open MWAIT freature for dom0 in xen
>>> hypervisor? We can discuss it post 4.0.
>>> 
>>> Jimmy
>>> 
>>> ACPI: re-enable mwait for xen cpuidle
>>> 
>>> Xen hypervisor doesn't export mwait feature to dom0, but latest
>>> Linux kernel start to check this feature while initializing _PDC
>>> object. Bypass such check in pv-ops case to re-enable mwait for xen
>>> cpuidle. 
>>> 
>> 
>> Why not just set or clear the MWAIT feature flag in xen_cpuid?
> 
> Yes, you pointed out another approach in dom0 side which has similar
> effect as re-open MWAIT feature for dom0 in xen. I am not sure
> whether exposing the MWAIT feature to whole dom0 kernel is ok. My way
> is just to narrow the impact.   
> 
> Jimmy

[-- Attachment #2: re-enable-mwait_pv-ops-dom0-v2.patch --]
[-- Type: application/octet-stream, Size: 1274 bytes --]

ACPI: re-enable mwait for xen cpuidle

Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel start to check this feature while initializing _PDC object. Modify xen_cpuid to re-enable mwait for xen cpuidle.

Signed-off-by: Wei Gang <gang.wei@intel.com>

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index c3e8bff..565244f 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -189,6 +189,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	unsigned maskebx = ~0;
 	unsigned maskecx = ~0;
 	unsigned maskedx = ~0;
+	unsigned setecx  =  0;
+	unsigned setedx  =  0;
 
 	/*
 	 * Mask out inconvenient features, to try and disable as many
@@ -198,6 +200,12 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	case 0x1:
 		maskecx = cpuid_leaf1_ecx_mask;
 		maskedx = cpuid_leaf1_edx_mask;
+		setecx  = 1 << (X86_FEATURE_MWAIT % 32);
+		break;
+
+	case 0x5: /* MWAIT INFO */
+		setecx  = 0x3; /* EXTENSIONS_SUPPORTED | INTERRUPT_BREAK */
+		setedx  = ~0;
 		break;
 
 	case 0xb:
@@ -220,6 +228,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	*bx &= maskebx;
 	*cx &= maskecx;
 	*dx &= maskedx;
+	*cx |= setecx;
+	*dx |= setedx;
 }
 
 static __init void xen_init_cpuid_mask(void)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Wei, Gang]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

The 3rd choice: don't clear MWAIT feature in xen. This should be the best choice.

Jimmy

X86: re-enable mwait for xen cpuidle

Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel
start to check this feature while initializing _PDC object. Pass the MWAIT
feature to dom0 to let dom0 got & pass to xen correct C state info.

Signed-off-by: Wei Gang <gang.wei@intel.com>

diff -r 537451477469 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c	Thu Apr 01 09:55:27 2010 +0100
+++ b/xen/arch/x86/traps.c	Sat Apr 03 05:26:53 2010 +0800
@@ -776,7 +776,6 @@ static void pv_cpuid(struct cpu_user_reg
         __clear_bit(X86_FEATURE_PBE, &d);
 
         __clear_bit(X86_FEATURE_DTES64 % 32, &c);
-        __clear_bit(X86_FEATURE_MWAIT % 32, &c);
         __clear_bit(X86_FEATURE_DSCPL % 32, &c);
         __clear_bit(X86_FEATURE_VMXE % 32, &c);
         __clear_bit(X86_FEATURE_SMXE % 32, &c);
@@ -814,7 +813,6 @@ static void pv_cpuid(struct cpu_user_reg
         __clear_bit(X86_FEATURE_SKINIT % 32, &c);
         __clear_bit(X86_FEATURE_WDT % 32, &c);
         break;
-    case 5: /* MONITOR/MWAIT */
     case 0xa: /* Architectural Performance Monitor Features */
     case 0x8000000a: /* SVM revision and features */
     case 0x8000001b: /* Instruction Based Sampling */

[-- Attachment #2: re-enable-mwait-for-dom0.patch --]
[-- Type: application/octet-stream, Size: 1200 bytes --]

X86: re-enable mwait for xen cpuidle

Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel
start to check this feature while initializing _PDC object. Pass the MWAIT
feature to dom0 to let dom0 got & pass to xen correct C state info.

Signed-off-by: Wei Gang <gang.wei@intel.com>

diff -r 537451477469 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c	Thu Apr 01 09:55:27 2010 +0100
+++ b/xen/arch/x86/traps.c	Sat Apr 03 05:26:53 2010 +0800
@@ -776,7 +776,6 @@ static void pv_cpuid(struct cpu_user_reg
         __clear_bit(X86_FEATURE_PBE, &d);
 
         __clear_bit(X86_FEATURE_DTES64 % 32, &c);
-        __clear_bit(X86_FEATURE_MWAIT % 32, &c);
         __clear_bit(X86_FEATURE_DSCPL % 32, &c);
         __clear_bit(X86_FEATURE_VMXE % 32, &c);
         __clear_bit(X86_FEATURE_SMXE % 32, &c);
@@ -814,7 +813,6 @@ static void pv_cpuid(struct cpu_user_reg
         __clear_bit(X86_FEATURE_SKINIT % 32, &c);
         __clear_bit(X86_FEATURE_WDT % 32, &c);
         break;
-    case 5: /* MONITOR/MWAIT */
     case 0xa: /* Architectural Performance Monitor Features */
     case 0x8000000a: /* SVM revision and features */
     case 0x8000001b: /* Instruction Based Sampling */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Keir Fraser]

It also has wider impact, including questionable effect on existing dom0
kernels which have never been tested with MONITOR/MWAIT present in
virtualised CPUID. I'm not sure about making this change even in current
xen-unstable (but not dead against it), and there's no chance for 4.0 and
3.4 branches.

 -- Keir

On 02/04/2010 14:41, "Wei, Gang" <gang.wei@intel.com> wrote:

> The 3rd choice: don't clear MWAIT feature in xen. This should be the best
> choice.
> 
> Jimmy
> 
> X86: re-enable mwait for xen cpuidle
> 
> Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel
> start to check this feature while initializing _PDC object. Pass the MWAIT
> feature to dom0 to let dom0 got & pass to xen correct C state info.
> 
> Signed-off-by: Wei Gang <gang.wei@intel.com>
> 
> diff -r 537451477469 xen/arch/x86/traps.c
> --- a/xen/arch/x86/traps.c Thu Apr 01 09:55:27 2010 +0100
> +++ b/xen/arch/x86/traps.c Sat Apr 03 05:26:53 2010 +0800
> @@ -776,7 +776,6 @@ static void pv_cpuid(struct cpu_user_reg
>          __clear_bit(X86_FEATURE_PBE, &d);
>  
>          __clear_bit(X86_FEATURE_DTES64 % 32, &c);
> -        __clear_bit(X86_FEATURE_MWAIT % 32, &c);
>          __clear_bit(X86_FEATURE_DSCPL % 32, &c);
>          __clear_bit(X86_FEATURE_VMXE % 32, &c);
>          __clear_bit(X86_FEATURE_SMXE % 32, &c);
> @@ -814,7 +813,6 @@ static void pv_cpuid(struct cpu_user_reg
>          __clear_bit(X86_FEATURE_SKINIT % 32, &c);
>          __clear_bit(X86_FEATURE_WDT % 32, &c);
>          break;
> -    case 5: /* MONITOR/MWAIT */
>      case 0xa: /* Architectural Performance Monitor Features */
>      case 0x8000000a: /* SVM revision and features */
>      case 0x8000001b: /* Instruction Based Sampling */

Re: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Keir Fraser]

Think about it some more: Since this is about partial enabling of
MONITOR/WAIT, just to get C-state info to Xen -- i.e., something a bit hacky
anyway -- the right answer is for Linux to lie to itself and your 2nd patch
attempt is best. I would think of Xen advertising the fature to dom0 via
CPUID as meaning that the guest can use it as a 1st-class feature itself,
which is not the case.

 -- Keir

On 02/04/2010 15:27, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> It also has wider impact, including questionable effect on existing dom0
> kernels which have never been tested with MONITOR/MWAIT present in
> virtualised CPUID. I'm not sure about making this change even in current
> xen-unstable (but not dead against it), and there's no chance for 4.0 and
> 3.4 branches.
> 
>  -- Keir
> 
> On 02/04/2010 14:41, "Wei, Gang" <gang.wei@intel.com> wrote:
> 
>> The 3rd choice: don't clear MWAIT feature in xen. This should be the best
>> choice.
>> 
>> Jimmy
>> 
>> X86: re-enable mwait for xen cpuidle
>> 
>> Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel
>> start to check this feature while initializing _PDC object. Pass the MWAIT
>> feature to dom0 to let dom0 got & pass to xen correct C state info.
>> 
>> Signed-off-by: Wei Gang <gang.wei@intel.com>
>> 
>> diff -r 537451477469 xen/arch/x86/traps.c
>> --- a/xen/arch/x86/traps.c Thu Apr 01 09:55:27 2010 +0100
>> +++ b/xen/arch/x86/traps.c Sat Apr 03 05:26:53 2010 +0800
>> @@ -776,7 +776,6 @@ static void pv_cpuid(struct cpu_user_reg
>>          __clear_bit(X86_FEATURE_PBE, &d);
>>  
>>          __clear_bit(X86_FEATURE_DTES64 % 32, &c);
>> -        __clear_bit(X86_FEATURE_MWAIT % 32, &c);
>>          __clear_bit(X86_FEATURE_DSCPL % 32, &c);
>>          __clear_bit(X86_FEATURE_VMXE % 32, &c);
>>          __clear_bit(X86_FEATURE_SMXE % 32, &c);
>> @@ -814,7 +813,6 @@ static void pv_cpuid(struct cpu_user_reg
>>          __clear_bit(X86_FEATURE_SKINIT % 32, &c);
>>          __clear_bit(X86_FEATURE_WDT % 32, &c);
>>          break;
>> -    case 5: /* MONITOR/MWAIT */
>>      case 0xa: /* Architectural Performance Monitor Features */
>>      case 0x8000000a: /* SVM revision and features */
>>      case 0x8000001b: /* Instruction Based Sampling */
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Wei, Gang]

On Friday, 2010-4-2 10:46 PM, Keir Fraser wrote:
> Think about it some more: Since this is about partial enabling of
> MONITOR/WAIT, just to get C-state info to Xen -- i.e., something a
> bit hacky anyway -- the right answer is for Linux to lie to itself
> and your 2nd patch attempt is best. I would think of Xen advertising
> the fature to dom0 via CPUID as meaning that the guest can use it as
> a 1st-class feature itself, which is not the case.

Yes, you tell the truth. That is why I consider the 1st & 2nd patch first.

Jimmy

RE: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Wei, Gang]

[-- Attachment #1: Type: text/plain, Size: 2318 bytes --]

Updated the 2nd patch, only set MWAIT feature for dom0.

Jimmy

ACPI: re-enable mwait for xen cpuidle

Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel start to check this feature while initializing _PDC object. Modify xen_cpuid to re-enable mwait for xen cpuidle.

Signed-off-by: Wei Gang <gang.wei@intel.com>

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index c3e8bff..82f3826 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -182,6 +182,9 @@ static void __init xen_banner(void)
 static __read_mostly unsigned int cpuid_leaf1_edx_mask = ~0;
 static __read_mostly unsigned int cpuid_leaf1_ecx_mask = ~0;
 static __read_mostly unsigned int cpuid_leaf81_edx_mask = ~0;
+static __read_mostly unsigned int cpuid_leaf1_ecx_set;
+static __read_mostly unsigned int cpuid_leaf5_ecx_set;
+static __read_mostly unsigned int cpuid_leaf5_edx_set;
 
 static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
@@ -189,6 +192,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	unsigned maskebx = ~0;
 	unsigned maskecx = ~0;
 	unsigned maskedx = ~0;
+	unsigned setecx  =  0;
+	unsigned setedx  =  0;
 
 	/*
 	 * Mask out inconvenient features, to try and disable as many
@@ -198,6 +203,12 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	case 0x1:
 		maskecx = cpuid_leaf1_ecx_mask;
 		maskedx = cpuid_leaf1_edx_mask;
+		setecx  = cpuid_leaf1_ecx_set;
+		break;
+
+	case 0x5: /* MWAIT INFO */
+		setecx = cpuid_leaf5_ecx_set;
+		setedx = cpuid_leaf5_edx_set;
 		break;
 
 	case 0xb:
@@ -220,6 +231,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	*bx &= maskebx;
 	*cx &= maskecx;
 	*dx &= maskedx;
+	*cx |= setecx;
+	*dx |= setedx;
 }
 
 static __init void xen_init_cpuid_mask(void)
@@ -238,6 +251,11 @@ static __init void xen_init_cpuid_mask(void)
 			  (1 << X86_FEATURE_MCA)  |  /* disable MCA */
 			  (1 << X86_FEATURE_APIC) |  /* disable local APIC */
 			  (1 << X86_FEATURE_ACPI));  /* disable ACPI */
+	else {
+		cpuid_leaf1_ecx_set = 1 << (X86_FEATURE_MWAIT % 32);
+		cpuid_leaf5_ecx_set = 0x3; /* EXTENSIONS_SUPPORTED | INTERRUPT_BREAK */
+		cpuid_leaf5_edx_set = ~0;
+	}
 
 	ax = 1;
 	cx = 0;

[-- Attachment #2: re-enable-mwait_pv-ops-dom0-v2.1.patch --]
[-- Type: application/octet-stream, Size: 2189 bytes --]

ACPI: re-enable mwait for xen cpuidle

Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel start to check this feature while initializing _PDC object. Modify xen_cpuid to re-enable mwait for xen cpuidle.

Signed-off-by: Wei Gang <gang.wei@intel.com>

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index c3e8bff..82f3826 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -182,6 +182,9 @@ static void __init xen_banner(void)
 static __read_mostly unsigned int cpuid_leaf1_edx_mask = ~0;
 static __read_mostly unsigned int cpuid_leaf1_ecx_mask = ~0;
 static __read_mostly unsigned int cpuid_leaf81_edx_mask = ~0;
+static __read_mostly unsigned int cpuid_leaf1_ecx_set;
+static __read_mostly unsigned int cpuid_leaf5_ecx_set;
+static __read_mostly unsigned int cpuid_leaf5_edx_set;
 
 static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
@@ -189,6 +192,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	unsigned maskebx = ~0;
 	unsigned maskecx = ~0;
 	unsigned maskedx = ~0;
+	unsigned setecx  =  0;
+	unsigned setedx  =  0;
 
 	/*
 	 * Mask out inconvenient features, to try and disable as many
@@ -198,6 +203,12 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	case 0x1:
 		maskecx = cpuid_leaf1_ecx_mask;
 		maskedx = cpuid_leaf1_edx_mask;
+		setecx  = cpuid_leaf1_ecx_set;
+		break;
+
+	case 0x5: /* MWAIT INFO */
+		setecx = cpuid_leaf5_ecx_set;
+		setedx = cpuid_leaf5_edx_set;
 		break;
 
 	case 0xb:
@@ -220,6 +231,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	*bx &= maskebx;
 	*cx &= maskecx;
 	*dx &= maskedx;
+	*cx |= setecx;
+	*dx |= setedx;
 }
 
 static __init void xen_init_cpuid_mask(void)
@@ -238,6 +251,11 @@ static __init void xen_init_cpuid_mask(void)
 			  (1 << X86_FEATURE_MCA)  |  /* disable MCA */
 			  (1 << X86_FEATURE_APIC) |  /* disable local APIC */
 			  (1 << X86_FEATURE_ACPI));  /* disable ACPI */
+	else {
+		cpuid_leaf1_ecx_set = 1 << (X86_FEATURE_MWAIT % 32);
+		cpuid_leaf5_ecx_set = 0x3; /* EXTENSIONS_SUPPORTED | INTERRUPT_BREAK */
+		cpuid_leaf5_edx_set = ~0;
+	}
 
 	ax = 1;
 	cx = 0;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Jeremy Fitzhardinge]

On 04/02/2010 09:27 AM, Wei, Gang wrote:
> Updated the 2nd patch, only set MWAIT feature for dom0.
>
> Jimmy
>
> ACPI: re-enable mwait for xen cpuidle
>
> Xen hypervisor doesn't export mwait feature to dom0, but latest Linux kernel start to check this feature while initializing _PDC object. Modify xen_cpuid to re-enable mwait for xen cpuidle.
>    

What if the CPU really doesn't have MWAIT?

But I agree with your original assessment that setting MWAIT just to get 
a couple of paths in ACPI parsing enabled is probably overkill, but I 
don't like the idea of putting xen-specific tests into the acpi code.

Would it be possible to change the parser code to parse unconditionally 
and then ignore the MWAIT-specific stuff later on?  (I haven't looked at 
the structure of the code, so I'm not sure if this suggestion even makes 
sense.)

     J

> Signed-off-by: Wei Gang<gang.wei@intel.com>
>
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index c3e8bff..82f3826 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -182,6 +182,9 @@ static void __init xen_banner(void)
>   static __read_mostly unsigned int cpuid_leaf1_edx_mask = ~0;
>   static __read_mostly unsigned int cpuid_leaf1_ecx_mask = ~0;
>   static __read_mostly unsigned int cpuid_leaf81_edx_mask = ~0;
> +static __read_mostly unsigned int cpuid_leaf1_ecx_set;
> +static __read_mostly unsigned int cpuid_leaf5_ecx_set;
> +static __read_mostly unsigned int cpuid_leaf5_edx_set;
>
>   static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>   		      unsigned int *cx, unsigned int *dx)
> @@ -189,6 +192,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>   	unsigned maskebx = ~0;
>   	unsigned maskecx = ~0;
>   	unsigned maskedx = ~0;
> +	unsigned setecx  =  0;
> +	unsigned setedx  =  0;
>
>   	/*
>   	 * Mask out inconvenient features, to try and disable as many
> @@ -198,6 +203,12 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>   	case 0x1:
>   		maskecx = cpuid_leaf1_ecx_mask;
>   		maskedx = cpuid_leaf1_edx_mask;
> +		setecx  = cpuid_leaf1_ecx_set;
> +		break;
> +
> +	case 0x5: /* MWAIT INFO */
> +		setecx = cpuid_leaf5_ecx_set;
> +		setedx = cpuid_leaf5_edx_set;
>   		break;
>
>   	case 0xb:
> @@ -220,6 +231,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>   	*bx&= maskebx;
>   	*cx&= maskecx;
>   	*dx&= maskedx;
> +	*cx |= setecx;
> +	*dx |= setedx;
>   }
>
>   static __init void xen_init_cpuid_mask(void)
> @@ -238,6 +251,11 @@ static __init void xen_init_cpuid_mask(void)
>   			  (1<<  X86_FEATURE_MCA)  |  /* disable MCA */
>   			  (1<<  X86_FEATURE_APIC) |  /* disable local APIC */
>   			  (1<<  X86_FEATURE_ACPI));  /* disable ACPI */
> +	else {
> +		cpuid_leaf1_ecx_set = 1<<  (X86_FEATURE_MWAIT % 32);
> +		cpuid_leaf5_ecx_set = 0x3; /* EXTENSIONS_SUPPORTED | INTERRUPT_BREAK */
> +		cpuid_leaf5_edx_set = ~0;
> +	}
>
>   	ax = 1;
>   	cx = 0;

RE: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Wei, Gang]

On Saturday, 2010-4-3 2:33 AM, Jeremy Fitzhardinge wrote:
> On 04/02/2010 09:27 AM, Wei, Gang wrote:
>> Updated the 2nd patch, only set MWAIT feature for dom0.
>> 
>> Jimmy
>> 
>> ACPI: re-enable mwait for xen cpuidle
>> 
>> Xen hypervisor doesn't export mwait feature to dom0, but latest
>> Linux kernel start to check this feature while initializing _PDC
>> object. Modify xen_cpuid to re-enable mwait for xen cpuidle.  
>> 
> 
> What if the CPU really doesn't have MWAIT?

If the CPU really doesn't have MWAIT, BIOS should know it and BIOS acpi code should not give C state table with MWAIT entry method. Even the BIOS give the wrong MWAIT C state info, xen cpuidle will refuse it and mark that C state as invalid.

> But I agree with your original assessment that setting MWAIT just to
> get a couple of paths in ACPI parsing enabled is probably overkill,
> but I don't like the idea of putting xen-specific tests into the acpi
> code. 

I don't like it too. But some time we have to accept a workaround temporarily even we don't like it, until we find a graceful solution.

> Would it be possible to change the parser code to parse
> unconditionally and then ignore the MWAIT-specific stuff later on? 
> (I haven't looked at the structure of the code, so I'm not sure if
> this suggestion even makes sense.)

That means to turn back to old change set. In 2.6.18, this check doesn't exist in the parser code path. I have to say, these checks made Linux kernel itself more robust. I am not sure whether we can find a better way which is also compatible with Xen's need.

Jimmy

Re: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Jeremy Fitzhardinge]

On 04/03/2010 07:07 AM, Wei, Gang wrote:
> On Saturday, 2010-4-3 2:33 AM, Jeremy Fitzhardinge wrote:
>    
>> On 04/02/2010 09:27 AM, Wei, Gang wrote:
>>      
>>> Updated the 2nd patch, only set MWAIT feature for dom0.
>>>
>>> Jimmy
>>>
>>> ACPI: re-enable mwait for xen cpuidle
>>>
>>> Xen hypervisor doesn't export mwait feature to dom0, but latest
>>> Linux kernel start to check this feature while initializing _PDC
>>> object. Modify xen_cpuid to re-enable mwait for xen cpuidle.
>>>
>>>        
>> What if the CPU really doesn't have MWAIT?
>>      
> If the CPU really doesn't have MWAIT, BIOS should know it and BIOS acpi code should not give C state table with MWAIT entry method. Even the BIOS give the wrong MWAIT C state info, xen cpuidle will refuse it and mark that C state as invalid.
>
>    
>> But I agree with your original assessment that setting MWAIT just to
>> get a couple of paths in ACPI parsing enabled is probably overkill,
>> but I don't like the idea of putting xen-specific tests into the acpi
>> code.
>>      
> I don't like it too. But some time we have to accept a workaround temporarily even we don't like it, until we find a graceful solution.
>
>    
>> Would it be possible to change the parser code to parse
>> unconditionally and then ignore the MWAIT-specific stuff later on?
>> (I haven't looked at the structure of the code, so I'm not sure if
>> this suggestion even makes sense.)
>>      
> That means to turn back to old change set. In 2.6.18, this check doesn't exist in the parser code path. I have to say, these checks made Linux kernel itself more robust. I am not sure whether we can find a better way which is also compatible with Xen's need.
>    

I had a closer look at the code, and I don't really understand it:

			if (acpi_processor_ffh_cstate_probe
					(pr->id,&cx, reg) == 0) {
				cx.entry_method = ACPI_CSTATE_FFH;
[1]			} else if (cx.type == ACPI_STATE_C1) {
				/*
				 * C1 is a special case where FIXED_HARDWARE
				 * can be handled in non-MWAIT way as well.
				 * In that case, save this _CST entry info.
				 * Otherwise, ignore this info and continue.
				 */
[2]				cx.entry_method = ACPI_CSTATE_HALT;
[3]				snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI HLT");
			} else {
				continue;
			}
[4]			if (cx.type == ACPI_STATE_C1&&
					(idle_halt || idle_nomwait)) {
				/*
				 * In most cases the C1 space_id obtained from
				 * _CST object is FIXED_HARDWARE access mode.
				 * But when the option of idle=halt is added,
				 * the entry_method type should be changed from
				 * CSTATE_FFH to CSTATE_HALT.
				 * When the option of idle=nomwait is added,
				 * the C1 entry_method type should be
				 * CSTATE_HALT.
				 */
[5]				cx.entry_method = ACPI_CSTATE_HALT;
[6]				snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI HLT");
			}


What's the if() at [1] doing?  If it succeeds, then it does [2,3] then 
falls into [4], which does the same test as [1] but also checks for 
idle_halt || idle_nomwait and then performs [5,6] which looks identical 
to [2,3].  It all seems a bit excessively convoluted, so I'm not quite 
sure how your patch interacts with this.

     J

RE: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Wei, Gang]

On Tuesday, 2010-4-6 2:31 AM, Jeremy Fitzhardinge wrote:
> 
> I had a closer look at the code, and I don't really understand it:
> 
> 			if (acpi_processor_ffh_cstate_probe
> 					(pr->id,&cx, reg) == 0) {
> 				cx.entry_method = ACPI_CSTATE_FFH;
> [1]			} else if (cx.type == ACPI_STATE_C1) {
> 				/*
> 				 * C1 is a special case where FIXED_HARDWARE
> 				 * can be handled in non-MWAIT way as well.
> 				 * In that case, save this _CST entry info.
> 				 * Otherwise, ignore this info and continue.
> 				 */
> [2]				cx.entry_method = ACPI_CSTATE_HALT;
> [3]				snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI HLT");
> 			} else {
> 				continue;
> 			}
> [4]			if (cx.type == ACPI_STATE_C1&&
> 					(idle_halt || idle_nomwait)) {
> 				/*
> 				 * In most cases the C1 space_id obtained from
> 				 * _CST object is FIXED_HARDWARE access mode.
> 				 * But when the option of idle=halt is added,
> 				 * the entry_method type should be changed from
> 				 * CSTATE_FFH to CSTATE_HALT.
> 				 * When the option of idle=nomwait is added,
> 				 * the C1 entry_method type should be
> 				 * CSTATE_HALT.
> 				 */
> [5]				cx.entry_method = ACPI_CSTATE_HALT;
> [6]				snprintf(cx.desc, ACPI_CX_DESC_LEN, "ACPI HLT");
> 			}
> 
> 
> What's the if() at [1] doing?  If it succeeds, then it does [2,3] then
> falls into [4], which does the same test as [1] but also checks for
> idle_halt || idle_nomwait and then performs [5,6] which looks
> identical to [2,3].  It all seems a bit excessively convoluted, so
> I'm not quite sure how your patch interacts with this.

Those two if()s do the identical things for different condition.
 
When the acpi table tells a C-state w/ MWAIT entry method, but this C-state can't pass the check - no MWAIT feature or this C-state info doesn't conform to cpuid leaf5 info, then this C-state should be ignored. There is a exception. C1 can always be entered w/ halt instruction, so for C1 just turn back to HALT. That is the if() at [1] doing.

The if() at [4] just tries to change the C1 entry_method to HALT if either option 'idle=halt' or 'idle=nomwait' is specified even if the h/w really support MWAIT.

My patch just ensure all row info in reg can be cached and passed to Xen. The change to cx.entry_method doesn't impact my patch. The dom0 option 'idle=halt' and 'idle=nomwait' should not be used. Xen cpuidle should be controlled by Xen itself, all we did in dom0 is trying to get the completed acpi cstate info.

Jimmy