Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Boris Ostrovsky <boris.ostrovsky@amd.com>]

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<boris.ostrovsky@amd.com>  # 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<boris.ostrovsky@amd.com>
>>>
>>> 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
>
>