WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Subject: RE: [Xen-devel] [PATCH]ACPI: re-enable mwait for xen cpuidle
From: "Wei, Gang" <gang.wei@xxxxxxxxx>
Date: Tue, 6 Apr 2010 09:46:45 +0800
Accept-language: zh-CN, en-US
Acceptlanguage: zh-CN, en-US
Cc: Keir, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Fraser <keir.fraser@xxxxxxxxxxxxx>, "Yu, Ke" <ke.yu@xxxxxxxxx>
Delivery-date: Mon, 05 Apr 2010 18:49:15 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4BBA2C5D.3060204@xxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <E6467867A6B05E4FA831B7DF29925F5C40E677DD@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <4BB58353.5000008@xxxxxxxx> <E6467867A6B05E4FA831B7DF29925F5C40E67932@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <E6467867A6B05E4FA831B7DF29925F5C40E67A39@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <E6467867A6B05E4FA831B7DF29925F5C40E67A90@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <4BB6385D.4090404@xxxxxxxx> <E6467867A6B05E4FA831B7DF29925F5C40E67AC0@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <4BBA2C5D.3060204@xxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AcrU7iMOEDZ3wEQYTkiAt1L0Y7Nh/gANngVQ
Thread-topic: [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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel