[Qemu-devel] [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Qemu-devel] [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Michael S. Tsirkin]

The following fixes a class of long-standing bugs in qemu:
when kvm is enabled, guest might access device structures
in memory while they are updated by qemu on another CPU.
In this scenario, memory barriers are necessary to prevent
host CPU from reordering memory accesses, which might confuse
the guest.

This patch only fixes virtio, but other emulated devices
might have a similar bug. They'll need to be discovered
and addressed case by case.

This is still under test ... meanwhile: any early feedback/flames?

-- 
MST


Michael S. Tsirkin (3):
  qemu: add barriers.h header
  virtio: use a real wmb
  virtio: add missing barriers

 hw/barriers.h |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio.c   |   18 ++++----
 2 files changed, 139 insertions(+), 10 deletions(-)
 create mode 100644 hw/barriers.h

[Qemu-devel] Re: [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Michael S. Tsirkin]

On Tue, Dec 08, 2009 at 06:18:18PM +0200, Michael S. Tsirkin wrote:
> The following fixes a class of long-standing bugs in qemu:
> when kvm is enabled, guest might access device structures
> in memory while they are updated by qemu on another CPU.
> In this scenario, memory barriers are necessary to prevent
> host CPU from reordering memory accesses, which might confuse
> the guest.
> 
> This patch only fixes virtio, but other emulated devices
> might have a similar bug. They'll need to be discovered
> and addressed case by case.
> 
> This is still under test ... meanwhile: any early feedback/flames?
> 

Any comments on this one?
The patch works fine in my testing, and even though
it did not fix a crash that I hoped it will fix,
it seems required for correctness... Right?
 
> Michael S. Tsirkin (3):
>   qemu: add barriers.h header
>   virtio: use a real wmb
>   virtio: add missing barriers
> 
>  hw/barriers.h |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio.c   |   18 ++++----
>  2 files changed, 139 insertions(+), 10 deletions(-)
>  create mode 100644 hw/barriers.h
> 

[Qemu-devel] Re: [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Anthony Liguori]

On 12/22/2009 05:26 AM, Michael S. Tsirkin wrote:
> On Tue, Dec 08, 2009 at 06:18:18PM +0200, Michael S. Tsirkin wrote:
>    
>> The following fixes a class of long-standing bugs in qemu:
>> when kvm is enabled, guest might access device structures
>> in memory while they are updated by qemu on another CPU.
>> In this scenario, memory barriers are necessary to prevent
>> host CPU from reordering memory accesses, which might confuse
>> the guest.
>>
>> This patch only fixes virtio, but other emulated devices
>> might have a similar bug. They'll need to be discovered
>> and addressed case by case.
>>
>> This is still under test ... meanwhile: any early feedback/flames?
>>
>>      
> Any comments on this one?
> The patch works fine in my testing, and even though
> it did not fix a crash that I hoped it will fix,
> it seems required for correctness... Right?
>    

It's definitely better than what we have.  Rusty mentioned something to 
me a bit ago about the barriers for virtio in the kernel needing some 
work.  I've been meaning to ask him about it in the context of this patch.

Rusty, am I remembering correctly or have I been sipping too much 
eggnog? :-)

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Paul Brook]

On Tuesday 22 December 2009, Anthony Liguori wrote:
> On 12/22/2009 05:26 AM, Michael S. Tsirkin wrote:
> > On Tue, Dec 08, 2009 at 06:18:18PM +0200, Michael S. Tsirkin wrote:
> >> The following fixes a class of long-standing bugs in qemu:
> >> when kvm is enabled, guest might access device structures
> >> in memory while they are updated by qemu on another CPU.
> >> In this scenario, memory barriers are necessary to prevent
> >> host CPU from reordering memory accesses, which might confuse
> >> the guest.
> >>
> >> This patch only fixes virtio, but other emulated devices
> >> might have a similar bug. They'll need to be discovered
> >> and addressed case by case.

Real devices generally aren't cache coherent, so I'd expect problems to be 
rare. I guess theoretically you may need barriers around the MMIO/IO port 
handlers, though in practice the KVM context switch probably provides this 
anyway.

> >> This is still under test ... meanwhile: any early feedback/flames?
> >
> > Any comments on this one?
> > The patch works fine in my testing, and even though
> > it did not fix a crash that I hoped it will fix,
> > it seems required for correctness... Right?
> 
> It's definitely better than what we have.  Rusty mentioned something to
> me a bit ago about the barriers for virtio in the kernel needing some
> work.  I've been meaning to ask him about it in the context of this patch.

Given this is supposed to be portable code, I wonder if we should have atomic 
ordered memory accessors instead.

Paul

[Qemu-devel] Re: [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Michael S. Tsirkin]

On Tue, Dec 22, 2009 at 04:25:32PM +0000, Paul Brook wrote:
> On Tuesday 22 December 2009, Anthony Liguori wrote:
> > On 12/22/2009 05:26 AM, Michael S. Tsirkin wrote:
> > > On Tue, Dec 08, 2009 at 06:18:18PM +0200, Michael S. Tsirkin wrote:
> > >> The following fixes a class of long-standing bugs in qemu:
> > >> when kvm is enabled, guest might access device structures
> > >> in memory while they are updated by qemu on another CPU.
> > >> In this scenario, memory barriers are necessary to prevent
> > >> host CPU from reordering memory accesses, which might confuse
> > >> the guest.
> > >>
> > >> This patch only fixes virtio, but other emulated devices
> > >> might have a similar bug. They'll need to be discovered
> > >> and addressed case by case.
> 
> Real devices generally aren't cache coherent, so I'd expect problems to be 
> rare. I guess theoretically you may need barriers around the MMIO/IO port 
> handlers, though in practice the KVM context switch probably provides this 
> anyway.
> 
> > >> This is still under test ... meanwhile: any early feedback/flames?
> > >
> > > Any comments on this one?
> > > The patch works fine in my testing, and even though
> > > it did not fix a crash that I hoped it will fix,
> > > it seems required for correctness... Right?
> > 
> > It's definitely better than what we have.  Rusty mentioned something to
> > me a bit ago about the barriers for virtio in the kernel needing some
> > work.  I've been meaning to ask him about it in the context of this patch.
> 
> Given this is supposed to be portable code, I wonder if we should have atomic 
> ordered memory accessors instead.
> 
> Paul

Could you clarify please?

The infiniband bits I used as base are very portable,
I know they build on a ton of platforms. I just stripped
a couple of infiniband specific assumptions from there.

Do you suggest we use __sync_synchronize?
Unfortunately this is broken or slow on many platforms.
I do use it when it seems safe or when we see a platform
we don't know about.

-- 
MST

Re: [Qemu-devel] Re: [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Avi Kivity]

On 12/22/2009 06:25 PM, Paul Brook wrote:
> On Tuesday 22 December 2009, Anthony Liguori wrote:
>    
>> On 12/22/2009 05:26 AM, Michael S. Tsirkin wrote:
>>      
>>> On Tue, Dec 08, 2009 at 06:18:18PM +0200, Michael S. Tsirkin wrote:
>>>        
>>>> The following fixes a class of long-standing bugs in qemu:
>>>> when kvm is enabled, guest might access device structures
>>>> in memory while they are updated by qemu on another CPU.
>>>> In this scenario, memory barriers are necessary to prevent
>>>> host CPU from reordering memory accesses, which might confuse
>>>> the guest.
>>>>
>>>> This patch only fixes virtio, but other emulated devices
>>>> might have a similar bug. They'll need to be discovered
>>>> and addressed case by case.
>>>>          
> Real devices generally aren't cache coherent, so I'd expect problems to be
> rare. I guess theoretically you may need barriers around the MMIO/IO port
> handlers, though in practice the KVM context switch probably provides this
> anyway.
>    

We're not guaranteed to have a context switch.  One thread can update 
the ring while another consumes it.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Paul Brook]

> > Given this is supposed to be portable code, I wonder if we should have
> > atomic ordered memory accessors instead.
> >
> > Paul
> 
> Could you clarify please?
> 
> The infiniband bits I used as base are very portable,
> I know they build on a ton of platforms. I just stripped
> a couple of infiniband specific assumptions from there.
> 
> Do you suggest we use __sync_synchronize?
> Unfortunately this is broken or slow on many platforms.
> I do use it when it seems safe or when we see a platform
> we don't know about.

I mean have a single function that does both the atomic load/store and the 
memory barrier. Instead of:

  stw_phys(addr, val)
  barrier();

We do:

  stw_phys_barrier(addr, val).

This avoids issues in the future (multithreaded TCG) where atomic memory 
accesses may be nontrivial.

Paul

[Qemu-devel] Re: [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Rusty Russell]

On Thu, 24 Dec 2009 03:06:00 am Michael S. Tsirkin wrote:
> On Wed, Dec 23, 2009 at 05:04:19PM +1030, Rusty Russell wrote:
> > It's possible, but I don't know of any missing cases.  Certainly *lguest* i
> > is missing barriers, since it's UP, but the core virtio should have them.
> 
> Something that Paul Brook pointed out, is that
> using a 16 bit value in C like we do in guest, e.g. with
> ring->avail.idx
> might in theory result in two single byte reads.
> 
> If this happens, guest will see a wrong index value.

In the Linux kernel we make atomicity assumptions about fundamental types.
(Specifically pointers).

QEMU may not want to rely on such assumptions however, and make them explicit.
I have sympathy with Paul here.

Cheers,
Rusty.