Re: [Xen-devel] [PATCH 3/4] xen/x86: Replace incorrect mandatory barriers with SMP barriers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.12.16 at 15:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 05/12/16 14:03, Jan Beulich wrote:
>>>>> On 05.12.16 at 14:29, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 05/12/16 11:47, Jan Beulich wrote:
>>>>>>> On 05.12.16 at 11:05, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> --- a/xen/arch/x86/acpi/cpu_idle.c
>>>>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>>>>> @@ -391,9 +391,9 @@ void mwait_idle_with_hints(unsigned int eax, unsigned 
>>>>> int 
> ecx)
>>>>>  
>>>>>      if ( boot_cpu_has(X86_FEATURE_CLFLUSH_MONITOR) )
>>>>>      {
>>>>> -        mb();
>>>>> +        smp_mb();
>>>>>          clflush((void *)&mwait_wakeup(cpu));
>>>>> -        mb();
>>>>> +        smp_mb();
>>>>>      }
>>>> Both need to stay as they are afaict: In order for the clflush() to do
>>>> what we want we have to order it wrt earlier as well as later writes,
>>>> regardless of SMP-ness. Or wait - the SDM has changed in that
>>>> respect (and a footnote describes the earlier specified behavior now).
>>>> AMD, otoh, continues to require MFENCE for ordering purposes.
>>> mb() == smp_mb().  They are both mfence instructions.
>> Of course. But still smp_mb() would be less correct from an
>> abstract perspective
> 
> ? That is entirely the purpose and intended meaning of the abstraction.
> 
> smp_mb() orders operations such that (visible to other CPUs in the
> system), all writes will have completed before any subsequent reads begin.

Yes, but this code is not about multiple CPUs, but just the local
one (we want to make sure CLFLUSH does what we want). Hence
using smp_ prefixed barriers would be wrong. But anyway, with
this becoming explicit mfence() invocations, the discussion is all
moot now.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel