Paolo Bonzini <[email protected]> writes: > Il 19/06/2013 10:24, Michael S. Tsirkin ha scritto: >>> > 2) We introduce VIRTIO_NET_F_ANY_LAYOUT and VIRTIO_BLK_F_ANY_LAYOUT >>> > specifically for net and block (note the new names). > > So why not a transport feature? Is it just because the SCSI commands > for virtio-blk also require a config space field? Sorry if I missed > this upthread.
Mainly because I'm not sure that *all* devices are now safe. Are they? I had a stress test half-written for this, pasted below. Otherwise I'd be happy to do both: use feature 25 for VIRTIO_F_ANY_LAYOUT and another feature bit for the virtio-blk layout change. Cheers, Rusty. virtio: CONFIG_VIRTIO_DEVICE_TORTURE Virtio devices are not supposed to depend on the framing of the scatter-gather lists, but various implementations did. Safeguard this in future by adding an option to deliberately create perverse descriptors. Signed-off-by: Rusty Russell <[email protected]> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig index 8d5bddb..99c0187 100644 --- a/drivers/virtio/Kconfig +++ b/drivers/virtio/Kconfig @@ -5,6 +5,22 @@ config VIRTIO bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST, CONFIG_RPMSG or CONFIG_S390_GUEST. +config VIRTIO_TORTURE + bool "Virtio torture debugging" + depends on VIRTIO && DEBUG_KERNEL + help + + This makes the virtio_ring implementation stress-test + devices. In particularly, creatively change the format of + requests to make sure that devices are properly implemented, + as well as implement various checks to ensure drivers are + correct. This will make your virtual machine slow *and* + unreliable! Say N. + + Put virtio_ring.device_torture to your boot commandline to + torture devices (otherwise only simply sanity checks are + done). + menu "Virtio drivers" config VIRTIO_PCI diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index e82821a..6e5271c 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -45,35 +45,6 @@ #define virtio_wmb(vq) wmb() #endif -#ifdef DEBUG -/* For development, we want to crash whenever the ring is screwed. */ -#define BAD_RING(_vq, fmt, args...) \ - do { \ - dev_err(&(_vq)->vq.vdev->dev, \ - "%s:"fmt, (_vq)->vq.name, ##args); \ - BUG(); \ - } while (0) -/* Caller is supposed to guarantee no reentry. */ -#define START_USE(_vq) \ - do { \ - if ((_vq)->in_use) \ - panic("%s:in_use = %i\n", \ - (_vq)->vq.name, (_vq)->in_use); \ - (_vq)->in_use = __LINE__; \ - } while (0) -#define END_USE(_vq) \ - do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0) -#else -#define BAD_RING(_vq, fmt, args...) \ - do { \ - dev_err(&_vq->vq.vdev->dev, \ - "%s:"fmt, (_vq)->vq.name, ##args); \ - (_vq)->broken = true; \ - } while (0) -#define START_USE(vq) -#define END_USE(vq) -#endif - struct indirect_cache { unsigned int max; struct vring_desc *cache; @@ -109,7 +80,7 @@ struct vring_virtqueue /* How to notify other side. FIXME: commonalize hcalls! */ void (*notify)(struct virtqueue *vq); -#ifdef DEBUG +#ifdef CONFIG_VIRTIO_TORTURE /* They're supposed to lock for us. */ unsigned int in_use; @@ -134,6 +105,200 @@ static inline struct indirect_cache *indirect_cache(struct vring_virtqueue *vq) return (struct indirect_cache *)&vq->data[vq->vring.num]; } +#ifdef CONFIG_VIRTIO_TORTURE +/* For development, we want to crash whenever the ring is screwed. */ +#define BAD_RING(_vq, fmt, args...) \ + do { \ + dev_err(&(_vq)->vq.vdev->dev, \ + "%s:"fmt, (_vq)->vq.name, ##args); \ + BUG(); \ + } while (0) +/* Caller is supposed to guarantee no reentry. */ +#define START_USE(_vq) \ + do { \ + if ((_vq)->in_use) \ + panic("%s:in_use = %i\n", \ + (_vq)->vq.name, (_vq)->in_use); \ + (_vq)->in_use = __LINE__; \ + } while (0) +#define END_USE(_vq) \ + do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0) + +static bool device_torture; +module_param(device_torture, bool, 0644); + +struct torture { + unsigned int orig_out, orig_in; + void *orig_data; + struct scatterlist sg[4]; + struct scatterlist orig_sg[]; +}; + +static unsigned tot_len(struct scatterlist sg[], unsigned num) +{ + unsigned len, i; + + for (len = 0, i = 0; i < num; i++) + len += sg[i].length; + + return len; +} + +static void copy_sg_data(const struct scatterlist *dst, unsigned dnum, + const struct scatterlist *src, unsigned snum) +{ + unsigned len; + struct scatterlist s, d; + + s = *src; + d = *dst; + + while (snum && dnum) { + len = min(s.length, d.length); + memcpy(sg_virt(&d), sg_virt(&s), len); + d.offset += len; + d.length -= len; + s.offset += len; + s.length -= len; + if (!s.length) { + BUG_ON(snum == 0); + src++; + snum--; + s = *src; + } + if (!d.length) { + BUG_ON(dnum == 0); + dst++; + dnum--; + d = *dst; + } + } +} + +static bool torture_replace(struct scatterlist **sg, + unsigned int *out, + unsigned int *in, + void **data, + gfp_t gfp) +{ + static size_t seed; + struct torture *t; + unsigned long outlen, inlen, ourseed, len1; + void *buf; + + if (!device_torture) + return true; + + outlen = tot_len(*sg, *out); + inlen = tot_len(*sg + *out, *in); + + /* This will break horribly on large block requests. */ + t = kmalloc(sizeof(*t) + (*out + *in) * sizeof(t->orig_sg[1]) + + outlen + 1 + inlen + 1, gfp); + if (!t) + return false; + + sg_init_table(t->sg, 4); + buf = &t->orig_sg[*out + *in]; + + memcpy(t->orig_sg, *sg, sizeof(**sg) * (*out + *in)); + t->orig_out = *out; + t->orig_in = *in; + t->orig_data = *data; + *data = t; + + ourseed = ACCESS_ONCE(seed); + seed++; + + *sg = t->sg; + if (outlen) { + /* Split outbuf into two parts, one byte apart. */ + *out = 2; + len1 = ourseed % (outlen + 1); + sg_set_buf(&t->sg[0], buf, len1); + buf += len1 + 1; + sg_set_buf(&t->sg[1], buf, outlen - len1); + buf += outlen - len1; + copy_sg_data(t->sg, *out, t->orig_sg, t->orig_out); + } + + if (inlen) { + /* Split inbuf into two parts, one byte apart. */ + *in = 2; + len1 = ourseed % (inlen + 1); + sg_set_buf(&t->sg[*out], buf, len1); + buf += len1 + 1; + sg_set_buf(&t->sg[*out + 1], buf, inlen - len1); + buf += inlen - len1; + } + return true; +} + +static void *torture_done(struct torture *t) +{ + void *data; + + if (!device_torture) + return t; + + if (t->orig_in) + copy_sg_data(t->orig_sg + t->orig_out, t->orig_in, + t->sg + (t->orig_out ? 2 : 0), 2); + + data = t->orig_data; + kfree(t); + return data; +} + +static unsigned long tot_inlen(struct virtqueue *vq, unsigned int i) +{ + struct vring_desc *desc; + unsigned long len = 0; + + if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) { + unsigned int num = vq->vring.desc[i].len / sizeof(*desc); + desc = phys_to_virt(vq->vring.desc[i].addr); + + for (i = 0; i < num; i++) { + if (desc[i].flags & VRING_DESC_F_WRITE) + len += desc[i].flags.len; + } + } else { + desc = vq->vring.desc; + while (desc[i].flags & VRING_DESC_F_NEXT) { + if (desc[i].flags & VRING_DESC_F_WRITE) + len += desc[i].flags.len; + i = desc[i].next; + } + } + + return len; +} +#else +static bool torture_replace(struct scatterlist **sg, + unsigned int *out, + unsigned int *in, + void **data, + gfp_t gfp) +{ + return true; +} + +static void *torture_done(void *data) +{ + return data; +} + +#define BAD_RING(_vq, fmt, args...) \ + do { \ + dev_err(&_vq->vq.vdev->dev, \ + "%s:"fmt, (_vq)->vq.name, ##args); \ + (_vq)->broken = true; \ + } while (0) +#define START_USE(vq) +#define END_USE(vq) +#endif /* CONFIG_VIRTIO_TORTURE */ + /* Set up an indirect table of descriptors and add it to the queue. */ static int vring_add_indirect(struct vring_virtqueue *vq, struct scatterlist sg[], @@ -228,7 +393,10 @@ int virtqueue_add_buf(struct virtqueue *_vq, BUG_ON(data == NULL); -#ifdef DEBUG + if (!torture_replace(&sg, &out, &in, &data, gfp)) + return -ENOMEM; + +#ifdef CONFIG_VIRTIO_TORTURE { ktime_t now = ktime_get(); @@ -261,6 +429,7 @@ int virtqueue_add_buf(struct virtqueue *_vq, if (out) vq->notify(&vq->vq); END_USE(vq); + torture_done(data); return -ENOSPC; } @@ -341,7 +510,7 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq) new = vq->vring.avail->idx; vq->num_added = 0; -#ifdef DEBUG +#ifdef CONFIG_VIRTIO_TORTURE if (vq->last_add_time_valid) { WARN_ON(ktime_to_ms(ktime_sub(ktime_get(), vq->last_add_time)) > 100); @@ -474,6 +643,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len) return NULL; } +#ifdef CONFIG_VIRTIO_TORTURE + if (unlikely(tot_inlen(vq, i) < *len)) { + BAD_RING(vq, "id %u: %u > %u used!\n", + i, *len, tot_inlen(vq, i)); + return NULL; + } +#endif + /* detach_buf clears data, so grab it now. */ ret = vq->data[i]; detach_buf(vq, i); @@ -486,12 +663,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len) virtio_mb(vq); } -#ifdef DEBUG +#ifdef CONFIG_VIRTIO_TORTURE vq->last_add_time_valid = false; + BUG_ON(*len > + #endif END_USE(vq); - return ret; + return torture_done(ret); } EXPORT_SYMBOL_GPL(virtqueue_get_buf); @@ -683,7 +862,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int index, vq->last_used_idx = 0; vq->num_added = 0; list_add_tail(&vq->vq.list, &vdev->vqs); -#ifdef DEBUG +#ifdef CONFIG_VIRTIO_TORTURE vq->in_use = false; vq->last_add_time_valid = false; #endif _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
