Keir Fraser wrote:
> On 11/03/2011 17:53, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>
>> Keir Fraser wrote:
>>> On 11/03/2011 16:50, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>>>
>>>> we did experiment, if did wbinvd at current position (at
>>>> play_dead), sometimes it bring strange issue when repeatly cpu
>>>> offline/online. so for cpu dead, the near wbinvd to last step, the
>>>> safer.
>>>> wbinvd would better be the last ops before cpu dead, to avoid
>>>> potential cache coherency break.
>>>
>>> Okay, I applied your patches. However in a follow-up patch (c/s
>>> 23025) I have removed the WBINVD instructions from the default paths
>>> (i.e., the HLT loops) as the CPU still does cache coherency while
>>> in HLT/C1 state.
>>>
>>> Does that look okay to you?
>>>
>>> -- Keir
>>
>> It's right that cpu continue snoop bus transaction to keep cache
>> coherency when HLT/C1.
>>
>> However, I think it better not remove wbinvd from HLT loops.
>> I'm not quite sure if 'enter Cx' == 'cpu play dead'. After all,
>> resume from Cx different from cpu booting: when cpu online, it start
>> from SIPI-SIPI.
>> I worried some unknown hardware behavior will bring issue if we
>> treat 'cpu play dead' totally same as 'enter Cx'.
>> Maybe that's the reason why kernel insist on reserveing wbinvd
>> before HLT when play dead.
>
> An APIC INIT doesn't touch the memory hierarchy, cache controls, nor
> CR0.CD and CR0.NW. The care over cache invalidation I believe solely
> comes down to the deep sleep that the CPU can be placed into via
> MWAIT, in which the cache-control logic gets switched off.
>
> -- Keir
Reasonable ot me, thanks!
Jinsong
>
>> Thanks,
>> Jinsong
>>
>>>
>>>> In fact, it can do wbinvd inside loop, but as cpu_offline_3.patch
>>>> said, at Xen 7400 when hyperthreading, the offlined thread may be
>>>> spuriously waken up by its brother, and frequently waken inside
>>>> the dead loop. In such case, considering heavy workload of wbinvd,
>>>> we add a light-weight clflush instruction inside loop.
>>>>
>>>> Thanks,
>>>> Jinsong
>>>>
>>>>
>>>>>
>>>>>> Signed-off-by: Liu, Jinsong <jinsong.liu@xxxxxxxxx>
>>>>>>
>>>>>> diff -r 2dc3c1cc1bba xen/arch/x86/acpi/cpu_idle.c
>>>>>> --- a/xen/arch/x86/acpi/cpu_idle.c Mon Mar 07 05:31:46 2022 +0800
>>>>>> +++ b/xen/arch/x86/acpi/cpu_idle.c Thu Mar 10 23:40:51 2022 +0800
>>>>>> @@ -562,11 +562,14 @@ static void acpi_dead_idle(void)
>>>>>> if ( (cx = &power->states[power->count-1]) == NULL )
>>>>>> goto default_halt;
>>>>>>
>>>>>> + /*
>>>>>> + * cache must be flashed as the last ops before cpu going
>>>>>> into dead, + * otherwise, cpu may dead with dirty data
>>>>>> breaking cache coherency, + * leading to strange errors. +
>>>>>> */ + wbinvd();
>>>>>> for ( ; ; )
>>>>>> {
>>>>>> - if ( !power->flags.bm_check && cx->type ==
>>>>>> ACPI_STATE_C3 )
>>>>>> - ACPI_FLUSH_CPU_CACHE();
>>>>>> -
>>>>>> switch ( cx->entry_method )
>>>>>> {
>>>>>> case ACPI_CSTATE_EM_FFH:
>>>>>> @@ -584,6 +587,7 @@ static void acpi_dead_idle(void) }
>>>>>>
>>>>>> default_halt:
>>>>>> + wbinvd();
>>>>>> for ( ; ; )
>>>>>> halt();
>>>>>> }
>>>>>> diff -r 2dc3c1cc1bba xen/arch/x86/domain.c
>>>>>> --- a/xen/arch/x86/domain.c Mon Mar 07 05:31:46 2022 +0800
>>>>>> +++ b/xen/arch/x86/domain.c Thu Mar 10 23:40:51 2022 +0800
>>>>>> @@ -93,6 +93,12 @@ static void default_idle(void)
>>>>>>
>>>>>> static void default_dead_idle(void)
>>>>>> {
>>>>>> + /*
>>>>>> + * cache must be flashed as the last ops before cpu going
>>>>>> into dead, + * otherwise, cpu may dead with dirty data
>>>>>> breaking cache coherency, + * leading to strange errors. +
>>>>>> */ + wbinvd();
>>>>>> for ( ; ; )
>>>>>> halt();
>>>>>> }
>>>>>> @@ -100,7 +106,6 @@ static void play_dead(void)
>>>>>> static void play_dead(void)
>>>>>> {
>>>>>> local_irq_disable();
>>>>>> - wbinvd();
>>>>>>
>>>>>> /*
>>>>>> * NOTE: After cpu_exit_clear, per-cpu variables are no
>>>>>> longer accessible,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home