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: "Wei, Gang" <gang.wei@xxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH]ACPI: re-enable mwait for xen cpuidle
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Mon, 05 Apr 2010 11:30:53 -0700
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Keir Fraser <keir.fraser@xxxxxxxxxxxxx>, "Yu, Ke" <ke.yu@xxxxxxxxx>
Delivery-date: Mon, 05 Apr 2010 11:31:33 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <E6467867A6B05E4FA831B7DF29925F5C40E67AC0@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
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>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100330 Fedora/3.0.4-1.fc12 Lightning/1.0b2pre Thunderbird/3.0.4
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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel