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
|
Home