thr3ads.net - Xen devel - [Xen-devel] [PATCH]ACPI: re-enable mwait for xen cpuidle [Apr 2010]

If this information is useful, please help other people find it:
Share via:

Wei, Gang

2010-Apr-02 02:38 UTC

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

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)) {

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

Jeremy Fitzhardinge

2010-Apr-02 05:40 UTC

head link

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

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
>    

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

Wei, Gang

2010-Apr-02 06:32 UTC

head link

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

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


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

Wei, Gang

2010-Apr-02 09:34 UTC

head link

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

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

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

Wei, Gang

2010-Apr-02 13:41 UTC

head link

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

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

Keir Fraser

2010-Apr-02 14:27 UTC

head link

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

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

Keir Fraser

2010-Apr-02 14:46 UTC

head link

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

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


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

Wei, Gang

2010-Apr-02 15:18 UTC

head link

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

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Wei, Gang

2010-Apr-02 16:27 UTC

head link

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

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;

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

Jeremy Fitzhardinge

2010-Apr-02 18:33 UTC

head link

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

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;

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

Wei, Gang

2010-Apr-03 14:07 UTC

head link

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

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


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

Jeremy Fitzhardinge

2010-Apr-05 18:30 UTC

head link

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

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

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

Wei, Gang

2010-Apr-06 01:46 UTC

head link

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

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Apr 2010 - [PATCH]ACPI: re-enable mwait for xen cpuidle

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

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

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

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

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

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

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

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

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

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

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

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

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