virtio-net: inline header support

Capacity math on ring full is wrong: we are looking at num_sg but that might be optimistic because of indirect buffer use.

The implementation also penalizes fast path with extra memory accesses for the benefit of ring full condition handling which is slow path.

It's easy to query ring capacity so let's do just that.

Thinking about Sasha's patches, we can reduce ring usage for virtio net small packets dramatically if we put virtio net header inline with the data.
This can be done for free in case guest net stack allocated extra head room for the packet, and I don't see why would this have any downsides.

Even though with my recent patches qemu
no longer requires header to be the first s/g element, we need a new feature bit to detect this.
A trivial qemu patch will be sent separately.

Quoted 6 lines · Michael S. Tsirkin

Thinking about Sasha's patches, we can reduce ring usage for virtio net small packets dramatically if we put virtio net header inline with the data.
This can be done for free in case guest net stack allocated extra head room for the packet, and I don't see why would this have any downsides.

I've been wanting to do this for the longest time... but...

Quoted 2 lines

Even though with my recent patches qemu
no longer requires header to be the first s/g element,

Quoted 6 lines · Michael S. Tsirkin

Thinking about Sasha's patches, we can reduce ring usage for virtio net small packets dramatically if we put virtio net header inline with the data.
This can be done for free in case guest net stack allocated extra head room for the packet, and I don't see why would this have any downsides.

I've been wanting to do this for the longest time... but...

Quoted 4 lines

Even though with my recent patches qemu
no longer requires header to be the first s/g element, we need a new feature bit to detect this.
A trivial qemu patch will be sent separately.

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

Quoted 11 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin

Thinking about Sasha's patches, we can reduce ring usage for virtio net small packets dramatically if we put virtio net header inline with the data.
This can be done for free in case guest net stack allocated extra head room for the packet, and I don't see why would this have any downsides.

I've been wanting to do this for the longest time... but...

Quoted 8 lines

Quoted 4 lines

Even though with my recent patches qemu
no longer requires header to be the first s/g element, we need a new feature bit to detect this.
A trivial qemu patch will be sent separately.

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

This is a bug in the specification.

The QEMU implementation pre-dates the specification. All of the actual implementations of virtio relied on the semantics of s/g elements and still do.

What's in the specification really doesn't matter when it doesn't agree with all of the existing implementations.

Users use implementations, not specifications. The specification really ought to be changed here.

Quoted 24 lines · Anthony Liguori

Quoted 11 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin

Thinking about Sasha's patches, we can reduce ring usage for virtio net small packets dramatically if we put virtio net header inline with the data.
This can be done for free in case guest net stack allocated extra head room for the packet, and I don't see why would this have any downsides.

I've been wanting to do this for the longest time... but...

Quoted 8 lines

Quoted 4 lines

Even though with my recent patches qemu
no longer requires header to be the first s/g element, we need a new feature bit to detect this.
A trivial qemu patch will be sent separately.

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

This is a bug in the specification.

Quoted 3 lines

The QEMU implementation pre-dates the specification. All of the actual implementations of virtio relied on the semantics of s/g elements and still do.

lguest fix is pending in my queue. lkvm and qemu are broken; lkvm isn't ever going to be merged, so I'm not sure what its status is? But I'm determined to fix qemu, and hence my torture patch to make sure this doesn't creep in again.

Quoted 5 lines

What's in the specification really doesn't matter when it doesn't agree with all of the existing implementations.

Users use implementations, not specifications. The specification really ought to be changed here.

I'm sorely tempted, except that we're losing a real optimization because of this :(

The specification has long contained the footnote:

The current qemu device implementations mistakenly insist that the first descriptor cover the header in these cases exactly, so a cautious driver should arrange it so.

I'd like to tie this caveat to the PCI capability change, so this note will move to the appendix with the old PCI layout.

Cheers,
Rusty.

Quoted 4 lines

lguest fix is pending in my queue. lkvm and qemu are broken; lkvm isn't ever going to be merged, so I'm not sure what its status is? But I'm determined to fix qemu, and hence my torture patch to make sure this doesn't creep in again.

There are even more implementations out there and I'd wager they all rely on framing.

Quoted 24 lines · Anthony Liguori

Quoted 11 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin

Thinking about Sasha's patches, we can reduce ring usage for virtio net small packets dramatically if we put virtio net header inline with the data.
This can be done for free in case guest net stack allocated extra head room for the packet, and I don't see why would this have any downsides.

I've been wanting to do this for the longest time... but...

Quoted 8 lines

Quoted 4 lines

Even though with my recent patches qemu
no longer requires header to be the first s/g element, we need a new feature bit to detect this.
A trivial qemu patch will be sent separately.

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

This is a bug in the specification.

Quoted 3 lines

The QEMU implementation pre-dates the specification. All of the actual implementations of virtio relied on the semantics of s/g elements and still do.

lguest fix is pending in my queue. lkvm and qemu are broken; lkvm isn't ever going to be merged, so I'm not sure what its status is? But I'm determined to fix qemu, and hence my torture patch to make sure this doesn't creep in again.

"Michael S. Tsirkin" <[email protected]> writes:

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

So my plan was to tie this assumption to the new PCI layout. And have a stress-testing patch like the one below in the kernel (see my virtio-wip branch for stuff like this). Turn it on at boot with "virtio_ring.torture" on the kernel commandline.

BTW, I've fixed lguest, but my kvm here (Ubuntu precise, kvm-qemu 1.0) is too old. Building the latest git now...

Cheers,
Rusty.

Subject: 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]>

drivers/virtio/Kconfig +9 -0

index 8d5bddb..930a4ea 100644
@@ -5,6 +5,15 @@ config VIRTIO
 	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST,
 	  CONFIG_RPMSG or CONFIG_S390_GUEST.
 
+config VIRTIO_DEVICE_TORTURE
+	bool "Virtio device torture tests"
+	depends on VIRTIO && DEBUG_KERNEL
+	help
+	  This makes the virtio_ring implementation creatively change
+	  the format of requests to make sure that devices are
+	  properly implemented.  This will make your virtual machine
+	  slow *and* unreliable!  Say N.
+
 menu "Virtio drivers"
 
 config VIRTIO_PCI

drivers/virtio/virtio_ring.c +148 -2

index e639584..8893753 100644
@@ -124,6 +124,149 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
+#ifdef CONFIG_VIRTIO_DEVICE_TORTURE
+static bool torture;
+module_param(torture, bool, 0644);
+
+struct torture {
+	unsigned int orig_out, orig_in;
+	void *orig_data;
+	struct scatterlist sg[4];
+	struct scatterlist orig_sg[];
+};
+
+static size_t tot_len(struct scatterlist sg[], unsigned num)
+{
+	size_t 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;
+	size_t outlen, inlen, ourseed, len1;
+	void *buf;
+
+	if (!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 (!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;
+}
+
+#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;
+}
+#endif /* CONFIG_VIRTIO_DEVICE_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[],

@@ -213,6 +356,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 
 	BUG_ON(data == NULL);
 
+	if (!torture_replace(&sg, &out, &in, &data, gfp))
+		return -ENOMEM;
+
 #ifdef DEBUG
 	{
 		ktime_t now = ktime_get();

@@ -246,6 +392,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 		if (out)
 			vq->notify(&vq->vq);
 		END_USE(vq);
+		torture_done(data);
 		return -ENOSPC;
 	}
 

@@ -476,7 +623,7 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 #endif
 
 	END_USE(vq);
-	return ret;
+	return torture_done(ret);
 }
 EXPORT_SYMBOL_GPL(virtqueue_get_buf);
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 17 lines · Rusty Russell

"Michael S. Tsirkin" <[email protected]> writes:

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

So my plan was to tie this assumption to the new PCI layout. And have a stress-testing patch like the one below in the kernel (see my virtio-wip branch for stuff like this). Turn it on at boot with "virtio_ring.torture" on the kernel commandline.

BTW, I've fixed lguest, but my kvm here (Ubuntu precise, kvm-qemu 1.0) is too old. Building the latest git now...

Cheers,
Rusty.

Quoted 7 lines

Subject: 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]>

Ignore framing is really a bad idea. You want backends to enforce reasonable framing because guest's shouldn't do silly things with framing.

For instance, with virtio-blk, if you want decent performance, you absolutely want to avoid bouncing the data. If you're using O_DIRECT in the host to submit I/O requests, then it's critical that all of the s/g elements are aligned to a sector boundary and sized to a sector boundary.

Yes, QEMU can handle if that's not the case, but it would be insanely stupid for a guest not to do this. This is the sort of thing that ought to be enforced in the specification because a guest cannot perform well if it doesn't follow these rules.

A spec isn't terribly useful if the result is guest drivers that are slow. There's very little to gain by not enforcing rules around framing and there's a lot to lose if a guest frames incorrectly.

In the rare case where we want to make a framing change, we should use feature bits like Michael is proposing.

In this case, we should simply say that with the feature bit, the vnet header can be in the same element as the data but not allow the header to be spread across multiple elements.

Quoted 6 lines · Michael S. Tsirkin

Thinking about Sasha's patches, we can reduce ring usage for virtio net small packets dramatically if we put virtio net header inline with the data.
This can be done for free in case guest net stack allocated extra head room for the packet, and I don't see why would this have any downsides.

I've been wanting to do this for the longest time... but...

Quoted 4 lines

Even though with my recent patches qemu
no longer requires header to be the first s/g element, we need a new feature bit to detect this.
A trivial qemu patch will be sent separately.

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

So my plan was to tie this assumption to the new PCI layout.

Thinking about Sasha's patches, we can reduce ring usage for virtio net small packets dramatically if we put virtio net header inline with the data.
This can be done for free in case guest net stack allocated extra head room for the packet, and I don't see why would this have any downsides.

Even though with my recent patches qemu
no longer requires header to be the first s/g element, we need a new feature bit to detect this.
A trivial qemu patch will be sent separately.

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

Il 03/10/2012 08:44, Rusty Russell ha scritto:

Quoted 3 lines

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

However, this bug affects (almost) all implementations and (almost) all devices. It even makes sense to reserve a transport feature bit for it instead of a device feature bit.

Paolo

Quoted 7 lines

Quoted 3 lines

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

However, this bug affects (almost) all implementations and (almost) all devices. It even makes sense to reserve a transport feature bit for it instead of a device feature bit.

Perhaps, but we have to fix the bugs first!

As I said, my torture patch broke qemu immediately. Since noone has leapt onto fixing that, I'll take a look now...

Il 04/10/2012 02:11, Rusty Russell ha scritto:

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

However, this bug affects (almost) all implementations and (almost) all devices. It even makes sense to reserve a transport feature bit for it instead of a device feature bit.

Quoted 1 line

Perhaps, but we have to fix the bugs first!

Yes. :) Isn't that what mst's patch does?

Quoted 2 lines

As I said, my torture patch broke qemu immediately. Since noone has leapt onto fixing that, I'll take a look now...

I can look at virtio-scsi.

Quoted 13 lines · Paolo Bonzini

Il 04/10/2012 02:11, Rusty Russell ha scritto:

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

However, this bug affects (almost) all implementations and (almost) all devices. It even makes sense to reserve a transport feature bit for it instead of a device feature bit.

Perhaps, but we have to fix the bugs first!

Yes. :) Isn't that what mst's patch does?

Quoted 4 lines

Quoted 2 lines

As I said, my torture patch broke qemu immediately. Since noone has leapt onto fixing that, I'll take a look now...

I can look at virtio-scsi.

Actually, you can't, see my reply to Anthony...

Message-ID: <[email protected]>

Il 04/10/2012 14:51, Rusty Russell ha scritto:

Quoted 15 lines

Quoted 13 lines · Paolo Bonzini

Il 04/10/2012 02:11, Rusty Russell ha scritto:

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

However, this bug affects (almost) all implementations and (almost) all devices. It even makes sense to reserve a transport feature bit for it instead of a device feature bit.

Perhaps, but we have to fix the bugs first!

Yes. :) Isn't that what mst's patch does?

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

As I said, my torture patch broke qemu immediately. Since noone has leapt onto fixing that, I'll take a look now...

I can look at virtio-scsi.

Actually, you can't, see my reply to Anthony...

Message-ID: <[email protected]>

    struct virtio_scsi_req_cmd {
        // Read-only
        u8 lun[8];
        u64 id;
        u8 task_attr;
        u8 prio;
        u8 crn;
        char cdb[cdb_size];
        char dataout[];
        // Write-only part
        u32 sense_len;
        u32 residual;
        u16 status_qualifier;
        u8 status;
        u8 response;
        u8 sense[sense_size];
        char datain[];
    };

where cdb_size and sense_size come from configuration space. The device right now expects everything before dataout/datain to be in a single descriptor, but that's in no way part of the spec. Am I missing something egregious?

Il 04/10/2012 14:51, Rusty Russell ha scritto:

Quoted 15 lines

Quoted 13 lines · Paolo Bonzini

Il 04/10/2012 02:11, Rusty Russell ha scritto:

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

There's a reason I haven't done this. I really, really dislike "my implemention isn't broken" feature bits. We could have an infinite number of them, for each bug in each device.

However, this bug affects (almost) all implementations and (almost) all devices. It even makes sense to reserve a transport feature bit for it instead of a device feature bit.

Perhaps, but we have to fix the bugs first!

Yes. :) Isn't that what mst's patch does?

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

As I said, my torture patch broke qemu immediately. Since noone has leapt onto fixing that, I'll take a look now...

I can look at virtio-scsi.

Actually, you can't, see my reply to Anthony...

Message-ID: <[email protected]>

    struct virtio_scsi_req_cmd {
        // Read-only
        u8 lun[8];
        u64 id;
        u8 task_attr;
        u8 prio;
        u8 crn;
        char cdb[cdb_size];
        char dataout[];
        // Write-only part
        u32 sense_len;
        u32 residual;
        u16 status_qualifier;
        u8 status;
        u8 response;
        u8 sense[sense_size];
        char datain[];
    };

where cdb_size and sense_size come from configuration space. The device right now expects everything before dataout/datain to be in a single descriptor, but that's in no way part of the spec. Am I missing something egregious?

Quoted 19 lines

Quoted 19 lines

    struct virtio_scsi_req_cmd {
        // Read-only
        u8 lun[8];
        u64 id;
        u8 task_attr;
        u8 prio;
        u8 crn;
        char cdb[cdb_size];
        char dataout[];
        // Write-only part
        u32 sense_len;
        u32 residual;
        u16 status_qualifier;
        u8 status;
        u8 response;
        u8 sense[sense_size];
        char datain[];
    };

Quoted 4 lines

Quoted 4 lines

where cdb_size and sense_size come from configuration space. The device right now expects everything before dataout/datain to be in a single descriptor, but that's in no way part of the spec. Am I missing something egregious?

Since you wrote it, I hope not :)

That's good. But virtio_blk's scsi command is insoluble AFAICT. As I said to Anthony, the best rules are "always" and "never", so I'd really rather not have to grandfather that in.

Il 05/10/2012 07:43, Rusty Russell ha scritto:

Quoted 3 lines

That's good. But virtio_blk's scsi command is insoluble AFAICT. As I said to Anthony, the best rules are "always" and "never", so I'd really rather not have to grandfather that in.

It is, but we can add a rule that if the (transport) flag VIRTIO_RING_F_ANY_HEADER_SG is set, the cdb field is always 32 bytes in virtio-blk.

Quoted 5 lines · Paolo Bonzini

Il 05/10/2012 07:43, Rusty Russell ha scritto:

Quoted 3 lines

That's good. But virtio_blk's scsi command is insoluble AFAICT. As I said to Anthony, the best rules are "always" and "never", so I'd really rather not have to grandfather that in.

Quoted 3 lines

It is, but we can add a rule that if the (transport) flag VIRTIO_RING_F_ANY_HEADER_SG is set, the cdb field is always 32 bytes in virtio-blk.

Could we do that? It's the cmd length I'm concerned about; is it always 32 in practice for some reason?

Currently qemu does:

    struct sg_io_hdr hdr;
    memset(&hdr, 0, sizeof(struct sg_io_hdr));
    hdr.interface_id = 'S';
    hdr.cmd_len = req->elem.out_sg[1].iov_len;
    hdr.cmdp = req->elem.out_sg[1].iov_base;
    hdr.dxfer_len = 0;

If it's a command which expects more output data, there's no way to guess where the boundary is between that command and the data.

Il 09/10/2012 06:59, Rusty Russell ha scritto:

Quoted 6 lines

Quoted 5 lines · Paolo Bonzini

Il 05/10/2012 07:43, Rusty Russell ha scritto:

Quoted 3 lines

That's good. But virtio_blk's scsi command is insoluble AFAICT. As I said to Anthony, the best rules are "always" and "never", so I'd really rather not have to grandfather that in.

Quoted 6 lines

Quoted 3 lines

It is, but we can add a rule that if the (transport) flag VIRTIO_RING_F_ANY_HEADER_SG is set, the cdb field is always 32 bytes in virtio-blk.

Could we do that? It's the cmd length I'm concerned about; is it always 32 in practice for some reason?

It is always 32 or less except in very obscure cases that are pretty much confined to iSCSI. We don't care about the obscure cases, and the extra bytes don't hurt.

BTW, 32 is the default cdb_size used by virtio-scsi.

Quoted 9 lines

Currently qemu does:

    struct sg_io_hdr hdr;
    memset(&hdr, 0, sizeof(struct sg_io_hdr));
    hdr.interface_id = 'S';
    hdr.cmd_len = req->elem.out_sg[1].iov_len;
    hdr.cmdp = req->elem.out_sg[1].iov_base;
    hdr.dxfer_len = 0;

Quoted 2 lines

If it's a command which expects more output data, there's no way to guess where the boundary is between that command and the data.

Yep, so I understood the problem right.

Quoted 29 lines · Paolo Bonzini

Il 09/10/2012 06:59, Rusty Russell ha scritto:

Quoted 6 lines

Quoted 5 lines · Paolo Bonzini

Il 05/10/2012 07:43, Rusty Russell ha scritto:

Quoted 3 lines

That's good. But virtio_blk's scsi command is insoluble AFAICT. As I said to Anthony, the best rules are "always" and "never", so I'd really rather not have to grandfather that in.

Quoted 6 lines

Quoted 3 lines

It is, but we can add a rule that if the (transport) flag VIRTIO_RING_F_ANY_HEADER_SG is set, the cdb field is always 32 bytes in virtio-blk.

Could we do that? It's the cmd length I'm concerned about; is it always 32 in practice for some reason?

It is always 32 or less except in very obscure cases that are pretty much confined to iSCSI. We don't care about the obscure cases, and the extra bytes don't hurt.

BTW, 32 is the default cdb_size used by virtio-scsi.

Quoted 9 lines

Currently qemu does:

    struct sg_io_hdr hdr;
    memset(&hdr, 0, sizeof(struct sg_io_hdr));
    hdr.interface_id = 'S';
    hdr.cmd_len = req->elem.out_sg[1].iov_len;
    hdr.cmdp = req->elem.out_sg[1].iov_base;
    hdr.dxfer_len = 0;

Quoted 4 lines

Quoted 2 lines

If it's a command which expects more output data, there's no way to guess where the boundary is between that command and the data.

Yep, so I understood the problem right.

OK. Well, Anthony wants qemu to be robust in this regard, so I am tempted to rework all the qemu drivers to handle arbitrary layouts. They could use a good audit anyway.

This would become a glaring exception, but I'm tempted to fix it to 32 bytes at the same time as we get the new pci layout (ie. for the virtio 1.0 spec).

The Linux driver would carefully be backwards compatible, of course, and the spec would document why.

Cheers,
Rusty.

Quoted 3 lines · Rusty Russell

OK. Well, Anthony wants qemu to be robust in this regard, so I am tempted to rework all the qemu drivers to handle arbitrary layouts. They could use a good audit anyway.

I agree here. Still trying to understand whether we can agree to use a feature bit for this, or not.

Quoted 3 lines

This would become a glaring exception, but I'm tempted to fix it to 32 bytes at the same time as we get the new pci layout (ie. for the virtio 1.0 spec).

But this isn't a virtio-pci only issue, is it?
qemu has s390 bus with same limmitation.
How can we tie it to pci layout?

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell

OK. Well, Anthony wants qemu to be robust in this regard, so I am tempted to rework all the qemu drivers to handle arbitrary layouts. They could use a good audit anyway.

Quoted 2 lines

I agree here. Still trying to understand whether we can agree to use a feature bit for this, or not.

I'd *like* to imply it by the new PCI layout, but if it doesn't work we'll add a new feature bit.

I'm resisting a feature bit, since it constrains future implementations which could otherwise assume it.

Quoted 7 lines

Quoted 3 lines

This would become a glaring exception, but I'm tempted to fix it to 32 bytes at the same time as we get the new pci layout (ie. for the virtio 1.0 spec).

But this isn't a virtio-pci only issue, is it?
qemu has s390 bus with same limmitation.
How can we tie it to pci layout?

They can use a transport feature if they need to, of course. But perhaps the timing with ccw will coincide with the fix, in which they don't need to, but it might be a bit late.

Cornelia?

Cheers,
Rusty.

Quoted 7 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell

OK. Well, Anthony wants qemu to be robust in this regard, so I am tempted to rework all the qemu drivers to handle arbitrary layouts. They could use a good audit anyway.

I agree here. Still trying to understand whether we can agree to use a feature bit for this, or not.

I'd *like* to imply it by the new PCI layout, but if it doesn't work we'll add a new feature bit.

I'm resisting a feature bit, since it constrains future implementations which could otherwise assume it.

Quoted 7 lines

Quoted 3 lines

This would become a glaring exception, but I'm tempted to fix it to 32 bytes at the same time as we get the new pci layout (ie. for the virtio 1.0 spec).

But this isn't a virtio-pci only issue, is it?
qemu has s390 bus with same limmitation.
How can we tie it to pci layout?

They can use a transport feature if they need to, of course. But perhaps the timing with ccw will coincide with the fix, in which they don't need to, but it might be a bit late.

Cornelia?

Cheers,
Rusty.