[Patch] x86: enforce strict memory order for x2apic

[Patch] x86: enforce strict memory order for x2apic

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

enforce strict memory order for x2apic.

x2apic allows using MSR to access APIC registers. For
efficient access, serializing semantics of WRMSR to
APIC regs are relaxed. This breaks some assumptions
made on old MMIO access style. In those places, wmb()
is used which is a nop on x86, as MMIO APIC regs are
mapped in uncacheable style which enforces program
order. However when x2apic is enabled, it's possible
for wrmsr reordered even before earlier global data
writes. We observed occasional crash in
smp_call_function_interrupt, where stale call_data
is referenced.

We kick a fix by simply using 'mb' instead of 'wmb'.
This can effectively enforce program order, as args
for WRMSR are loaded from memory. Those memory loads
are ordered by mb and so does WRMSR. Alternative is
to add a 'fence' parameter to send_IPI_mask and then
only use 'mb' in x2apic version. We think it's not
necessary to go with that complexity.

Signed-off-by Kevin Tian <kevin.tian@intel.com>
Signed-off-by Yunhong Jiang <yunhong.jiang@intel.com>

diff -r cfa7cabb56cb xen/arch/x86/smp.c
--- a/xen/arch/x86/smp.c        Thu Sep 25 05:33:35 2008 +0800
+++ b/xen/arch/x86/smp.c        Thu Sep 25 05:52:03 2008 +0800
@@ -186,6 +186,7 @@ void flush_area_mask(cpumask_t mask, con
         flush_cpumask = mask;
         flush_va      = va;
         flush_flags   = flags;
+        mb();
         send_IPI_mask(mask, INVALIDATE_TLB_VECTOR);
         while ( !cpus_empty(flush_cpumask) )
             cpu_relax();
@@ -280,7 +281,7 @@ int on_selected_cpus(
     spin_lock(&call_lock);

     call_data = &data;
-    wmb();
+    mb();

     send_IPI_mask(selected, CALL_FUNCTION_VECTOR);

[-- Attachment #2: x2apic-ipi.patch --]
[-- Type: application/octet-stream, Size: 1650 bytes --]

enforce strict memory order for x2apic.

x2apic allows using MSR to access APIC registers. For
efficient access, serializing semantics of WRMSR to
APIC regs are relaxed. This breaks some assumptions
made on old MMIO access style. In those places, wmb() 
is used which is a nop on x86, as MMIO APIC regs are
mapped in uncacheable style which enforces program
order. However when x2apic is enabled, it's possible
for wrmsr reordered even before earlier global data
writes. We observed occasional crash in 
smp_call_function_interrupt, where stale call_data
is referenced.

We kick a fix by simply using 'mb' instead of 'wmb'.
This can effectively enforce program order, as args
for WRMSR are loaded from memory. Those memory loads
are ordered by mb and so does WRMSR. Alternative is 
to add a 'fence' parameter to send_IPI_mask and then 
only use 'mb' in x2apic version. We think it's not 
necessary to go with that complexity.

Signed-off-by Kevin Tian <kevin.tian@intel.com>
Signed-off-by Yunhong Jiang <yunhong.jiang@intel.com>

diff -r cfa7cabb56cb xen/arch/x86/smp.c
--- a/xen/arch/x86/smp.c	Thu Sep 25 05:33:35 2008 +0800
+++ b/xen/arch/x86/smp.c	Thu Sep 25 05:52:03 2008 +0800
@@ -186,6 +186,7 @@ void flush_area_mask(cpumask_t mask, con
         flush_cpumask = mask;
         flush_va      = va;
         flush_flags   = flags;
+        mb();
         send_IPI_mask(mask, INVALIDATE_TLB_VECTOR);
         while ( !cpus_empty(flush_cpumask) )
             cpu_relax();
@@ -280,7 +281,7 @@ int on_selected_cpus(
     spin_lock(&call_lock);
 
     call_data = &data;
-    wmb();
+    mb();
 
     send_IPI_mask(selected, CALL_FUNCTION_VECTOR);
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Patch] x86: enforce strict memory order for x2apic

[Jan Beulich]

>>> "Tian, Kevin" <kevin.tian@intel.com> 25.09.08 08:55 >>>
>We kick a fix by simply using 'mb' instead of 'wmb'.
>This can effectively enforce program order, as args
>for WRMSR are loaded from memory. Those memory loads
>are ordered by mb and so does WRMSR. Alternative is

The whole change looks a little dubious to me, perhaps largely as I can't
spot which WRMSR (loading its arguments from memory) you're talking
about.

>to add a 'fence' parameter to send_IPI_mask and then
>only use 'mb' in x2apic version. We think it's not
>necessary to go with that complexity.

On the other hand you're penalizing LAPIC systems with this, too. I'm
not certain that's better than fixing the (incorrect) assumptions just in
the x2APIC case.

Jan

RE: [Patch] x86: enforce strict memory order for x2apic

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

>From: Jan Beulich [mailto:jbeulich@novell.com]
>Sent: 2008年9月25日 16:25
>
>>>> "Tian, Kevin" <kevin.tian@intel.com> 25.09.08 08:55 >>>
>>We kick a fix by simply using 'mb' instead of 'wmb'.
>>This can effectively enforce program order, as args
>>for WRMSR are loaded from memory. Those memory loads
>>are ordered by mb and so does WRMSR. Alternative is
>
>The whole change looks a little dubious to me, perhaps largely
>as I can't
>spot which WRMSR (loading its arguments from memory) you're talking
>about.

WRMSR to ICR, where EDX/EAX is derived from cpu_online
_map and x86_cpu_to_apicid

>
>>to add a 'fence' parameter to send_IPI_mask and then
>>only use 'mb' in x2apic version. We think it's not
>>necessary to go with that complexity.
>
>On the other hand you're penalizing LAPIC systems with this, too. I'm
>not certain that's better than fixing the (incorrect)
>assumptions just in
>the x2APIC case.

I agree. I'll revise the patch to add a 'mb()' in x2APIC instead,
though that may bring a bit overhead to cases when fencing
is not required, like event check IPI.

Thanks,
Kevin

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Patch] x86: enforce strict memory order for x2apic

[Keir Fraser]

On 25/9/08 09:29, "Tian, Kevin" <kevin.tian@intel.com> wrote:

>> On the other hand you're penalizing LAPIC systems with this, too. I'm
>> not certain that's better than fixing the (incorrect)
>> assumptions just in
>> the x2APIC case.
> 
> I agree. I'll revise the patch to add a 'mb()' in x2APIC instead,
> though that may bring a bit overhead to cases when fencing
> is not required, like event check IPI.

To be precise, mb() at top of send_IPI_mask_x2apic(), and wmb() may as well
go from on_selected_cpus() as we can assume send_IPI_mask() is a barrier.

I'll do the patch for this myself.

 -- Keir

RE: [Patch] x86: enforce strict memory order for x2apic

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>Sent: 2008年9月25日 16:36
>
>On 25/9/08 09:29, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>
>>> On the other hand you're penalizing LAPIC systems with
>this, too. I'm
>>> not certain that's better than fixing the (incorrect)
>>> assumptions just in
>>> the x2APIC case.
>>
>> I agree. I'll revise the patch to add a 'mb()' in x2APIC instead,
>> though that may bring a bit overhead to cases when fencing
>> is not required, like event check IPI.
>
>To be precise, mb() at top of send_IPI_mask_x2apic(), and
>wmb() may as well
>go from on_selected_cpus() as we can assume send_IPI_mask() is
>a barrier.
>
>I'll do the patch for this myself.
>

Yes, and thanks. Also please add a wmb() in flush_area_mask,
though it's a nop but semanticly desired like for on_selected_cpus.

Thanks,
Kevin

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [Patch] x86: enforce strict memory order for x2apic

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

Please forget this one. I made mistake since we'll assume
send_IPI_mask as a barrier now. :-)

Thanks,
Kevin

>From: Tian, Kevin
>Sent: 2008年9月25日 16:39
>
>>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>>Sent: 2008年9月25日 16:36
>>
>>On 25/9/08 09:29, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>>
>>>> On the other hand you're penalizing LAPIC systems with
>>this, too. I'm
>>>> not certain that's better than fixing the (incorrect)
>>>> assumptions just in
>>>> the x2APIC case.
>>>
>>> I agree. I'll revise the patch to add a 'mb()' in x2APIC instead,
>>> though that may bring a bit overhead to cases when fencing
>>> is not required, like event check IPI.
>>
>>To be precise, mb() at top of send_IPI_mask_x2apic(), and
>>wmb() may as well
>>go from on_selected_cpus() as we can assume send_IPI_mask() is
>>a barrier.
>>
>>I'll do the patch for this myself.
>>
>
>Yes, and thanks. Also please add a wmb() in flush_area_mask,
>though it's a nop but semanticly desired like for on_selected_cpus.


[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Patch] x86: enforce strict memory order for x2apic

[Keir Fraser]

Yes. Now applied as c/s 18541 including explanation of why mb() inside
send_IPI_mask_x2apic() is sufficient (it's subtle!).

 -- Keir

On 25/9/08 09:40, "Tian, Kevin" <kevin.tian@intel.com> wrote:

> Please forget this one. I made mistake since we'll assume
> send_IPI_mask as a barrier now. :-)
> 
> Thanks,
> Kevin
> 
>> From: Tian, Kevin
>> Sent: 2008年9月25日 16:39
>> 
>>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>>> Sent: 2008年9月25日 16:36
>>> 
>>> On 25/9/08 09:29, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>>> 
>>>>> On the other hand you're penalizing LAPIC systems with
>>> this, too. I'm
>>>>> not certain that's better than fixing the (incorrect)
>>>>> assumptions just in
>>>>> the x2APIC case.
>>>> 
>>>> I agree. I'll revise the patch to add a 'mb()' in x2APIC instead,
>>>> though that may bring a bit overhead to cases when fencing
>>>> is not required, like event check IPI.
>>> 
>>> To be precise, mb() at top of send_IPI_mask_x2apic(), and
>>> wmb() may as well
>>> go from on_selected_cpus() as we can assume send_IPI_mask() is
>>> a barrier.
>>> 
>>> I'll do the patch for this myself.
>>> 
>> 
>> Yes, and thanks. Also please add a wmb() in flush_area_mask,
>> though it's a nop but semanticly desired like for on_selected_cpus.
>