Daniel Verkamp
2020-Jan-03 18:40 UTC
[PATCH v2 1/2] virtio-balloon: initialize all vq callbacks
Ensure that elements of the callbacks array that correspond to unavailable features are set to NULL; previously, they would be left uninitialized. Since the corresponding names array elements were explicitly set to NULL, the uninitialized callback pointers would not actually be dereferenced; however, the uninitialized callbacks elements would still be read in vp_find_vqs_msix() and used to calculate the number of MSI-X vectors required. Cc: stable at vger.kernel.org Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT") Reviewed-by: Cornelia Huck <cohuck at redhat.com> Signed-off-by: Daniel Verkamp <dverkamp at chromium.org> --- v1: https://lists.linuxfoundation.org/pipermail/virtualization/2019-December/044829.html Changes from v1: - Clarified "array" in commit message to "callbacks array" drivers/virtio/virtio_balloon.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 93f995f6cf36..8e400ece9273 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -475,7 +475,9 @@ static int init_vqs(struct virtio_balloon *vb) 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; if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { -- 2.24.1.735.g03f4e72817-goog
Daniel Verkamp
2020-Jan-03 18:40 UTC
[PATCH v2 2/2] virtio-pci: check name when counting MSI-X vectors
VQs without a name specified are not valid; they are skipped in the later loop that assigns MSI-X vectors to queues, but the per_vq_vectors loop above that counts the required number of vectors previously still counted any queue with a non-NULL callback as needing a vector. Add a check to the per_vq_vectors loop so that vectors with no name are not counted to make the two loops consistent. This prevents over-counting unnecessary vectors (e.g. for features which were not negotiated with the device). Cc: stable at vger.kernel.org Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT") Reviewed-by: Cornelia Huck <cohuck at redhat.com> Signed-off-by: Daniel Verkamp <dverkamp at chromium.org> --- v1: https://lists.linuxfoundation.org/pipermail/virtualization/2019-December/044828.html drivers/virtio/virtio_pci_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index f2862f66c2ac..222d630c41fc 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs, /* Best option: one for change interrupt, one per vq. */ nvectors = 1; for (i = 0; i < nvqs; ++i) - if (callbacks[i]) + if (names[i] && callbacks[i]) ++nvectors; } else { /* Second best: one for change, shared for all vqs. */ -- 2.24.1.735.g03f4e72817-goog
Wang, Wei W
2020-Jan-05 12:24 UTC
[PATCH v2 1/2] virtio-balloon: initialize all vq callbacks
On Saturday, January 4, 2020 2:41 AM, Daniel Verkamp wrote:> Subject: [PATCH v2 1/2] virtio-balloon: initialize all vq callbacks > > Ensure that elements of the callbacks array that correspond to unavailable > features are set to NULL; previously, they would be left uninitialized. > > Since the corresponding names array elements were explicitly set to NULL, > the uninitialized callback pointers would not actually be dereferenced; > however, the uninitialized callbacks elements would still be read in > vp_find_vqs_msix() and used to calculate the number of MSI-X vectors > required.The above description doesn't seem true after your second patch gets applied.> Cc: stable at vger.kernel.org > Fixes: 86a559787e6f ("virtio-balloon: > VIRTIO_BALLOON_F_FREE_PAGE_HINT") > Reviewed-by: Cornelia Huck <cohuck at redhat.com> > Signed-off-by: Daniel Verkamp <dverkamp at chromium.org> > --- > > v1: > https://lists.linuxfoundation.org/pipermail/virtualization/2019-December/04 > 4829.html > > Changes from v1: > - Clarified "array" in commit message to "callbacks array" > > drivers/virtio/virtio_balloon.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 93f995f6cf36..8e400ece9273 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -475,7 +475,9 @@ static int init_vqs(struct virtio_balloon *vb) > 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;Could you remove other redundant NULL initialization well? https://lists.linuxfoundation.org/pipermail/virtualization/2019-December/044837.html Best, Wei
Wang, Wei W
2020-Jan-05 12:27 UTC
[PATCH v2 2/2] virtio-pci: check name when counting MSI-X vectors
On Saturday, January 4, 2020 2:41 AM, Daniel Verkamp wrote:> Subject: [PATCH v2 2/2] virtio-pci: check name when counting MSI-X vectors > > VQs without a name specified are not valid; they are skipped in the later loop > that assigns MSI-X vectors to queues, but the per_vq_vectors loop above that > counts the required number of vectors previously still counted any queue with a > non-NULL callback as needing a vector. > > Add a check to the per_vq_vectors loop so that vectors with no name are not > counted to make the two loops consistent. This prevents over-counting > unnecessary vectors (e.g. for features which were not negotiated with the > device). > > Cc: stable at vger.kernel.org > Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT") > Reviewed-by: Cornelia Huck <cohuck at redhat.com> > Signed-off-by: Daniel Verkamp <dverkamp at chromium.org> > --- > > v1: > https://lists.linuxfoundation.org/pipermail/virtualization/2019-December/0448 > 28.html > > drivers/virtio/virtio_pci_common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_pci_common.c > b/drivers/virtio/virtio_pci_common.c > index f2862f66c2ac..222d630c41fc 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, > unsigned nvqs, > /* Best option: one for change interrupt, one per vq. */ > nvectors = 1; > for (i = 0; i < nvqs; ++i) > - if (callbacks[i]) > + if (names[i] && callbacks[i]) > ++nvectors; > } else { > /* Second best: one for change, shared for all vqs. */ > -- > 2.24.1.735.g03f4e72817-googReviewed-by: Wang, Wei W <wei.w.wang at intel.com> Best, Wei
Michael S. Tsirkin
2020-Jan-05 13:25 UTC
[PATCH v2 1/2] virtio-balloon: initialize all vq callbacks
On Sun, Jan 05, 2020 at 12:24:11PM +0000, Wang, Wei W wrote:> On Saturday, January 4, 2020 2:41 AM, Daniel Verkamp wrote: > > Subject: [PATCH v2 1/2] virtio-balloon: initialize all vq callbacks > > > > Ensure that elements of the callbacks array that correspond to unavailable > > features are set to NULL; previously, they would be left uninitialized. > > > > Since the corresponding names array elements were explicitly set to NULL, > > the uninitialized callback pointers would not actually be dereferenced; > > however, the uninitialized callbacks elements would still be read in > > vp_find_vqs_msix() and used to calculate the number of MSI-X vectors > > required. > > The above description doesn't seem true after your second patch gets applied. > > > Cc: stable at vger.kernel.org > > Fixes: 86a559787e6f ("virtio-balloon: > > VIRTIO_BALLOON_F_FREE_PAGE_HINT") > > Reviewed-by: Cornelia Huck <cohuck at redhat.com> > > Signed-off-by: Daniel Verkamp <dverkamp at chromium.org> > > --- > > > > v1: > > https://lists.linuxfoundation.org/pipermail/virtualization/2019-December/04 > > 4829.html > > > > Changes from v1: > > - Clarified "array" in commit message to "callbacks array" > > > > drivers/virtio/virtio_balloon.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > > index 93f995f6cf36..8e400ece9273 100644 > > --- a/drivers/virtio/virtio_balloon.c > > +++ b/drivers/virtio/virtio_balloon.c > > @@ -475,7 +475,9 @@ static int init_vqs(struct virtio_balloon *vb) > > 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; > > Could you remove other redundant NULL initialization well? > https://lists.linuxfoundation.org/pipermail/virtualization/2019-December/044837.html > > Best, > WeiI queued as is, can be tweaked by a patch on top. -- MST
Reasonably Related Threads
- [PATCH 2/2] virtio-pci: check name when counting MSI-X vectors
- [PATCH v2 1/2] virtio-balloon: initialize all vq callbacks
- [PATCH v2 1/2] virtio-balloon: initialize all vq callbacks
- [PATCH 1/2] virtio-balloon: initialize all vq callbacks
- [PATCH 1/2] virtio-balloon: initialize all vq callbacks