* [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue @ 2017-03-22 15:10 Ladi Prosek 2017-03-22 16:14 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Ladi Prosek @ 2017-03-22 15:10 UTC (permalink / raw) To: virtualization; +Cc: mst When init_vqs runs, virtio_balloon.stats is either uninitialized or contains stale values. The host updates its state with garbage data because it has no way of knowing that this is just a marker buffer used for signaling. This patch initializes all tags with U16_MAX which is guaranteed to be ignored by the host. The alternative fix would be to push an empty buffer in init_vqs but that's not easily done with the current virtio implementation and would still not eliminate the invariant that update_balloon_stats has to update all VIRTIO_BALLOON_S_NR fields. Signed-off-by: Ladi Prosek <lprosek@redhat.com> --- drivers/virtio/virtio_balloon.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 4e11915..d34f56f 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb) vb->deflate_vq = vqs[1]; if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { struct scatterlist sg; + int idx; vb->stats_vq = vqs[2]; /* * Prime this virtqueue with one buffer so the hypervisor can * use it to signal us later (it can't be broken yet!). */ + for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++) + update_stat(vb, idx, U16_MAX, 0); + sg_init_one(&sg, vb->stats, sizeof vb->stats); if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL) < 0) -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue 2017-03-22 15:10 [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue Ladi Prosek @ 2017-03-22 16:14 ` Michael S. Tsirkin 2017-03-22 17:05 ` Ladi Prosek 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2017-03-22 16:14 UTC (permalink / raw) To: Ladi Prosek; +Cc: virtualization On Wed, Mar 22, 2017 at 04:10:27PM +0100, Ladi Prosek wrote: > When init_vqs runs, virtio_balloon.stats is either uninitialized or > contains stale values. The host updates its state with garbage data > because it has no way of knowing that this is just a marker buffer > used for signaling. > > This patch initializes all tags with U16_MAX which is guaranteed to > be ignored by the host. > > The alternative fix would be to push an empty buffer in init_vqs but > that's not easily done with the current virtio implementation and > would still not eliminate the invariant that update_balloon_stats > has to update all VIRTIO_BALLOON_S_NR fields. > > Signed-off-by: Ladi Prosek <lprosek@redhat.com> > --- > drivers/virtio/virtio_balloon.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 4e11915..d34f56f 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb) > vb->deflate_vq = vqs[1]; > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { > struct scatterlist sg; > + int idx; > vb->stats_vq = vqs[2]; > > /* > * Prime this virtqueue with one buffer so the hypervisor can > * use it to signal us later (it can't be broken yet!). > */ > + for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++) > + update_stat(vb, idx, U16_MAX, 0); > + Can't we fill in actual values? > sg_init_one(&sg, vb->stats, sizeof vb->stats); > if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL) > < 0) > -- > 2.7.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue 2017-03-22 16:14 ` Michael S. Tsirkin @ 2017-03-22 17:05 ` Ladi Prosek 2017-03-22 17:15 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Ladi Prosek @ 2017-03-22 17:05 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization On Wed, Mar 22, 2017 at 5:14 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Mar 22, 2017 at 04:10:27PM +0100, Ladi Prosek wrote: >> When init_vqs runs, virtio_balloon.stats is either uninitialized or >> contains stale values. The host updates its state with garbage data >> because it has no way of knowing that this is just a marker buffer >> used for signaling. >> >> This patch initializes all tags with U16_MAX which is guaranteed to >> be ignored by the host. >> >> The alternative fix would be to push an empty buffer in init_vqs but >> that's not easily done with the current virtio implementation and >> would still not eliminate the invariant that update_balloon_stats >> has to update all VIRTIO_BALLOON_S_NR fields. >> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com> >> --- >> drivers/virtio/virtio_balloon.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c >> index 4e11915..d34f56f 100644 >> --- a/drivers/virtio/virtio_balloon.c >> +++ b/drivers/virtio/virtio_balloon.c >> @@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb) >> vb->deflate_vq = vqs[1]; >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { >> struct scatterlist sg; >> + int idx; >> vb->stats_vq = vqs[2]; >> >> /* >> * Prime this virtqueue with one buffer so the hypervisor can >> * use it to signal us later (it can't be broken yet!). >> */ >> + for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++) >> + update_stat(vb, idx, U16_MAX, 0); >> + > > Can't we fill in actual values? We can if we're fine with the unsolicited update. In QEMU, this would mean that you'll see valid stats in /machine/peripheral/balloon0/guest-stats without setting the guest-stats-polling-interval. And they would be updated on driver (re-)load, so kind of arbitrarily from host pov. On the other hand, this patch does not really comply with (5.5.6.3.1 Driver Requirements: Memory Statistics): "Driver MUST supply the same subset of statistics in all buffers submitted to the statsq." So going by the letter of the law, we should fill in actual values. Going by the (the purpose of the initial buffer is to wait for the host to request stats), we should push an empty buffer or invalid values. >> sg_init_one(&sg, vb->stats, sizeof vb->stats); >> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL) >> < 0) >> -- >> 2.7.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue 2017-03-22 17:05 ` Ladi Prosek @ 2017-03-22 17:15 ` Michael S. Tsirkin 2017-03-22 20:20 ` Ladi Prosek 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2017-03-22 17:15 UTC (permalink / raw) To: Ladi Prosek; +Cc: virtualization On Wed, Mar 22, 2017 at 06:05:39PM +0100, Ladi Prosek wrote: > On Wed, Mar 22, 2017 at 5:14 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Mar 22, 2017 at 04:10:27PM +0100, Ladi Prosek wrote: > >> When init_vqs runs, virtio_balloon.stats is either uninitialized or > >> contains stale values. The host updates its state with garbage data > >> because it has no way of knowing that this is just a marker buffer > >> used for signaling. > >> > >> This patch initializes all tags with U16_MAX which is guaranteed to > >> be ignored by the host. > >> > >> The alternative fix would be to push an empty buffer in init_vqs but > >> that's not easily done with the current virtio implementation and > >> would still not eliminate the invariant that update_balloon_stats > >> has to update all VIRTIO_BALLOON_S_NR fields. > >> > >> Signed-off-by: Ladi Prosek <lprosek@redhat.com> > >> --- > >> drivers/virtio/virtio_balloon.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > >> index 4e11915..d34f56f 100644 > >> --- a/drivers/virtio/virtio_balloon.c > >> +++ b/drivers/virtio/virtio_balloon.c > >> @@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb) > >> vb->deflate_vq = vqs[1]; > >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { > >> struct scatterlist sg; > >> + int idx; > >> vb->stats_vq = vqs[2]; > >> > >> /* > >> * Prime this virtqueue with one buffer so the hypervisor can > >> * use it to signal us later (it can't be broken yet!). > >> */ > >> + for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++) > >> + update_stat(vb, idx, U16_MAX, 0); > >> + > > > > Can't we fill in actual values? > > We can if we're fine with the unsolicited update. > > In QEMU, this would mean that you'll see valid stats in > /machine/peripheral/balloon0/guest-stats without setting the > guest-stats-polling-interval. And they would be updated on driver > (re-)load, so kind of arbitrarily from host pov. IUC the only issue would be that someone might come to depend on this. Doesn't look like a big deal to me. > On the other hand, this patch does not really comply with (5.5.6.3.1 > Driver Requirements: Memory Statistics): > > "Driver MUST supply the same subset of statistics in all buffers > submitted to the statsq." > > So going by the letter of the law, we should fill in actual values. > Going by the (the purpose of the initial buffer is to wait for the > host to request stats), we should push an empty buffer or invalid > values. > >> sg_init_one(&sg, vb->stats, sizeof vb->stats); > >> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL) > >> < 0) > >> -- > >> 2.7.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue 2017-03-22 17:15 ` Michael S. Tsirkin @ 2017-03-22 20:20 ` Ladi Prosek 0 siblings, 0 replies; 5+ messages in thread From: Ladi Prosek @ 2017-03-22 20:20 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization On Wed, Mar 22, 2017 at 6:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Mar 22, 2017 at 06:05:39PM +0100, Ladi Prosek wrote: >> On Wed, Mar 22, 2017 at 5:14 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Wed, Mar 22, 2017 at 04:10:27PM +0100, Ladi Prosek wrote: >> >> When init_vqs runs, virtio_balloon.stats is either uninitialized or >> >> contains stale values. The host updates its state with garbage data >> >> because it has no way of knowing that this is just a marker buffer >> >> used for signaling. >> >> >> >> This patch initializes all tags with U16_MAX which is guaranteed to >> >> be ignored by the host. >> >> >> >> The alternative fix would be to push an empty buffer in init_vqs but >> >> that's not easily done with the current virtio implementation and >> >> would still not eliminate the invariant that update_balloon_stats >> >> has to update all VIRTIO_BALLOON_S_NR fields. >> >> >> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com> >> >> --- >> >> drivers/virtio/virtio_balloon.c | 4 ++++ >> >> 1 file changed, 4 insertions(+) >> >> >> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c >> >> index 4e11915..d34f56f 100644 >> >> --- a/drivers/virtio/virtio_balloon.c >> >> +++ b/drivers/virtio/virtio_balloon.c >> >> @@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb) >> >> vb->deflate_vq = vqs[1]; >> >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { >> >> struct scatterlist sg; >> >> + int idx; >> >> vb->stats_vq = vqs[2]; >> >> >> >> /* >> >> * Prime this virtqueue with one buffer so the hypervisor can >> >> * use it to signal us later (it can't be broken yet!). >> >> */ >> >> + for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++) >> >> + update_stat(vb, idx, U16_MAX, 0); >> >> + >> > >> > Can't we fill in actual values? >> >> We can if we're fine with the unsolicited update. >> >> In QEMU, this would mean that you'll see valid stats in >> /machine/peripheral/balloon0/guest-stats without setting the >> guest-stats-polling-interval. And they would be updated on driver >> (re-)load, so kind of arbitrarily from host pov. > > IUC the only issue would be that someone might come to > depend on this. Doesn't look like a big deal to me. Ok, I'll post v2 of the patch to use actual values when sending the initial buffer. Btw, I checked the Windows driver and it's been sending all FF's in the first stats buffer since about 2011. Just for completeness, probably won't change your mind :) Thanks! Ladi >> On the other hand, this patch does not really comply with (5.5.6.3.1 >> Driver Requirements: Memory Statistics): >> >> "Driver MUST supply the same subset of statistics in all buffers >> submitted to the statsq." >> >> So going by the letter of the law, we should fill in actual values. >> Going by the (the purpose of the initial buffer is to wait for the >> host to request stats), we should push an empty buffer or invalid >> values. >> >> sg_init_one(&sg, vb->stats, sizeof vb->stats); >> >> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL) >> >> < 0) >> >> -- >> >> 2.7.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-22 20:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-22 15:10 [PATCH] virtio_balloon: don't push uninitialized buffers to stats virtqueue Ladi Prosek 2017-03-22 16:14 ` Michael S. Tsirkin 2017-03-22 17:05 ` Ladi Prosek 2017-03-22 17:15 ` Michael S. Tsirkin 2017-03-22 20:20 ` Ladi Prosek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.