* [PATCH] virtio: reset device on bad guest index in virtio_load()
@ 2020-11-20 18:51 John Levon
2020-11-30 15:51 ` John Levon
2020-12-02 9:50 ` Michael S. Tsirkin
0 siblings, 2 replies; 5+ messages in thread
From: John Levon @ 2020-11-20 18:51 UTC (permalink / raw)
To: qemu-devel@nongnu.org
If we find a queue with an inconsistent guest index value, explicitly mark the
device as needing a reset - and broken - via virtio_error().
There's at least one driver implementation - the virtio-win NetKVM driver - that
is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
restore the device to a working state. Other implementations do not correctly
handle this, but as the VQ is not in a functional state anyway, this is still
worth doing.
Signed-off-by: John Levon <john.levon@nutanix.com>
---
hw/virtio/virtio.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ceb58fda6c..eff35fab7c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
/* Check it isn't doing strange things with descriptor numbers. */
if (nheads > vdev->vq[i].vring.num) {
- qemu_log_mask(LOG_GUEST_ERROR,
- "VQ %d size 0x%x Guest index 0x%x "
- "inconsistent with Host index 0x%x: delta 0x%x",
- i, vdev->vq[i].vring.num,
- vring_avail_idx(&vdev->vq[i]),
- vdev->vq[i].last_avail_idx, nheads);
+ virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
+ "inconsistent with Host index 0x%x: delta 0x%x",
+ i, vdev->vq[i].vring.num,
+ vring_avail_idx(&vdev->vq[i]),
+ vdev->vq[i].last_avail_idx, nheads);
+ vdev->vq[i].used_idx = 0;
+ vdev->vq[i].shadow_avail_idx = 0;
+ vdev->vq[i].inuse = 0;
+ continue;
}
vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
--
2.22.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
2020-11-20 18:51 [PATCH] virtio: reset device on bad guest index in virtio_load() John Levon
@ 2020-11-30 15:51 ` John Levon
2020-11-30 15:52 ` Michael S. Tsirkin
2020-12-02 9:50 ` Michael S. Tsirkin
1 sibling, 1 reply; 5+ messages in thread
From: John Levon @ 2020-11-30 15:51 UTC (permalink / raw)
To: John Levon
Cc: Michael S . Tsirkin, Jason Wang, Markus Armbruster, Peter Xu,
qemu-devel@nongnu.org, Stefan Hajnoczi, Felipe Franciosi,
Paolo Bonzini
On Fri, Nov 20, 2020 at 06:51:07PM +0000, John Levon wrote:
> If we find a queue with an inconsistent guest index value, explicitly mark the
> device as needing a reset - and broken - via virtio_error().
>
> There's at least one driver implementation - the virtio-win NetKVM driver - that
> is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
> restore the device to a working state. Other implementations do not correctly
> handle this, but as the VQ is not in a functional state anyway, this is still
> worth doing.
Ping, anyone have issues with doing this?
cheers
john
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
> hw/virtio/virtio.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ceb58fda6c..eff35fab7c 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> /* Check it isn't doing strange things with descriptor numbers. */
> if (nheads > vdev->vq[i].vring.num) {
> - qemu_log_mask(LOG_GUEST_ERROR,
> - "VQ %d size 0x%x Guest index 0x%x "
> - "inconsistent with Host index 0x%x: delta 0x%x",
> - i, vdev->vq[i].vring.num,
> - vring_avail_idx(&vdev->vq[i]),
> - vdev->vq[i].last_avail_idx, nheads);
> + virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> + "inconsistent with Host index 0x%x: delta 0x%x",
> + i, vdev->vq[i].vring.num,
> + vring_avail_idx(&vdev->vq[i]),
> + vdev->vq[i].last_avail_idx, nheads);
> + vdev->vq[i].used_idx = 0;
> + vdev->vq[i].shadow_avail_idx = 0;
> + vdev->vq[i].inuse = 0;
> + continue;
> }
> vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
> vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
2020-11-30 15:51 ` John Levon
@ 2020-11-30 15:52 ` Michael S. Tsirkin
0 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2020-11-30 15:52 UTC (permalink / raw)
To: John Levon
Cc: Jason Wang, qemu-devel@nongnu.org, Peter Xu, Markus Armbruster,
Stefan Hajnoczi, Felipe Franciosi, Paolo Bonzini
No, but how about sending the patch to me and the mailing list?
I didn't get it through either channel.
On Mon, Nov 30, 2020 at 03:51:13PM +0000, John Levon wrote:
> On Fri, Nov 20, 2020 at 06:51:07PM +0000, John Levon wrote:
>
> > If we find a queue with an inconsistent guest index value, explicitly mark the
> > device as needing a reset - and broken - via virtio_error().
> >
> > There's at least one driver implementation - the virtio-win NetKVM driver - that
> > is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
> > restore the device to a working state. Other implementations do not correctly
> > handle this, but as the VQ is not in a functional state anyway, this is still
> > worth doing.
>
> Ping, anyone have issues with doing this?
>
> cheers
> john
>
> > Signed-off-by: John Levon <john.levon@nutanix.com>
> > ---
> > hw/virtio/virtio.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index ceb58fda6c..eff35fab7c 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> > nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> > /* Check it isn't doing strange things with descriptor numbers. */
> > if (nheads > vdev->vq[i].vring.num) {
> > - qemu_log_mask(LOG_GUEST_ERROR,
> > - "VQ %d size 0x%x Guest index 0x%x "
> > - "inconsistent with Host index 0x%x: delta 0x%x",
> > - i, vdev->vq[i].vring.num,
> > - vring_avail_idx(&vdev->vq[i]),
> > - vdev->vq[i].last_avail_idx, nheads);
> > + virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> > + "inconsistent with Host index 0x%x: delta 0x%x",
> > + i, vdev->vq[i].vring.num,
> > + vring_avail_idx(&vdev->vq[i]),
> > + vdev->vq[i].last_avail_idx, nheads);
> > + vdev->vq[i].used_idx = 0;
> > + vdev->vq[i].shadow_avail_idx = 0;
> > + vdev->vq[i].inuse = 0;
> > + continue;
> > }
> > vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
> > vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio: reset device on bad guest index in virtio_load()
2020-11-20 18:51 [PATCH] virtio: reset device on bad guest index in virtio_load() John Levon
2020-11-30 15:51 ` John Levon
@ 2020-12-02 9:50 ` Michael S. Tsirkin
1 sibling, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2020-12-02 9:50 UTC (permalink / raw)
To: John Levon
Cc: Jason Wang, qemu-devel@nongnu.org, Peter Xu, Markus Armbruster,
Stefan Hajnoczi, Felipe Franciosi, Paolo Bonzini
On Fri, Nov 20, 2020 at 06:51:07PM +0000, John Levon wrote:
>
> If we find a queue with an inconsistent guest index value, explicitly mark the
> device as needing a reset - and broken - via virtio_error().
>
> There's at least one driver implementation - the virtio-win NetKVM driver - that
> is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
> restore the device to a working state. Other implementations do not correctly
> handle this, but as the VQ is not in a functional state anyway, this is still
> worth doing.
>
> Signed-off-by: John Levon <john.levon@nutanix.com>
I tagged this for after the release. pls ping me after the release
to help make sure it does not get lost.
> ---
> hw/virtio/virtio.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ceb58fda6c..eff35fab7c 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> /* Check it isn't doing strange things with descriptor numbers. */
> if (nheads > vdev->vq[i].vring.num) {
> - qemu_log_mask(LOG_GUEST_ERROR,
> - "VQ %d size 0x%x Guest index 0x%x "
> - "inconsistent with Host index 0x%x: delta 0x%x",
> - i, vdev->vq[i].vring.num,
> - vring_avail_idx(&vdev->vq[i]),
> - vdev->vq[i].last_avail_idx, nheads);
> + virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> + "inconsistent with Host index 0x%x: delta 0x%x",
> + i, vdev->vq[i].vring.num,
> + vring_avail_idx(&vdev->vq[i]),
> + vdev->vq[i].last_avail_idx, nheads);
> + vdev->vq[i].used_idx = 0;
> + vdev->vq[i].shadow_avail_idx = 0;
> + vdev->vq[i].inuse = 0;
> + continue;
> }
> vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
> vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
> --
> 2.22.3
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] virtio: reset device on bad guest index in virtio_load()
@ 2020-11-20 17:47 John Levon
0 siblings, 0 replies; 5+ messages in thread
From: John Levon @ 2020-11-20 17:47 UTC (permalink / raw)
To: qemu-devel@nongnu.org; +Cc: levon@movementarian.org
If we find a queue with an inconsistent guest index value, explicitly mark the
device as needing a reset - and broken - via virtio_error().
There's at least one driver implementation - the virtio-win NetKVM driver - that
is able to handle a VIRTIO_CONFIG_S_NEEDS_RESET notification and successfully
restore the device to a working state. Other implementations do not correctly
handle this, but as the VQ is not in a functional state anyway, this is still
worth doing.
Signed-off-by: John Levon <john.levon@nutanix.com>
---
hw/virtio/virtio.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ceb58fda6c..eff35fab7c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3161,12 +3161,15 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
/* Check it isn't doing strange things with descriptor numbers. */
if (nheads > vdev->vq[i].vring.num) {
- qemu_log_mask(LOG_GUEST_ERROR,
- "VQ %d size 0x%x Guest index 0x%x "
- "inconsistent with Host index 0x%x: delta 0x%x",
- i, vdev->vq[i].vring.num,
- vring_avail_idx(&vdev->vq[i]),
- vdev->vq[i].last_avail_idx, nheads);
+ virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
+ "inconsistent with Host index 0x%x: delta 0x%x",
+ i, vdev->vq[i].vring.num,
+ vring_avail_idx(&vdev->vq[i]),
+ vdev->vq[i].last_avail_idx, nheads);
+ vdev->vq[i].used_idx = 0;
+ vdev->vq[i].shadow_avail_idx = 0;
+ vdev->vq[i].inuse = 0;
+ continue;
}
vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]);
--
2.22.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-02 10:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-20 18:51 [PATCH] virtio: reset device on bad guest index in virtio_load() John Levon
2020-11-30 15:51 ` John Levon
2020-11-30 15:52 ` Michael S. Tsirkin
2020-12-02 9:50 ` Michael S. Tsirkin
-- strict thread matches above, loose matches on Subject: below --
2020-11-20 17:47 John Levon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).