On Wed, Dec 13, 2017 at 04:45:20PM +0300, Ilya Maximets wrote: > >> That > >> looks very strange. Some of the functions gets 'old_status', others > >> the 'new_status'. I'm a bit confused. > > > > OK, fair enough. Fixed - let's pass old status everywhere, > > users that need the new one can get it from the vdev. > > > >> And it's not functional in current state: > >> > >> hw/net/virtio-net.c:264:28: error: ‘status’ undeclared > > > > Fixed too. new version below. > > This doesn't fix the segmentation fault.
Hmm you are right. Looking into it. > I have exactly same crash stacktrace: > > #0 vhost_memory_unmap hw/virtio/vhost.c:446 > #1 vhost_virtqueue_stop hw/virtio/vhost.c:1155 > #2 vhost_dev_stop hw/virtio/vhost.c:1594 > #3 vhost_net_stop_one hw/net/vhost_net.c:289 > #4 vhost_net_stop hw/net/vhost_net.c:368 > #5 virtio_net_vhost_status (old_status=15 '\017', n=0x5625f3901100) at > hw/net/virtio-net.c:180 > #6 virtio_net_set_status (vdev=0x5625f3901100, old_status=<optimized out>) > at hw/net/virtio-net.c:254 > #7 virtio_set_status (vdev=vdev@entry=0x5625f3901100, val=<optimized out>) > at hw/virtio/virtio.c:1152 > #8 virtio_error (vdev=0x5625f3901100, fmt=fmt@entry=0x5625f014f688 "Guest > says index %u is available") at hw/virtio/virtio.c:2460 BTW what is causing this? Why is guest avail index corrupted? > #9 virtqueue_get_head at hw/virtio/virtio.c:543 > #10 virtqueue_drop_all hw/virtio/virtio.c:984 > #11 virtio_net_drop_tx_queue_data hw/net/virtio-net.c:240 > #12 virtio_bus_set_host_notifier at hw/virtio/virtio-bus.c:297 > #13 vhost_dev_disable_notifiers at hw/virtio/vhost.c:1431 > #14 vhost_net_stop_one at hw/net/vhost_net.c:290 > #15 vhost_net_stop at hw/net/vhost_net.c:368 > #16 virtio_net_vhost_status (old_status=15 '\017', n=0x5625f3901100) at > hw/net/virtio-net.c:180 > #17 virtio_net_set_status (vdev=0x5625f3901100, old_status=<optimized out>) > at hw/net/virtio-net.c:254 > #18 qmp_set_link at net/net.c:1430 > #19 chr_closed_bh at net/vhost-user.c:214 > #20 aio_bh_call at util/async.c:90 > #21 aio_bh_poll at util/async.c:118 > #22 aio_dispatch at util/aio-posix.c:429 > #23 aio_ctx_dispatch at util/async.c:261 > #24 g_main_context_dispatch () from /lib64/libglib-2.0.so.0 > #25 glib_pollfds_poll () at util/main-loop.c:213 > #26 os_host_main_loop_wait at util/main-loop.c:261 > #27 main_loop_wait at util/main-loop.c:515 > #28 main_loop () at vl.c:1917 > #29 main > > > Actually the logic doesn't change. In function virtio_net_vhost_status(): > > - if ((virtio_net_started(n, status) && !nc->peer->link_down) == > + if ((virtio_net_started(n, vdev->status) && !nc->peer->link_down) == > !!n->vhost_started) { > return; > } > > previously new 'status' was checked and the new 'vdev->status' checked now. > It's the same condition that doesn't work because vhost_started flag is still > set to 1. > Anyway, nc->peer->link_down is true in our case, so there is no difference if > we'll change the vdev->status. > > >>> > >>> Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > > > > Still completely untested, sorry about that - hope you can help here. > > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > index 098bdaa..f5d0ee1 100644 > > --- a/include/hw/virtio/virtio.h > > +++ b/include/hw/virtio/virtio.h > > @@ -115,7 +115,7 @@ typedef struct VirtioDeviceClass { > > void (*get_config)(VirtIODevice *vdev, uint8_t *config); > > void (*set_config)(VirtIODevice *vdev, const uint8_t *config); > > void (*reset)(VirtIODevice *vdev); > > - void (*set_status)(VirtIODevice *vdev, uint8_t val); > > + void (*set_status)(VirtIODevice *vdev, uint8_t old_status); > > /* For transitional devices, this is a bitmap of features > > * that are only exposed on the legacy interface but not > > * the modern one. > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 05d1440..b8b07ba 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -814,15 +814,15 @@ static uint64_t virtio_blk_get_features(VirtIODevice > > *vdev, uint64_t features, > > return features; > > } > > > > -static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status) > > +static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t old_status) > > { > > VirtIOBlock *s = VIRTIO_BLK(vdev); > > > > - if (!(status & (VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK))) { > > + if (!(vdev->status & (VIRTIO_CONFIG_S_DRIVER | > > VIRTIO_CONFIG_S_DRIVER_OK))) { > > assert(!s->dataplane_started); > > } > > > > - if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) { > > + if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { > > return; > > } > > > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c > > index 9470bd7..881b1ff 100644 > > --- a/hw/char/virtio-serial-bus.c > > +++ b/hw/char/virtio-serial-bus.c > > @@ -616,7 +616,7 @@ static void guest_reset(VirtIOSerial *vser) > > } > > } > > > > -static void set_status(VirtIODevice *vdev, uint8_t status) > > +static void set_status(VirtIODevice *vdev, uint8_t old_status) > > { > > VirtIOSerial *vser; > > VirtIOSerialPort *port; > > @@ -625,7 +625,7 @@ static void set_status(VirtIODevice *vdev, uint8_t > > status) > > port = find_port_by_id(vser, 0); > > > > if (port && !use_multiport(port->vser) > > - && (status & VIRTIO_CONFIG_S_DRIVER_OK)) { > > + && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { > > /* > > * Non-multiport guests won't be able to tell us guest > > * open/close status. Such guests can only have a port at id > > @@ -634,7 +634,7 @@ static void set_status(VirtIODevice *vdev, uint8_t > > status) > > */ > > port->guest_connected = true; > > } > > - if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) { > > + if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { > > guest_reset(vser); > > } > > > > diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c > > index 0e42f0d..abb817b 100644 > > --- a/hw/input/virtio-input.c > > +++ b/hw/input/virtio-input.c > > @@ -188,12 +188,12 @@ static uint64_t > > virtio_input_get_features(VirtIODevice *vdev, uint64_t f, > > return f; > > } > > > > -static void virtio_input_set_status(VirtIODevice *vdev, uint8_t val) > > +static void virtio_input_set_status(VirtIODevice *vdev, uint8_t old_val) > > { > > VirtIOInputClass *vic = VIRTIO_INPUT_GET_CLASS(vdev); > > VirtIOInput *vinput = VIRTIO_INPUT(vdev); > > > > - if (val & VIRTIO_CONFIG_S_DRIVER_OK) { > > + if (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) { > > if (!vinput->active) { > > vinput->active = true; > > if (vic->change_active) { > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index 38674b0..7851968 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -124,7 +124,7 @@ static void virtio_net_announce_timer(void *opaque) > > virtio_notify_config(vdev); > > } > > > > -static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > > +static void virtio_net_vhost_status(VirtIONet *n, uint8_t old_status) > > { > > VirtIODevice *vdev = VIRTIO_DEVICE(n); > > NetClientState *nc = qemu_get_queue(n->nic); > > @@ -134,7 +134,7 @@ static void virtio_net_vhost_status(VirtIONet *n, > > uint8_t status) > > return; > > } > > > > - if ((virtio_net_started(n, status) && !nc->peer->link_down) == > > + if ((virtio_net_started(n, vdev->status) && !nc->peer->link_down) == > > !!n->vhost_started) { > > return; > > } > > @@ -212,12 +212,12 @@ static bool virtio_net_set_vnet_endian(VirtIODevice > > *vdev, NetClientState *ncs, > > return false; > > } > > > > -static void virtio_net_vnet_endian_status(VirtIONet *n, uint8_t status) > > +static void virtio_net_vnet_endian_status(VirtIONet *n, uint8_t old_status) > > { > > VirtIODevice *vdev = VIRTIO_DEVICE(n); > > int queues = n->multiqueue ? n->max_queues : 1; > > > > - if (virtio_net_started(n, status)) { > > + if (virtio_net_started(n, vdev->status)) { > > /* Before using the device, we tell the network backend about the > > * endianness to use when parsing vnet headers. If the backend > > * can't do it, we fallback onto fixing the headers in the core > > @@ -225,7 +225,7 @@ static void virtio_net_vnet_endian_status(VirtIONet *n, > > uint8_t status) > > */ > > n->needs_vnet_hdr_swap = virtio_net_set_vnet_endian(vdev, > > n->nic->ncs, > > queues, true); > > - } else if (virtio_net_started(n, vdev->status)) { > > + } else if (virtio_net_started(n, old_status)) { > > /* After using the device, we need to reset the network backend to > > * the default (guest native endianness), otherwise the guest may > > * lose network connectivity if it is rebooted into a different > > @@ -243,15 +243,15 @@ static void > > virtio_net_drop_tx_queue_data(VirtIODevice *vdev, VirtQueue *vq) > > } > > } > > > > -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t > > status) > > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t > > old_status) > > { > > VirtIONet *n = VIRTIO_NET(vdev); > > VirtIONetQueue *q; > > int i; > > uint8_t queue_status; > > > > - virtio_net_vnet_endian_status(n, status); > > - virtio_net_vhost_status(n, status); > > + virtio_net_vnet_endian_status(n, old_status); > > + virtio_net_vhost_status(n, old_status); > > > > for (i = 0; i < n->max_queues; i++) { > > NetClientState *ncs = qemu_get_subqueue(n->nic, i); > > @@ -261,7 +261,7 @@ static void virtio_net_set_status(struct VirtIODevice > > *vdev, uint8_t status) > > if ((!n->multiqueue && i != 0) || i >= n->curr_queues) { > > queue_status = 0; > > } else { > > - queue_status = status; > > + queue_status = vdev->status; > > } > > queue_started = > > virtio_net_started(n, queue_status) && !n->vhost_started; > > @@ -2048,9 +2048,11 @@ static void virtio_net_device_unrealize(DeviceState > > *dev, Error **errp) > > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > VirtIONet *n = VIRTIO_NET(dev); > > int i, max_queues; > > + uint8_t old_status = vdev->status; > > > > /* This will stop vhost backend if appropriate. */ > > - virtio_net_set_status(vdev, 0); > > + vdev->status = 0; > > + virtio_net_set_status(vdev, old_status); > > > > g_free(n->netclient_name); > > n->netclient_name = NULL; > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > > index 9c1bea8..5a561e4 100644 > > --- a/hw/scsi/vhost-scsi.c > > +++ b/hw/scsi/vhost-scsi.c > > @@ -108,11 +108,11 @@ static void vhost_scsi_stop(VHostSCSI *s) > > vhost_scsi_common_stop(vsc); > > } > > > > -static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val) > > +static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t old_val) > > { > > VHostSCSI *s = VHOST_SCSI(vdev); > > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > - bool start = (val & VIRTIO_CONFIG_S_DRIVER_OK); > > + bool start = vdev->status & VIRTIO_CONFIG_S_DRIVER_OK; > > > > if (vsc->dev.started == start) { > > return; > > diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c > > index f7561e2..289a27e 100644 > > --- a/hw/scsi/vhost-user-scsi.c > > +++ b/hw/scsi/vhost-user-scsi.c > > @@ -38,11 +38,11 @@ static const int user_feature_bits[] = { > > VHOST_INVALID_FEATURE_BIT > > }; > > > > -static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status) > > +static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t > > old_status) > > { > > VHostUserSCSI *s = (VHostUserSCSI *)vdev; > > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s); > > - bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running; > > + bool start = (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && > > vdev->vm_running; > > > > if (vsc->dev.started == start) { > > return; > > diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c > > index 5ec1c6a..d3f445b 100644 > > --- a/hw/virtio/vhost-vsock.c > > +++ b/hw/virtio/vhost-vsock.c > > @@ -154,10 +154,10 @@ static void vhost_vsock_stop(VirtIODevice *vdev) > > vhost_dev_disable_notifiers(&vsock->vhost_dev, vdev); > > } > > > > -static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status) > > +static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t old_status) > > { > > VHostVSock *vsock = VHOST_VSOCK(vdev); > > - bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK; > > + bool should_start = vdev->status & VIRTIO_CONFIG_S_DRIVER_OK; > > > > if (!vdev->vm_running) { > > should_start = false; > > @@ -370,11 +370,14 @@ static void vhost_vsock_device_unrealize(DeviceState > > *dev, Error **errp) > > { > > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > VHostVSock *vsock = VHOST_VSOCK(dev); > > + uint8_t old_status; > > > > vhost_vsock_post_load_timer_cleanup(vsock); > > > > /* This will stop vhost backend if appropriate. */ > > - vhost_vsock_set_status(vdev, 0); > > + old_status = vdev->status; > > + vdev->status = 0; > > + vhost_vsock_set_status(vdev, old_status); > > > > vhost_dev_cleanup(&vsock->vhost_dev); > > virtio_cleanup(vdev); > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index 37cde38..84e897a 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -468,12 +468,12 @@ static void virtio_balloon_device_reset(VirtIODevice > > *vdev) > > } > > } > > > > -static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status) > > +static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t > > old_status) > > { > > VirtIOBalloon *s = VIRTIO_BALLOON(vdev); > > > > if (!s->stats_vq_elem && vdev->vm_running && > > - (status & VIRTIO_CONFIG_S_DRIVER_OK) && virtqueue_rewind(s->svq, > > 1)) { > > + (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && > > virtqueue_rewind(s->svq, 1)) { > > /* poll stats queue for the element we have discarded when the VM > > * was stopped */ > > virtio_balloon_receive_stats(vdev, s->svq); > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index ad564b0..4981858 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -1128,6 +1128,8 @@ static int virtio_validate_features(VirtIODevice > > *vdev) > > int virtio_set_status(VirtIODevice *vdev, uint8_t val) > > { > > VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > > + uint8_t old_status; > > + > > trace_virtio_set_status(vdev, val); > > > > if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { > > @@ -1140,10 +1142,13 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t > > val) > > } > > } > > } > > + > > + old_status = vdev->status; > > + vdev->status = val; > > + > > if (k->set_status) { > > - k->set_status(vdev, val); > > + k->set_status(vdev, old_status); > > } > > - vdev->status = val; > > return 0; > > } > > > > > > > >