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(-)
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():
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.dawar@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
On Fri, Mar 3, 2023 at 12:58 PM Gautam Dawar <gautam.dawar@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.dawar@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 >
On 3/3/23 20:28, Eugenio Perez Martin wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Fri, Mar 3, 2023 at 12:58 PM Gautam Dawar <gautam.dawar@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)); Apologies for a delayed response. This totally makes sense and I've not been able to reproduce this issue. > 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! I think the issue might have been a result of incorrectly returning VIRTIO_NET_F_GUEST_ANNOUNCE in the device features without handling of VIRTIO_NET_CTRL_ANNOUNCE_ACK in the parent vdpa driver. We can drop this patch. Gautam >> 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.dawar@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 >>
On Fri, Mar 03, 2023 at 05:28:10PM +0530, Gautam Dawar 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(): > > 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.dawar@amd.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Jason your tree right? > --- > 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
© 2016 - 2024 Red Hat, Inc.