Patchwork [uq/master,2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

login
register
mail settings
Submitter Marcelo Tosatti
Date Feb. 22, 2010, 1:59 p.m.
Message ID <20100222140210.130087300@amt.cnet>
Download mbox | patch
Permalink /patch/45963/
State New
Headers show

Comments

Marcelo Tosatti - Feb. 22, 2010, 1:59 p.m.
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Avi Kivity - Feb. 22, 2010, 2:23 p.m.
On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> Cc: "Michael S. Tsirkin"<mst@redhat.com>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>
> Index: qemu/kvm-all.c
> ===================================================================
> --- qemu.orig/kvm-all.c
> +++ qemu/kvm-all.c
> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>       return 1;
>   }
>
> +/* FIXME: arch dependant, x86 version */
> +#define smp_wmb()   asm volatile("" ::: "memory")
> +
>    

sfence?  what about other arches?
Michael S. Tsirkin - Feb. 22, 2010, 2:44 p.m.
On Mon, Feb 22, 2010 at 10:59:08AM -0300, Marcelo Tosatti wrote:
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

We'll need implementation for other arches, I'll dust off
my patch that adds it and repost, but for now this
is better than what we have.

> Index: qemu/kvm-all.c
> ===================================================================
> --- qemu.orig/kvm-all.c
> +++ qemu/kvm-all.c
> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port, 
>      return 1;
>  }
>  
> +/* FIXME: arch dependant, x86 version */
> +#define smp_wmb()   asm volatile("" ::: "memory")
> +
>  void kvm_flush_coalesced_mmio_buffer(void)
>  {
>  #ifdef KVM_CAP_COALESCED_MMIO
> @@ -730,7 +733,7 @@ void kvm_flush_coalesced_mmio_buffer(voi
>              ent = &ring->coalesced_mmio[ring->first];
>  
>              cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> -            /* FIXME smp_wmb() */
> +            smp_wmb();
>              ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>          }
>      }
>
Marcelo Tosatti - Feb. 22, 2010, 2:45 p.m.
On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> >Cc: "Michael S. Tsirkin"<mst@redhat.com>
> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >
> >Index: qemu/kvm-all.c
> >===================================================================
> >--- qemu.orig/kvm-all.c
> >+++ qemu/kvm-all.c
> >@@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
> >      return 1;
> >  }
> >
> >+/* FIXME: arch dependant, x86 version */
> >+#define smp_wmb()   asm volatile("" ::: "memory")
> >+
> 
> sfence?  

There is no need (for this case). Older read cannot be reordered with
write, writes are not reordered with other writes, writes by a single
processor are observed in the same order by all processors.

> what about other arches?

They need to be fixed? PPC needs an instruction apparently.

Is there any objection to including this patch?
Avi Kivity - Feb. 22, 2010, 2:57 p.m.
On 02/22/2010 04:45 PM, Marcelo Tosatti wrote:
> On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
>    
>> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
>>      
>>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>> Index: qemu/kvm-all.c
>>> ===================================================================
>>> --- qemu.orig/kvm-all.c
>>> +++ qemu/kvm-all.c
>>> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>>>       return 1;
>>>   }
>>>
>>> +/* FIXME: arch dependant, x86 version */
>>> +#define smp_wmb()   asm volatile("" ::: "memory")
>>> +
>>>        
>> sfence?
>>      
> There is no need (for this case). Older read cannot be reordered with
> write, writes are not reordered with other writes, writes by a single
> processor are observed in the same order by all processors.
>    

Well, Linux does use sfence.  Perhaps it's only needed for WC writes 
(movnti and friends), but better be careful here.

>> what about other arches?
>>      
> They need to be fixed? PPC needs an instruction apparently.
>
> Is there any objection to including this patch?
>    

I imagine all arches need an instruction.  For reads as well.

Note, gcc has a __sync_synchronize() builtin that compiles to mfence on 
x86.  We might use that as a baseline for both rmb and wmb, and let each 
arch override it incrementally.
Michael S. Tsirkin - Feb. 22, 2010, 2:57 p.m.
On Mon, Feb 22, 2010 at 04:57:29PM +0200, Avi Kivity wrote:
> On 02/22/2010 04:45 PM, Marcelo Tosatti wrote:
>> On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
>>    
>>> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
>>>      
>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>>
>>>> Index: qemu/kvm-all.c
>>>> ===================================================================
>>>> --- qemu.orig/kvm-all.c
>>>> +++ qemu/kvm-all.c
>>>> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>>>>       return 1;
>>>>   }
>>>>
>>>> +/* FIXME: arch dependant, x86 version */
>>>> +#define smp_wmb()   asm volatile("" ::: "memory")
>>>> +
>>>>        
>>> sfence?
>>>      
>> There is no need (for this case). Older read cannot be reordered with
>> write, writes are not reordered with other writes, writes by a single
>> processor are observed in the same order by all processors.
>>    
>
> Well, Linux does use sfence.

At least on 64 bit it doesnt.

>  Perhaps it's only needed for WC writes  
> (movnti and friends), but better be careful here.
>
>>> what about other arches?
>>>      
>> They need to be fixed? PPC needs an instruction apparently.
>>
>> Is there any objection to including this patch?
>>    
>
> I imagine all arches need an instruction.  For reads as well.
>
> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on  
> x86.  We might use that as a baseline for both rmb and wmb, and let each  
> arch override it incrementally.

This it what my patch did. Note it only works well for recent gcc.

> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Feb. 22, 2010, 3:08 p.m.
On 02/22/2010 04:57 PM, Michael S. Tsirkin wrote:
>
>    
>>> There is no need (for this case). Older read cannot be reordered with
>>> write, writes are not reordered with other writes, writes by a single
>>> processor are observed in the same order by all processors.
>>>
>>>        
>> Well, Linux does use sfence.
>>      
> At least on 64 bit it doesnt.
>    

Right, I was looking at wmb(), not smp_wmb().

>> I imagine all arches need an instruction.  For reads as well.
>>
>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>> arch override it incrementally.
>>      
> This it what my patch did. Note it only works well for recent gcc.
>    

Do you know how recent?
Michael S. Tsirkin - Feb. 22, 2010, 3:08 p.m.
On Mon, Feb 22, 2010 at 05:08:00PM +0200, Avi Kivity wrote:
> On 02/22/2010 04:57 PM, Michael S. Tsirkin wrote:
>>
>>    
>>>> There is no need (for this case). Older read cannot be reordered with
>>>> write, writes are not reordered with other writes, writes by a single
>>>> processor are observed in the same order by all processors.
>>>>
>>>>        
>>> Well, Linux does use sfence.
>>>      
>> At least on 64 bit it doesnt.
>>    
>
> Right, I was looking at wmb(), not smp_wmb().
>
>>> I imagine all arches need an instruction.  For reads as well.
>>>
>>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>>> arch override it incrementally.
>>>      
>> This it what my patch did. Note it only works well for recent gcc.
>>    
>
> Do you know how recent?

IIRC 4.3 has broken implementation.
4.4 seems OK as far as I can tell.

> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Feb. 22, 2010, 3:28 p.m.
On 02/22/2010 05:08 PM, Michael S. Tsirkin wrote:
>
>>
>>>> I imagine all arches need an instruction.  For reads as well.
>>>>
>>>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>>>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>>>> arch override it incrementally.
>>>>
>>>>          
>>> This it what my patch did. Note it only works well for recent gcc.
>>>
>>>        
>> Do you know how recent?
>>      
> IIRC 4.3 has broken implementation.
> 4.4 seems OK as far as I can tell.
>    

Well, so that idea's down.

Patch

Index: qemu/kvm-all.c
===================================================================
--- qemu.orig/kvm-all.c
+++ qemu/kvm-all.c
@@ -718,6 +718,9 @@  static int kvm_handle_io(uint16_t port, 
     return 1;
 }
 
+/* FIXME: arch dependant, x86 version */
+#define smp_wmb()   asm volatile("" ::: "memory")
+
 void kvm_flush_coalesced_mmio_buffer(void)
 {
 #ifdef KVM_CAP_COALESCED_MMIO
@@ -730,7 +733,7 @@  void kvm_flush_coalesced_mmio_buffer(voi
             ent = &ring->coalesced_mmio[ring->first];
 
             cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
-            /* FIXME smp_wmb() */
+            smp_wmb();
             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
         }
     }