Re: [PATCH]ACPI: re-enable mwait for xen cpuidle

[Jeremy Fitzhardinge <jeremy@goop.org>]

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