Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Thu, 20 Oct 2016 18:53:31 +0200
Paolo Bonzini  wrote:

> No, it's because virtio-mmio can be created without a device 
> underneath.  virtio_bus_start_ioeventfd in that case is wrong, but 
> virtio_bus_stop_ioeventfd should be a no-op.  The fix is trivial:

I tend to forget this virtio-mmio speciality...

> 
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 0479704..bf61f66 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -172,12 +172,15 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
> 
>  void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
>  {
> -VirtIODevice *vdev = virtio_bus_get_device(bus);
> -VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> +VirtIODevice *vdev;
> +VirtioDeviceClass *vdc;
> 
>  if (!bus->ioeventfd_started) {
>  return;
>  }
> +
> +vdev = virtio_bus_get_device(bus);
> +vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>  vdc->stop_ioeventfd(vdev);
>  bus->ioeventfd_started = false;
>  }

Looks sane, and make check passes with this applied.

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

On 20/10/2016 11:03, Cornelia Huck wrote:
> On Wed, 19 Oct 2016 22:44:18 +0200
> Paolo Bonzini  wrote:
> 
>> On 19/10/2016 17:38, Cornelia Huck wrote:
>>> - failures are in qom-test for aarch64:
>>> TEST: tests/qom-test... (pid=23997)
>>>   /aarch64/qom/integratorcp:   OK
>>>   /aarch64/qom/nuri:   OK
>>>   /aarch64/qom/verdex: OK
>>>   /aarch64/qom/ast2500-evb:OK
>>>   /aarch64/qom/smdkc210:   OK
>>>   /aarch64/qom/collie: OK
>>>   /aarch64/qom/imx25-pdk:  OK
>>>   /aarch64/qom/none:   OK
>>>   /aarch64/qom/spitz:  OK
>>>   /aarch64/qom/realview-pbx-a9:OK
>>>   /aarch64/qom/realview-eb:OK
>>>   /aarch64/qom/versatilepb:OK
>>>   /aarch64/qom/realview-pb-a8: OK
>>>   /aarch64/qom/musicpal:   OK
>>>   /aarch64/qom/z2: OK
>>>   /aarch64/qom/akita:  OK
>>>   /aarch64/qom/virt-2.7:   
>>> Broken pipe
>>> FAIL
>>> GTester: last random seed: R02Saec62eb6f9ebd3e5bfcbf42d0aaf165a
>>> (pid=24053)
>>>   /aarch64/qom/kzm:OK
>>>   /aarch64/qom/virt-2.8:   
>>> Broken pipe
>>> FAIL
>>> GTester: last random seed: R02S3472a1653451d1812262f7d72624492e
>>> (pid=24063)
>>>   /aarch64/qom/realview-eb-mpcore: OK
>>>   /aarch64/qom/sx1:OK
>>>   /aarch64/qom/sx1-v1: OK
>>>   /aarch64/qom/virt-2.6:   
>>> Broken pipe
>>> FAIL
>>> GTester: last random seed: R02Se4b753c5be66c0ef7870bebcca8771f8
>>> (pid=24098)
>>>   /aarch64/qom/cubieboard: OK
>>>   /aarch64/qom/highbank:   OK
>>>   /aarch64/qom/raspi2: OK
>>>   /aarch64/qom/netduino2:  OK
>>>   /aarch64/qom/terrier:OK
>>>   /aarch64/qom/n810:   OK
>>>   /aarch64/qom/mainstone:  OK
>>>   /aarch64/qom/palmetto-bmc:   OK
>>>   /aarch64/qom/sabrelite:  OK
>>>   /aarch64/qom/midway: OK
>>>   /aarch64/qom/cheetah:OK
>>>   /aarch64/qom/tosa:   OK
>>>   /aarch64/qom/borzoi: OK
>>>   /aarch64/qom/versatileab:OK
>>>   /aarch64/qom/lm3s6965evb:OK
>>>   /aarch64/qom/n800:   OK
>>>   /aarch64/qom/connex: OK
>>>   /aarch64/qom/xilinx-zynq-a9: OK
>>>   /aarch64/qom/xlnx-ep108: OK
>>>   /aarch64/qom/vexpress-a9:
>>> Broken pipe
>>> FAIL
>>> GTester: last random seed: R02Sdf4aceaaef3ceb060fd5996ecfd05bbb
>>> (pid=24180)
>>>   /aarch64/qom/vexpress-a15:   
>>> Broken pipe
>>> FAIL
>>> GTester: last random seed: R02Sdf4de27065ea3baf0b2acc109af636b8
>>> (pid=24187)
>>>   /aarch64/qom/xlnx-zcu102:OK
>>>   /aarch64/qom/canon-a1100:OK
>>>   /aarch64/qom/lm3s811evb: OK
>>> FAIL: tests/qom-test
>>>
>>> Do these boards maybe have something interesting in common?
>>>
>>> No further time to look into this today, sorry.
>>
>> They use virtio-mmio, probably.  I'll look at it tomorrow.
>>
>> The s390 spew is an abort(), isn't it?
>> Paolo
>>
> 
> virtio-mmio looks like the best candidate (maybe the eventfd is not
> stopped properly and writes happen where they are not supposed to
> anymore?) But staring at the patch didn't make anything jump out at
> me...

No, it's because virtio-mmio can be created 

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Wed, 19 Oct 2016 22:44:18 +0200
Paolo Bonzini  wrote:

> On 19/10/2016 17:38, Cornelia Huck wrote:
> > - failures are in qom-test for aarch64:
> > TEST: tests/qom-test... (pid=23997)
> >   /aarch64/qom/integratorcp:   OK
> >   /aarch64/qom/nuri:   OK
> >   /aarch64/qom/verdex: OK
> >   /aarch64/qom/ast2500-evb:OK
> >   /aarch64/qom/smdkc210:   OK
> >   /aarch64/qom/collie: OK
> >   /aarch64/qom/imx25-pdk:  OK
> >   /aarch64/qom/none:   OK
> >   /aarch64/qom/spitz:  OK
> >   /aarch64/qom/realview-pbx-a9:OK
> >   /aarch64/qom/realview-eb:OK
> >   /aarch64/qom/versatilepb:OK
> >   /aarch64/qom/realview-pb-a8: OK
> >   /aarch64/qom/musicpal:   OK
> >   /aarch64/qom/z2: OK
> >   /aarch64/qom/akita:  OK
> >   /aarch64/qom/virt-2.7:   
> > Broken pipe
> > FAIL
> > GTester: last random seed: R02Saec62eb6f9ebd3e5bfcbf42d0aaf165a
> > (pid=24053)
> >   /aarch64/qom/kzm:OK
> >   /aarch64/qom/virt-2.8:   
> > Broken pipe
> > FAIL
> > GTester: last random seed: R02S3472a1653451d1812262f7d72624492e
> > (pid=24063)
> >   /aarch64/qom/realview-eb-mpcore: OK
> >   /aarch64/qom/sx1:OK
> >   /aarch64/qom/sx1-v1: OK
> >   /aarch64/qom/virt-2.6:   
> > Broken pipe
> > FAIL
> > GTester: last random seed: R02Se4b753c5be66c0ef7870bebcca8771f8
> > (pid=24098)
> >   /aarch64/qom/cubieboard: OK
> >   /aarch64/qom/highbank:   OK
> >   /aarch64/qom/raspi2: OK
> >   /aarch64/qom/netduino2:  OK
> >   /aarch64/qom/terrier:OK
> >   /aarch64/qom/n810:   OK
> >   /aarch64/qom/mainstone:  OK
> >   /aarch64/qom/palmetto-bmc:   OK
> >   /aarch64/qom/sabrelite:  OK
> >   /aarch64/qom/midway: OK
> >   /aarch64/qom/cheetah:OK
> >   /aarch64/qom/tosa:   OK
> >   /aarch64/qom/borzoi: OK
> >   /aarch64/qom/versatileab:OK
> >   /aarch64/qom/lm3s6965evb:OK
> >   /aarch64/qom/n800:   OK
> >   /aarch64/qom/connex: OK
> >   /aarch64/qom/xilinx-zynq-a9: OK
> >   /aarch64/qom/xlnx-ep108: OK
> >   /aarch64/qom/vexpress-a9:
> > Broken pipe
> > FAIL
> > GTester: last random seed: R02Sdf4aceaaef3ceb060fd5996ecfd05bbb
> > (pid=24180)
> >   /aarch64/qom/vexpress-a15:   
> > Broken pipe
> > FAIL
> > GTester: last random seed: R02Sdf4de27065ea3baf0b2acc109af636b8
> > (pid=24187)
> >   /aarch64/qom/xlnx-zcu102:OK
> >   /aarch64/qom/canon-a1100:OK
> >   /aarch64/qom/lm3s811evb: OK
> > FAIL: tests/qom-test
> > 
> > Do these boards maybe have something interesting in common?
> > 
> > No further time to look into this today, sorry.
> 
> They use virtio-mmio, probably.  I'll look at it tomorrow.
> 
> The s390 spew is an abort(), isn't it?
> Paolo
> 

virtio-mmio looks like the best candidate (maybe the eventfd is not
stopped properly and writes happen where they are not supposed to
anymore?) But staring at the patch didn't make anything jump out at
me...

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

On 19/10/2016 17:38, Cornelia Huck wrote:
> - failures are in qom-test for aarch64:
> TEST: tests/qom-test... (pid=23997)
>   /aarch64/qom/integratorcp:   OK
>   /aarch64/qom/nuri:   OK
>   /aarch64/qom/verdex: OK
>   /aarch64/qom/ast2500-evb:OK
>   /aarch64/qom/smdkc210:   OK
>   /aarch64/qom/collie: OK
>   /aarch64/qom/imx25-pdk:  OK
>   /aarch64/qom/none:   OK
>   /aarch64/qom/spitz:  OK
>   /aarch64/qom/realview-pbx-a9:OK
>   /aarch64/qom/realview-eb:OK
>   /aarch64/qom/versatilepb:OK
>   /aarch64/qom/realview-pb-a8: OK
>   /aarch64/qom/musicpal:   OK
>   /aarch64/qom/z2: OK
>   /aarch64/qom/akita:  OK
>   /aarch64/qom/virt-2.7:   Broken 
> pipe
> FAIL
> GTester: last random seed: R02Saec62eb6f9ebd3e5bfcbf42d0aaf165a
> (pid=24053)
>   /aarch64/qom/kzm:OK
>   /aarch64/qom/virt-2.8:   Broken 
> pipe
> FAIL
> GTester: last random seed: R02S3472a1653451d1812262f7d72624492e
> (pid=24063)
>   /aarch64/qom/realview-eb-mpcore: OK
>   /aarch64/qom/sx1:OK
>   /aarch64/qom/sx1-v1: OK
>   /aarch64/qom/virt-2.6:   Broken 
> pipe
> FAIL
> GTester: last random seed: R02Se4b753c5be66c0ef7870bebcca8771f8
> (pid=24098)
>   /aarch64/qom/cubieboard: OK
>   /aarch64/qom/highbank:   OK
>   /aarch64/qom/raspi2: OK
>   /aarch64/qom/netduino2:  OK
>   /aarch64/qom/terrier:OK
>   /aarch64/qom/n810:   OK
>   /aarch64/qom/mainstone:  OK
>   /aarch64/qom/palmetto-bmc:   OK
>   /aarch64/qom/sabrelite:  OK
>   /aarch64/qom/midway: OK
>   /aarch64/qom/cheetah:OK
>   /aarch64/qom/tosa:   OK
>   /aarch64/qom/borzoi: OK
>   /aarch64/qom/versatileab:OK
>   /aarch64/qom/lm3s6965evb:OK
>   /aarch64/qom/n800:   OK
>   /aarch64/qom/connex: OK
>   /aarch64/qom/xilinx-zynq-a9: OK
>   /aarch64/qom/xlnx-ep108: OK
>   /aarch64/qom/vexpress-a9:Broken 
> pipe
> FAIL
> GTester: last random seed: R02Sdf4aceaaef3ceb060fd5996ecfd05bbb
> (pid=24180)
>   /aarch64/qom/vexpress-a15:   Broken 
> pipe
> FAIL
> GTester: last random seed: R02Sdf4de27065ea3baf0b2acc109af636b8
> (pid=24187)
>   /aarch64/qom/xlnx-zcu102:OK
>   /aarch64/qom/canon-a1100:OK
>   /aarch64/qom/lm3s811evb: OK
> FAIL: tests/qom-test
> 
> Do these boards maybe have something interesting in common?
> 
> No further time to look into this today, sorry.

They use virtio-mmio, probably.  I'll look at it tomorrow.

The s390 spew is an abort(), isn't it?
Paolo

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Wed, 19 Oct 2016 14:17:59 +0200
Cornelia Huck  wrote:

> On Mon, 10 Oct 2016 13:53:28 +0200
> Paolo Bonzini  wrote:
> 
> > This series started as an attempt to always use the dataplane path
> > for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> > was three-fold:
> > 
> > 1) to add more coverage for dataplane
> > 
> > 2) to remove virtio_add_queue_aio
> > 
> > 3) to simplify the dataplane start/stop code
> > 
> > It achieves the first two objectives, and while it doesn't quite
> > achieve the third it does cleanup the generic ioeventfd code in
> > virtio-bus more than I expected.  In particular, it reduces the set
> > of callbacks that transports must implement, and it removes the ugly
> > case where ioeventfd is started with generic callbacks and then moved
> > to the dataplane callbacks.  It also enables some simplification of the
> > functions that deal with host notifiers, and detects some configuration
> > errors better.
> > 
> > I've tested it with virtio-blk, virtio-scsi and vhost-net.
> 
> Hm. 'make check' on a s390 host faults in check-qtest-aarch64 (your
> branch rebased to current master; master itself is fine). I'll see if I
> can find out more (probably later today).

I've bisected this to patch 4 ("virtio: add start_ioeventfd and
stop_ioeventfd to VirtioDeviceClass").

More details of the failure:
- host is s390x with Fedora 23
- # Configured with: '../configure' '--target-list=s390x-softmmu 
s390x-linux-user aarch64-softmmu' '--enable-kvm' '--enable-vhost-net' 
'--enable-linux-aio'
- dmesg has:
[ 1774.006703] User process fault: interruption code 0010 ilc:3 in 
qemu-system-aarch64[1000+7b9000]
[ 1774.006720] Failing address:  TEID: 0800
[ 1774.006723] Fault in primary space mode while using user ASCE.
[ 1774.006731] AS:373ac1c7 R3:48148007 S:0020 
[ 1774.006739] CPU: 1 PID: 24183 Comm: qemu-system-aar Not tainted 
4.8.0-20161019.0.eb97ed0.8b99dbe.fc23.s390xdefault #1
[ 1774.006742] Hardware name: IBM  2827 H43  738
  (LPAR)
[ 1774.006745] task: 4ccb8008 task.stack: 355e
[ 1774.006748] User PSW : 070500018000 10332380
[ 1774.006753]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:1 AS:0 CC:0 PM:0 
RI:0 EA:3
[ 1774.006757] User GPRS: 0003   
0001
[ 1774.006761]102e1740 0178 1069b2ac 
101f8278
[ 1774.006764] 47471ee0  
101f6600
[ 1774.006766] 4795f788 102e14a0 
03ffe307e988
[ 1774.006774] User Code: 1033237a: 07febcr 15,%r14
  1033237c: 0707bcr 0,%r7
 #1033237e: 0707bcr 0,%r7
 >10332380: e3202004lg  
%r2,0(%r2)
  10332386: 07febcr 15,%r14
  10332388: e3102004lg  
%r1,0(%r2)
  1033238e: e32010500090llgc
%r2,80(%r1)
  10332394: 07febcr 15,%r14
[ 1774.006812] Last Breaking-Event-Address:
[ 1774.006816]  [<102e149a>] 0x102e149a

(once for each failure)
- failures are in qom-test for aarch64:
TEST: tests/qom-test... (pid=23997)
  /aarch64/qom/integratorcp:   OK
  /aarch64/qom/nuri:   OK
  /aarch64/qom/verdex: OK
  /aarch64/qom/ast2500-evb:OK
  /aarch64/qom/smdkc210:   OK
  /aarch64/qom/collie: OK
  /aarch64/qom/imx25-pdk:  OK
  /aarch64/qom/none:   OK
  /aarch64/qom/spitz:  OK
  /aarch64/qom/realview-pbx-a9:OK
  /aarch64/qom/realview-eb:OK
  /aarch64/qom/versatilepb:OK
  /aarch64/qom/realview-pb-a8: OK
  /aarch64/qom/musicpal:   OK
  /aarch64/qom/z2: OK
  /aarch64/qom/akita:  OK
  /aarch64/qom/virt-2.7:   Broken 
pipe
FAIL
GTester: last random seed: R02Saec62eb6f9ebd3e5bfcbf42d0aaf165a
(pid=24053)
  /aarch64/qom/kzm:OK
  

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:28 +0200
Paolo Bonzini  wrote:

> This series started as an attempt to always use the dataplane path
> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> was three-fold:
> 
> 1) to add more coverage for dataplane
> 
> 2) to remove virtio_add_queue_aio
> 
> 3) to simplify the dataplane start/stop code
> 
> It achieves the first two objectives, and while it doesn't quite
> achieve the third it does cleanup the generic ioeventfd code in
> virtio-bus more than I expected.  In particular, it reduces the set
> of callbacks that transports must implement, and it removes the ugly
> case where ioeventfd is started with generic callbacks and then moved
> to the dataplane callbacks.  It also enables some simplification of the
> functions that deal with host notifiers, and detects some configuration
> errors better.
> 
> I've tested it with virtio-blk, virtio-scsi and vhost-net.

Hm. 'make check' on a s390 host faults in check-qtest-aarch64 (your
branch rebased to current master; master itself is fine). I'll see if I
can find out more (probably later today).

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

- Original Message -
> From: "Cornelia Huck" 
> To: "Paolo Bonzini" 
> Cc: qemu-devel@nongnu.org, stefa...@redhat.com, borntrae...@de.ibm.com, 
> f...@redhat.com, m...@redhat.com
> Sent: Tuesday, October 18, 2016 7:24:45 PM
> Subject: Re: [PATCH 00/12] virtio: cleanup ioeventfd start/stop
> 
> On Mon, 10 Oct 2016 13:53:28 +0200
> Paolo Bonzini  wrote:
> 
> > This series started as an attempt to always use the dataplane path
> > for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> > was three-fold:
> > 
> > 1) to add more coverage for dataplane
> > 
> > 2) to remove virtio_add_queue_aio
> > 
> > 3) to simplify the dataplane start/stop code
> > 
> > It achieves the first two objectives, and while it doesn't quite
> > achieve the third it does cleanup the generic ioeventfd code in
> > virtio-bus more than I expected.  In particular, it reduces the set
> > of callbacks that transports must implement, and it removes the ugly
> > case where ioeventfd is started with generic callbacks and then moved
> > to the dataplane callbacks.  It also enables some simplification of the
> > functions that deal with host notifiers, and detects some configuration
> > errors better.
> > 
> > I've tested it with virtio-blk, virtio-scsi and vhost-net.
> 
> Do you have a branch somewhere?

ioeventfd-virtio at git://github.com/bonzini/qemu.git

Paolo

> > 
> > Patch 1 is a bugfix that I found while testing the TCG+dataplane combo.
> > 
> > Patches 2 and 3 are simplifications that are too nice to leave
> > them for later in the series.
> > 
> > Patch 4 moves some of the ioeventfd code from virtio-bus.c to
> > virtio.c.  At this point the transition is a bit half-assed, but
> > this changes as soon as we remove the generic->dataplane
> > handler transition.
> > 
> > Patches 5 to 7 do exactly that, and then the spring cleaning
> > begins, lasting for the whole second half of the series.
> 
> I'll continue to look at the patches from 4 on tomorrow.
> 
> 

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:28 +0200
Paolo Bonzini  wrote:

> This series started as an attempt to always use the dataplane path
> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> was three-fold:
> 
> 1) to add more coverage for dataplane
> 
> 2) to remove virtio_add_queue_aio
> 
> 3) to simplify the dataplane start/stop code
> 
> It achieves the first two objectives, and while it doesn't quite
> achieve the third it does cleanup the generic ioeventfd code in
> virtio-bus more than I expected.  In particular, it reduces the set
> of callbacks that transports must implement, and it removes the ugly
> case where ioeventfd is started with generic callbacks and then moved
> to the dataplane callbacks.  It also enables some simplification of the
> functions that deal with host notifiers, and detects some configuration
> errors better.
> 
> I've tested it with virtio-blk, virtio-scsi and vhost-net.

Do you have a branch somewhere?

> 
> Patch 1 is a bugfix that I found while testing the TCG+dataplane combo.
> 
> Patches 2 and 3 are simplifications that are too nice to leave
> them for later in the series.
> 
> Patch 4 moves some of the ioeventfd code from virtio-bus.c to
> virtio.c.  At this point the transition is a bit half-assed, but
> this changes as soon as we remove the generic->dataplane
> handler transition.
> 
> Patches 5 to 7 do exactly that, and then the spring cleaning
> begins, lasting for the whole second half of the series.

I'll continue to look at the patches from 4 on tomorrow.

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

On 10/10/2016 00:01, Michael S. Tsirkin wrote:
> On Wed, Sep 21, 2016 at 03:18:47PM +0200, Paolo Bonzini wrote:
>> This series started as an attempt to always use the dataplane path
>> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
>> was three-fold:
>>
>> 1) to add more coverage for dataplane
>>
>> 2) to remove virtio_add_queue_aio
>>
>> 3) to simplify the dataplane start/stop code
>>
>> It achieves the first two objectives, and while it doesn't quite
>> achieve the third it does cleanup the generic ioeventfd code in
>> virtio-bus more than I expected.  In particular, it reduces the set
>> of callbacks that transports must implement, and it removes the ugly
>> case where ioeventfd is started with generic callbacks and then moved
>> to the dataplane callbacks.  It also enables some simplification of the
>> functions that deal with host notifiers.
>>
>> I've tested it with virtio-blk, virtio-scsi and vhost-net.
>>
>> Patches 1 and 2 are simplifications that are too nice to leave
>> them for later in the series.
>>
>> Patch 3 moves some of the ioeventfd code from virtio-bus.c to
>> virtio.c.  At this point the transition is a bit half-assed, but
>> this changes as soon as we remove the generic->dataplane
>> handler transition.
>>
>> Patches 4 to 6 do exactly that, and then the spring cleaning
>> begins, lasting for the whole second half of the series.
>>
>> Opinions, reviews and bug reports?
>>
>> Thanks,
>>
>> Paolo
> 
> OK, this looks good to me. Can you pls rebase and repost
> so I can merge (there was also a bug report I think)?

Sure, I was going to do that today.  The bug is just too strict an
assertion, I'll fix it on repost.

Paolo

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Michael S. Tsirkin]

On Wed, Sep 21, 2016 at 03:18:47PM +0200, Paolo Bonzini wrote:
> This series started as an attempt to always use the dataplane path
> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> was three-fold:
> 
> 1) to add more coverage for dataplane
> 
> 2) to remove virtio_add_queue_aio
> 
> 3) to simplify the dataplane start/stop code
> 
> It achieves the first two objectives, and while it doesn't quite
> achieve the third it does cleanup the generic ioeventfd code in
> virtio-bus more than I expected.  In particular, it reduces the set
> of callbacks that transports must implement, and it removes the ugly
> case where ioeventfd is started with generic callbacks and then moved
> to the dataplane callbacks.  It also enables some simplification of the
> functions that deal with host notifiers.
> 
> I've tested it with virtio-blk, virtio-scsi and vhost-net.
> 
> Patches 1 and 2 are simplifications that are too nice to leave
> them for later in the series.
> 
> Patch 3 moves some of the ioeventfd code from virtio-bus.c to
> virtio.c.  At this point the transition is a bit half-assed, but
> this changes as soon as we remove the generic->dataplane
> handler transition.
> 
> Patches 4 to 6 do exactly that, and then the spring cleaning
> begins, lasting for the whole second half of the series.
> 
> Opinions, reviews and bug reports?
> 
> Thanks,
> 
> Paolo

OK, this looks good to me. Can you pls rebase and repost
so I can merge (there was also a bug report I think)?


> Paolo Bonzini (12):
>   virtio: move ioeventfd_disabled flag to VirtioBusState
>   virtio: move ioeventfd_started flag to VirtioBusState
>   virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass
>   virtio: introduce virtio_device_ioeventfd_enabled
>   virtio-blk: always use dataplane path if ioeventfd is active
>   virtio-scsi: always use dataplane path if ioeventfd is active
>   Revert "virtio: Introduce virtio_add_queue_aio"
>   virtio: remove set_handler argument from set_host_notifier_internal
>   virtio: remove ioeventfd_disabled altogether
>   virtio: do not export set_host_notifier_internal
>   virtio: inline virtio_queue_set_host_notifier_fd_handler
>   virtio: inline set_host_notifier_internal
> 
>  hw/block/dataplane/virtio-blk.c |  67 +++---
>  hw/block/dataplane/virtio-blk.h |   6 +-
>  hw/block/virtio-blk.c   |  16 ++---
>  hw/s390x/virtio-ccw.c   |  36 +-
>  hw/s390x/virtio-ccw.h   |   2 -
>  hw/scsi/virtio-scsi-dataplane.c |  51 --
>  hw/scsi/virtio-scsi.c   |  24 +++
>  hw/virtio/vhost.c   |   5 +-
>  hw/virtio/virtio-bus.c  | 153 
> +++-
>  hw/virtio/virtio-mmio.c |  35 +
>  hw/virtio/virtio-pci.c  |  32 +
>  hw/virtio/virtio-pci.h  |   2 -
>  hw/virtio/virtio.c  | 139 +++-
>  include/hw/virtio/virtio-bus.h  |  27 ---
>  include/hw/virtio/virtio-scsi.h |   6 +-
>  include/hw/virtio/virtio.h  |  11 +--
>  16 files changed, 272 insertions(+), 340 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Stefan Hajnoczi]

On Wed, Sep 21, 2016 at 03:18:47PM +0200, Paolo Bonzini wrote:
> Opinions, reviews and bug reports?

Looks good in general but I've skipped the tricky parts.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Christian Borntraeger]

On 09/27/2016 06:44 PM, Paolo Bonzini wrote:
> 
> 
> On 27/09/2016 16:45, Christian Borntraeger wrote:
>> On 09/21/2016 06:43 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 21/09/2016 16:01, Christian Borntraeger wrote:
 On 09/21/2016 03:18 PM, Paolo Bonzini wrote:
> This series started as an attempt to always use the dataplane path
> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> was three-fold:
>
> 1) to add more coverage for dataplane
>
> 2) to remove virtio_add_queue_aio
>
> 3) to simplify the dataplane start/stop code
>
> It achieves the first two objectives, and while it doesn't quite
> achieve the third it does cleanup the generic ioeventfd code in
> virtio-bus more than I expected.  In particular, it reduces the set
> of callbacks that transports must implement, and it removes the ugly
> case where ioeventfd is started with generic callbacks and then moved
> to the dataplane callbacks.  It also enables some simplification of the
> functions that deal with host notifiers.
>
> I've tested it with virtio-blk, virtio-scsi and vhost-net.
>
> Patches 1 and 2 are simplifications that are too nice to leave
> them for later in the series.
>
> Patch 3 moves some of the ioeventfd code from virtio-bus.c to
> virtio.c.  At this point the transition is a bit half-assed, but
> this changes as soon as we remove the generic->dataplane
> handler transition.
>
> Patches 4 to 6 do exactly that, and then the spring cleaning
> begins, lasting for the whole second half of the series.
>
> Opinions, reviews and bug reports?

 is there a branch?
>>>
>>> ioeventfd-virtio in my github repo.
>>
>> Triggering
>> qemu-system-s390x: /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:771: 
>> virtio_blk_set_status: Assertion `!s->dataplane_started' failed.
>>
>> Is this based on the old version that still had this bug?
> 
> No, it's a new assertion (though it looks the same as the old one),
> and a stupid one too.  This is hopefully enough:
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9f09e29..52927f7 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -768,7 +768,10 @@ static void virtio_blk_set_status(VirtIODevice *vdev, 
> uint8_t status)
>  {
>  VirtIOBlock *s = VIRTIO_BLK(vdev);
> 
> -assert(!s->dataplane_started);
> +if (!(status & (VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK))) {
> +assert(!s->dataplane_started);
> +}
> +
>  if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>  return;

seems to fix the assert. 

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

On 27/09/2016 16:45, Christian Borntraeger wrote:
> On 09/21/2016 06:43 PM, Paolo Bonzini wrote:
>>
>>
>> On 21/09/2016 16:01, Christian Borntraeger wrote:
>>> On 09/21/2016 03:18 PM, Paolo Bonzini wrote:
 This series started as an attempt to always use the dataplane path
 for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
 was three-fold:

 1) to add more coverage for dataplane

 2) to remove virtio_add_queue_aio

 3) to simplify the dataplane start/stop code

 It achieves the first two objectives, and while it doesn't quite
 achieve the third it does cleanup the generic ioeventfd code in
 virtio-bus more than I expected.  In particular, it reduces the set
 of callbacks that transports must implement, and it removes the ugly
 case where ioeventfd is started with generic callbacks and then moved
 to the dataplane callbacks.  It also enables some simplification of the
 functions that deal with host notifiers.

 I've tested it with virtio-blk, virtio-scsi and vhost-net.

 Patches 1 and 2 are simplifications that are too nice to leave
 them for later in the series.

 Patch 3 moves some of the ioeventfd code from virtio-bus.c to
 virtio.c.  At this point the transition is a bit half-assed, but
 this changes as soon as we remove the generic->dataplane
 handler transition.

 Patches 4 to 6 do exactly that, and then the spring cleaning
 begins, lasting for the whole second half of the series.

 Opinions, reviews and bug reports?
>>>
>>> is there a branch?
>>
>> ioeventfd-virtio in my github repo.
> 
> Triggering
> qemu-system-s390x: /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:771: 
> virtio_blk_set_status: Assertion `!s->dataplane_started' failed.
> 
> Is this based on the old version that still had this bug?

No, it's a new assertion (though it looks the same as the old one),
and a stupid one too.  This is hopefully enough:

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9f09e29..52927f7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -768,7 +768,10 @@ static void virtio_blk_set_status(VirtIODevice *vdev, 
uint8_t status)
 {
 VirtIOBlock *s = VIRTIO_BLK(vdev);
 
-assert(!s->dataplane_started);
+if (!(status & (VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK))) {
+assert(!s->dataplane_started);
+}
+
 if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
 return;
 }


Paolo

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Christian Borntraeger]

On 09/21/2016 06:43 PM, Paolo Bonzini wrote:
> 
> 
> On 21/09/2016 16:01, Christian Borntraeger wrote:
>> On 09/21/2016 03:18 PM, Paolo Bonzini wrote:
>>> This series started as an attempt to always use the dataplane path
>>> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
>>> was three-fold:
>>>
>>> 1) to add more coverage for dataplane
>>>
>>> 2) to remove virtio_add_queue_aio
>>>
>>> 3) to simplify the dataplane start/stop code
>>>
>>> It achieves the first two objectives, and while it doesn't quite
>>> achieve the third it does cleanup the generic ioeventfd code in
>>> virtio-bus more than I expected.  In particular, it reduces the set
>>> of callbacks that transports must implement, and it removes the ugly
>>> case where ioeventfd is started with generic callbacks and then moved
>>> to the dataplane callbacks.  It also enables some simplification of the
>>> functions that deal with host notifiers.
>>>
>>> I've tested it with virtio-blk, virtio-scsi and vhost-net.
>>>
>>> Patches 1 and 2 are simplifications that are too nice to leave
>>> them for later in the series.
>>>
>>> Patch 3 moves some of the ioeventfd code from virtio-bus.c to
>>> virtio.c.  At this point the transition is a bit half-assed, but
>>> this changes as soon as we remove the generic->dataplane
>>> handler transition.
>>>
>>> Patches 4 to 6 do exactly that, and then the spring cleaning
>>> begins, lasting for the whole second half of the series.
>>>
>>> Opinions, reviews and bug reports?
>>
>> is there a branch?
> 
> ioeventfd-virtio in my github repo.

Triggering
qemu-system-s390x: /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:771: 
virtio_blk_set_status: Assertion `!s->dataplane_started' failed.

Is this based on the old version that still had this bug?

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Michael S. Tsirkin]

On Wed, Sep 21, 2016 at 03:18:47PM +0200, Paolo Bonzini wrote:
> This series started as an attempt to always use the dataplane path
> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> was three-fold:
> 
> 1) to add more coverage for dataplane
> 
> 2) to remove virtio_add_queue_aio
> 
> 3) to simplify the dataplane start/stop code
> 
> It achieves the first two objectives, and while it doesn't quite
> achieve the third it does cleanup the generic ioeventfd code in
> virtio-bus more than I expected.  In particular, it reduces the set
> of callbacks that transports must implement, and it removes the ugly
> case where ioeventfd is started with generic callbacks and then moved
> to the dataplane callbacks.  It also enables some simplification of the
> functions that deal with host notifiers.
> 
> I've tested it with virtio-blk, virtio-scsi and vhost-net.
> 
> Patches 1 and 2 are simplifications that are too nice to leave
> them for later in the series.
> 
> Patch 3 moves some of the ioeventfd code from virtio-bus.c to
> virtio.c.  At this point the transition is a bit half-assed, but
> this changes as soon as we remove the generic->dataplane
> handler transition.
> 
> Patches 4 to 6 do exactly that, and then the spring cleaning
> begins, lasting for the whole second half of the series.
> 
> Opinions, reviews and bug reports?
> 
> Thanks,
> 
> Paolo

Overall this looks good, I'll try to review early next week.
Thanks!

> Paolo Bonzini (12):
>   virtio: move ioeventfd_disabled flag to VirtioBusState
>   virtio: move ioeventfd_started flag to VirtioBusState
>   virtio: add start_ioeventfd and stop_ioeventfd to VirtioDeviceClass
>   virtio: introduce virtio_device_ioeventfd_enabled
>   virtio-blk: always use dataplane path if ioeventfd is active
>   virtio-scsi: always use dataplane path if ioeventfd is active
>   Revert "virtio: Introduce virtio_add_queue_aio"
>   virtio: remove set_handler argument from set_host_notifier_internal
>   virtio: remove ioeventfd_disabled altogether
>   virtio: do not export set_host_notifier_internal
>   virtio: inline virtio_queue_set_host_notifier_fd_handler
>   virtio: inline set_host_notifier_internal
> 
>  hw/block/dataplane/virtio-blk.c |  67 +++---
>  hw/block/dataplane/virtio-blk.h |   6 +-
>  hw/block/virtio-blk.c   |  16 ++---
>  hw/s390x/virtio-ccw.c   |  36 +-
>  hw/s390x/virtio-ccw.h   |   2 -
>  hw/scsi/virtio-scsi-dataplane.c |  51 --
>  hw/scsi/virtio-scsi.c   |  24 +++
>  hw/virtio/vhost.c   |   5 +-
>  hw/virtio/virtio-bus.c  | 153 
> +++-
>  hw/virtio/virtio-mmio.c |  35 +
>  hw/virtio/virtio-pci.c  |  32 +
>  hw/virtio/virtio-pci.h  |   2 -
>  hw/virtio/virtio.c  | 139 +++-
>  include/hw/virtio/virtio-bus.h  |  27 ---
>  include/hw/virtio/virtio-scsi.h |   6 +-
>  include/hw/virtio/virtio.h  |  11 +--
>  16 files changed, 272 insertions(+), 340 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Paolo Bonzini]

On 21/09/2016 16:01, Christian Borntraeger wrote:
> On 09/21/2016 03:18 PM, Paolo Bonzini wrote:
>> This series started as an attempt to always use the dataplane path
>> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
>> was three-fold:
>>
>> 1) to add more coverage for dataplane
>>
>> 2) to remove virtio_add_queue_aio
>>
>> 3) to simplify the dataplane start/stop code
>>
>> It achieves the first two objectives, and while it doesn't quite
>> achieve the third it does cleanup the generic ioeventfd code in
>> virtio-bus more than I expected.  In particular, it reduces the set
>> of callbacks that transports must implement, and it removes the ugly
>> case where ioeventfd is started with generic callbacks and then moved
>> to the dataplane callbacks.  It also enables some simplification of the
>> functions that deal with host notifiers.
>>
>> I've tested it with virtio-blk, virtio-scsi and vhost-net.
>>
>> Patches 1 and 2 are simplifications that are too nice to leave
>> them for later in the series.
>>
>> Patch 3 moves some of the ioeventfd code from virtio-bus.c to
>> virtio.c.  At this point the transition is a bit half-assed, but
>> this changes as soon as we remove the generic->dataplane
>> handler transition.
>>
>> Patches 4 to 6 do exactly that, and then the spring cleaning
>> begins, lasting for the whole second half of the series.
>>
>> Opinions, reviews and bug reports?
> 
> is there a branch?

ioeventfd-virtio in my github repo.

Re: [Qemu-devel] [PATCH 00/12] virtio: cleanup ioeventfd start/stop

[Christian Borntraeger]

On 09/21/2016 03:18 PM, Paolo Bonzini wrote:
> This series started as an attempt to always use the dataplane path
> for virtio-blk and virtio-scsi when ioeventfd is active.  The aim
> was three-fold:
> 
> 1) to add more coverage for dataplane
> 
> 2) to remove virtio_add_queue_aio
> 
> 3) to simplify the dataplane start/stop code
> 
> It achieves the first two objectives, and while it doesn't quite
> achieve the third it does cleanup the generic ioeventfd code in
> virtio-bus more than I expected.  In particular, it reduces the set
> of callbacks that transports must implement, and it removes the ugly
> case where ioeventfd is started with generic callbacks and then moved
> to the dataplane callbacks.  It also enables some simplification of the
> functions that deal with host notifiers.
> 
> I've tested it with virtio-blk, virtio-scsi and vhost-net.
> 
> Patches 1 and 2 are simplifications that are too nice to leave
> them for later in the series.
> 
> Patch 3 moves some of the ioeventfd code from virtio-bus.c to
> virtio.c.  At this point the transition is a bit half-assed, but
> this changes as soon as we remove the generic->dataplane
> handler transition.
> 
> Patches 4 to 6 do exactly that, and then the spring cleaning
> begins, lasting for the whole second half of the series.
> 
> Opinions, reviews and bug reports?

is there a branch?