On Fri, Mar 3, 2023 at 12:58 PM Gautam Dawar <gautam.da...@amd.com> wrote: > > Guest announce capability is emulated by qemu in the .avail_handler > shadow virtqueue operation. It updates the status to success in > `*s->status` but incorrectly fetches the command execution > status from local variable `status` later in call to iov_from_buf(). > As `status` is initialized to VIRTIO_NET_ERR, it results in a > warning "Failed to ack link announce" in virtio_net driver's > virtnet_ack_link_announce() function after VM Live Migration. > Also, I noticed an invalid check in vhost_vdpa_net_handle_ctrl_avail() > that reports an error because status is not updated in call to > virtio_net_handle_ctrl_iov(): >
status should be updated through &in. It is declared as: const struct iovec in = { .iov_base = &status, .iov_len = sizeof(status), }; And it should be filled at the end of virtio_net_handle_ctrl_iov with a call to: s = iov_from_buf(in_sg, in_num, 0, &status, sizeof(status)); How do you obtain different values? Maybe const optimizations is invalid and the compiler thinks virtio_net_handle_ctrl_iov will never change status? Thanks! > virtio_net_handle_ctrl_iov(svq->vdev, &in, 1, &out, 1); > if (status != VIRTIO_NET_OK) { > error_report("Bad CVQ processing in model"); > } > Adding an optional OUT status parameter to virtio_net_handle_ctrl_iov() > would help resolving this issue and also send the correct status > value to the virtio-net driver. > > Signed-off-by: Gautam Dawar <gautam.da...@amd.com> > --- > hw/net/virtio-net.c | 9 +++++++-- > include/hw/virtio/virtio-net.h | 3 ++- > net/vhost-vdpa.c | 3 +-- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 3ae909041a..36a75592da 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -1503,7 +1503,8 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t > cmd, > size_t virtio_net_handle_ctrl_iov(VirtIODevice *vdev, > const struct iovec *in_sg, unsigned in_num, > const struct iovec *out_sg, > - unsigned out_num) > + unsigned out_num, > + virtio_net_ctrl_ack *status_out) > { > VirtIONet *n = VIRTIO_NET(vdev); > struct virtio_net_ctrl_hdr ctrl; > @@ -1514,6 +1515,8 @@ size_t virtio_net_handle_ctrl_iov(VirtIODevice *vdev, > if (iov_size(in_sg, in_num) < sizeof(status) || > iov_size(out_sg, out_num) < sizeof(ctrl)) { > virtio_error(vdev, "virtio-net ctrl missing headers"); > + if (status_out) > + *status_out = status; > return 0; > } > > @@ -1540,6 +1543,8 @@ size_t virtio_net_handle_ctrl_iov(VirtIODevice *vdev, > assert(s == sizeof(status)); > > g_free(iov2); > + if (status_out) > + *status_out = status; > return sizeof(status); > } > > @@ -1555,7 +1560,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, > VirtQueue *vq) > } > > written = virtio_net_handle_ctrl_iov(vdev, elem->in_sg, elem->in_num, > - elem->out_sg, elem->out_num); > + elem->out_sg, elem->out_num, > NULL); > if (written > 0) { > virtqueue_push(vq, elem, written); > virtio_notify(vdev, vq); > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h > index ef234ffe7e..da76cc414d 100644 > --- a/include/hw/virtio/virtio-net.h > +++ b/include/hw/virtio/virtio-net.h > @@ -224,7 +224,8 @@ struct VirtIONet { > size_t virtio_net_handle_ctrl_iov(VirtIODevice *vdev, > const struct iovec *in_sg, unsigned in_num, > const struct iovec *out_sg, > - unsigned out_num); > + unsigned out_num, > + virtio_net_ctrl_ack *status); > void virtio_net_set_netclient_name(VirtIONet *n, const char *name, > const char *type); > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index de5ed8ff22..c72b338633 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -638,8 +638,7 @@ static int > vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq, > return VIRTIO_NET_ERR; > } > > - status = VIRTIO_NET_ERR; > - virtio_net_handle_ctrl_iov(svq->vdev, &in, 1, &out, 1); > + virtio_net_handle_ctrl_iov(svq->vdev, &in, 1, &out, 1, &status); > if (status != VIRTIO_NET_OK) { > error_report("Bad CVQ processing in model"); > } > -- > 2.30.1 >