From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] x86: Use deep C states for off-lined CPUs Date: Wed, 29 Feb 2012 09:55:46 -0500 Message-ID: <4F4E3C72.2030101@amd.com> References: <9e5991ad9c85b5176ce2.1330466910@wotan.osrc.amd.com>, <12EBBFBA42012542B30BB4E52B6DCEEB03189495@sausexdag02.amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Liu, Jinsong" Cc: "Zhang, Yang Z" , "xen-devel@lists.xensource.com" , "Wei, Gang" List-Id: xen-devel@lists.xenproject.org On 02/29/12 00:21, Liu, Jinsong wrote: > Hmm, no. > > It need flush cache, as long as *deep Cx* would be spurious-wokenup. > The reason clflush here is, it's a light-weight flush, in fact it also could use wbinvd if not consider performance. What address would need to be CFLUSH'd ? Both "address" and "pmtmr_ioport_local"? > > For halt, it don't need to do so since cpu still keep snoop when sleep. If cpu not snoop when in deeper C-states, wouldn't we have a problem with acpi_idle_do_entry()? There is a code path (at least for for C2) where the cache is not flushed. Incidentally, if CFLUSH is required for MONITOR then perhaps mwait_idle_with_hints() needs to have it as well? -boris > > Thanks, > Jinsong > > Ostrovsky, Boris wrote: >> The patch is adding IO-based C-states. My understading is that CFLUSH >> was to work around a MONITOR-related erratum. >> >> Or are you referring to something else? >> >> -boris >> >> >> ________________________________________ >> From: Zhang, Yang Z [yang.z.zhang@intel.com] >> Sent: Tuesday, February 28, 2012 8:37 PM >> To: Ostrovsky, Boris; xen-devel@lists.xensource.com >> Subject: RE: [Xen-devel] [PATCH] x86: Use deep C states for off-lined >> CPUs >> >> I noticed the following comments when using mwait based idle: >> ------------------------------------------------------------------------- >> while ( 1 ) >> { >> /* >> * 1. The CLFLUSH is a workaround for erratum AAI65 for >> * the Xeon 7400 series. >> * 2. The WBINVD is insufficient due to the >> spurious-wakeup >> * case where we return around the loop. >> * 3. Unlike wbinvd, clflush is a light weight but not >> serializing >> * instruction, hence memory fence is necessary to make >> sure all >> * load/store visible before flush cache line. >> */ >> mb(); >> clflush(mwait_ptr); >> __monitor(mwait_ptr, 0, 0); >> mb(); >> __mwait(cx->address, 0); >> } >> } >> ------------------------------------------------------------------------- >> Your patch should follow it too. >> >> best regards >> yang >> >> >>> -----Original Message----- >>> From: xen-devel-bounces@lists.xen.org >>> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Boris Ostrovsky >>> Sent: Wednesday, February 29, 2012 6:09 AM >>> To: xen-devel@lists.xensource.com >>> Cc: boris.ostrovsky@amd.com >>> Subject: [Xen-devel] [PATCH] x86: Use deep C states for off-lined >>> CPUs >>> >>> # HG changeset patch >>> # User Boris Ostrovsky # Date 1330466573 >>> -3600 # Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c >>> # Parent a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9 >>> x86: Use deep C states for off-lined CPUs >>> >>> Currently when a core is taken off-line it is placed in C1 state >>> (unless MONITOR/MWAIT is used). This patch allows a core to go to >>> deeper C states resulting in significantly higher power savings. >>> >>> Signed-off-by: Boris Ostrovsky >>> >>> diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c >>> --- a/xen/arch/x86/acpi/cpu_idle.c Mon Feb 27 17:05:18 2012 +0000 >>> +++ b/xen/arch/x86/acpi/cpu_idle.c Tue Feb 28 23:02:53 2012 +0100 >>> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void) >>> if ( (cx =&power->states[power->count-1]) == NULL ) >>> goto default_halt; >>> >>> - mwait_ptr = (void *)&mwait_wakeup(smp_processor_id()); - >>> if ( cx->entry_method == ACPI_CSTATE_EM_FFH ) >>> { >>> + mwait_ptr = (void *)&mwait_wakeup(smp_processor_id()); + >>> /* >>> * Cache must be flushed as the last operation before >>> sleeping. >>> * Otherwise, CPU may still hold dirty data, breaking cache >>> coherency, @@ -601,6 +601,20 @@ static void acpi_dead_idle(void) >>> mb(); __mwait(cx->address, 0); >>> } >>> + } >>> + else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO ) + { >>> + /* Avoid references to shared data after the cache flush */ >>> + u32 address = cx->address; >>> + u32 pmtmr_ioport_local = pmtmr_ioport; >>> + >>> + wbinvd(); >>> + >>> + while ( 1 ) >>> + { >>> + inb(address); >>> + inl(pmtmr_ioport_local); >>> + } >>> } >>> >>> default_halt: >>> >>> >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xen.org >>> http://lists.xen.org/xen-devel >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > >