Re: [PATCH v2 2/2] virtio: fix vq # for balloon
From: Daniel Verkamp <hidden>
Date: 2024-07-10 18:13:05
Also in:
kvm, linux-remoteproc, linux-s390, linux-um, lkml
On Wed, Jul 10, 2024 at 4:43 AM Michael S. Tsirkin [off-list ref] wrote:
virtio balloon communicates to the core that in some configurations vq #s are non-contiguous by setting name pointer to NULL. Unfortunately, core then turned around and just made them contiguous again. Result is that driver is out of spec.
Thanks for fixing this - I think the overall approach of the patch looks good.
Implement what the API was supposed to do in the 1st place. Compatibility with buggy hypervisors is handled inside virtio-balloon, which is the only driver making use of this facility, so far.
In addition to virtio-balloon, I believe the same problem also affects the virtio-fs device, since queue 1 is only supposed to be present if VIRTIO_FS_F_NOTIFICATION is negotiated, and the request queues are meant to be queue indexes 2 and up. From a look at the Linux driver (virtio_fs.c), it appears like it never acks VIRTIO_FS_F_NOTIFICATION and assumes that request queues start at index 1 rather than 2, which looks out of spec to me, but the current device implementations (that I am aware of, anyway) are also broken in the same way, so it ends up working today. Queue numbering in a spec-compliant device and the current Linux driver would mismatch; what the driver considers to be the first request queue (index 1) would be ignored by the device since queue index 1 has no function if F_NOTIFICATION isn't negotiated. [...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index 7d82facafd75..fa606e7321ad 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c@@ -293,7 +293,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs, struct virtio_pci_device *vp_dev = to_vp_device(vdev); struct virtqueue_info *vqi; u16 msix_vec; - int i, err, nvectors, allocated_vectors, queue_idx = 0; + int i, err, nvectors, allocated_vectors; vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL); if (!vp_dev->vqs)@@ -332,7 +332,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs, msix_vec = allocated_vectors++; else msix_vec = VP_MSIX_VQ_VECTOR; - vqs[i] = vp_setup_vq(vdev, queue_idx++, vqi->callback, + vqs[i] = vp_setup_vq(vdev, i, vqi->callback, vqi->name, vqi->ctx, msix_vec); if (IS_ERR(vqs[i])) { err = PTR_ERR(vqs[i]);@@ -368,7 +368,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs, struct virtqueue_info vqs_info[]) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); - int i, err, queue_idx = 0; + int i, err; vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL); if (!vp_dev->vqs)@@ -388,8 +388,13 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs, vqs[i] = NULL; continue; } +<<<<<<< HEAD vqs[i] = vp_setup_vq(vdev, queue_idx++, vqi->callback, vqi->name, vqi->ctx, +======= + vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i], + ctx ? ctx[i] : false, +>>>>>>> f814759f80b7... virtio: fix vq # for balloon
This still has merge markers in it. Thanks, -- Daniel