[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.