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.
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
