From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeremy Fitzhardinge Subject: Re: [PATCH]ACPI: re-enable mwait for xen cpuidle Date: Mon, 05 Apr 2010 11:30:53 -0700 Message-ID: <4BBA2C5D.3060204@goop.org> References: <4BB58353.5000008@goop.org> <4BB6385D.4090404@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Wei, Gang" Cc: "xen-devel@lists.xensource.com" , Keir Fraser , "Yu, Ke" List-Id: xen-devel@lists.xenproject.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