On Thu, Jun 20, 2024 at 4:08 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote: > > On Wed, Apr 24, 2024 at 05:15:28PM +0800, Xuan Zhuo wrote: > > Currently, the init_vqs function within the virtio_balloon driver relies > > on the condition that certain names array entries are null in order to > > skip the initialization of some virtual queues (vqs). This behavior is > > unique to this part of the codebase. In an upcoming commit, we plan to > > eliminate this dependency by removing the function entirely. Therefore, > > with this change, we are ensuring that the virtio_balloon no longer > > depends on the aforementioned function. > > > > As specification 1.0-1.2, vq indexes should not be contiguous if some > > vq does not exist. But currently the virtqueue index is contiguous for > > all existing devices. The Linux kernel does not implement functionality > > to allow vq indexes to be discontinuous. So the current behavior of the > > virtio-balloon device is different for the spec. But this commit has no > > functional changes. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> > > Acked-by: David Hildenbrand <david@xxxxxxxxxx> > > Acked-by: Jason Wang <jasowang@xxxxxxxxxx> > > I can't make heads of tails of this. > > David you acked so maybe you can help rewrite the commit log here? > > I don't understand what this says. > What in the balloon driver is out of spec? The problem is the spec has bug, see this: https://www.mail-archive.com/linux-um@xxxxxxxxxxxxxxxxxxx/msg04359.html Thanks > NULL in names *exactly* allows skipping init for some vqs. > How is that "does not implement"? > > And so on. > > > > --- > > drivers/virtio/virtio_balloon.c | 48 ++++++++++++++------------------- > > 1 file changed, 20 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > > index c0a63638f95e..ccda6d08493f 100644 > > --- a/drivers/virtio/virtio_balloon.c > > +++ b/drivers/virtio/virtio_balloon.c > > @@ -548,49 +548,41 @@ static int init_vqs(struct virtio_balloon *vb) > > struct virtqueue *vqs[VIRTIO_BALLOON_VQ_MAX]; > > vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX]; > > const char *names[VIRTIO_BALLOON_VQ_MAX]; > > - int err; > > + int err, idx = 0; > > > > - /* > > - * Inflateq and deflateq are used unconditionally. The names[] > > - * will be NULL if the related feature is not enabled, which will > > - * cause no allocation for the corresponding virtqueue in find_vqs. > > - */ > > - callbacks[VIRTIO_BALLOON_VQ_INFLATE] = balloon_ack; > > - names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate"; > > - callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack; > > - names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate"; > > - callbacks[VIRTIO_BALLOON_VQ_STATS] = NULL; > > - names[VIRTIO_BALLOON_VQ_STATS] = NULL; > > - callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL; > > - names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL; > > - names[VIRTIO_BALLOON_VQ_REPORTING] = NULL; > > + callbacks[idx] = balloon_ack; > > + names[idx++] = "inflate"; > > + callbacks[idx] = balloon_ack; > > + names[idx++] = "deflate"; > > > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { > > - names[VIRTIO_BALLOON_VQ_STATS] = "stats"; > > - callbacks[VIRTIO_BALLOON_VQ_STATS] = stats_request; > > + names[idx] = "stats"; > > + callbacks[idx++] = stats_request; > > } > > > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) { > > - names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq"; > > - callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL; > > + names[idx] = "free_page_vq"; > > + callbacks[idx++] = NULL; > > } > > > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) { > > - names[VIRTIO_BALLOON_VQ_REPORTING] = "reporting_vq"; > > - callbacks[VIRTIO_BALLOON_VQ_REPORTING] = balloon_ack; > > + names[idx] = "reporting_vq"; > > + callbacks[idx++] = balloon_ack; > > } > > > > - err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs, > > - callbacks, names, NULL); > > + err = virtio_find_vqs(vb->vdev, idx, vqs, callbacks, names, NULL); > > if (err) > > return err; > > > > - vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE]; > > - vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE]; > > + idx = 0; > > + > > + vb->inflate_vq = vqs[idx++]; > > + vb->deflate_vq = vqs[idx++]; > > + > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { > > struct scatterlist sg; > > unsigned int num_stats; > > - vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS]; > > + vb->stats_vq = vqs[idx++]; > > > > /* > > * Prime this virtqueue with one buffer so the hypervisor can > > @@ -610,10 +602,10 @@ static int init_vqs(struct virtio_balloon *vb) > > } > > > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) > > - vb->free_page_vq = vqs[VIRTIO_BALLOON_VQ_FREE_PAGE]; > > + vb->free_page_vq = vqs[idx++]; > > > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) > > - vb->reporting_vq = vqs[VIRTIO_BALLOON_VQ_REPORTING]; > > + vb->reporting_vq = vqs[idx++]; > > > > return 0; > > } > > -- > > 2.32.0.3.g01195cf9f >
