On Wed, Mar 22, 2017 at 6:15 PM, Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> >> >> --- >> >> 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 _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization