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

["Wei, Gang" <gang.wei@xxxxxxxxx>]

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