virtio with config abstraction and ring implementation

These helper routines supply most of the virtqueue_ops for hypervisors which want to use a ring for virtio. Unlike the previous lguest implementation:

3) The page numbers are always 64 bit (PAE anyone?)

Quoted 6 lines · Rusty Russell

These helper routines supply most of the virtqueue_ops for hypervisors which want to use a ring for virtio. Unlike the previous lguest implementation:

3) The page numbers are always 64 bit (PAE anyone?)

32 bits of page numbers give 44 bits of physical address on x86. That's 16TB per guest. Admittedly it's smaller on a VAX.

I don't like the chaining and would prefer the descriptor to refer to an array of page descriptors. However I can't complain since this isn't common code.

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Rusty Russell

These helper routines supply most of the virtqueue_ops for hypervisors which want to use a ring for virtio. Unlike the previous lguest implementation:

3) The page numbers are always 64 bit (PAE anyone?)

Quoted 3 lines

32 bits of page numbers give 44 bits of physical address on x86. That's 16TB per guest. Admittedly it's smaller on a VAX.

I like to feel that I make these mistakes to ensure others are paying attention. However, it does mean that I can just put an address in there and increase the length field to 32 bits. Much rejoicing.

Quoted 4 lines

I don't like the chaining and would prefer the descriptor to refer to an array of page descriptors. However I can't complain since this isn't common code.

The intent is for kvm to use it. I'll certainly consider your patches, although I suspect that managing descriptor pages will make things ugly enough to cause you to reconsider.

Quoted 8 lines · Rusty Russell

Quoted 2 lines · Avi Kivity

32 bits of page numbers give 44 bits of physical address on x86. That's 16TB per guest. Admittedly it's smaller on a VAX.

I like to feel that I make these mistakes to ensure others are paying attention. However, it does mean that I can just put an address in there and increase the length field to 32 bits. Much rejoicing.

Why are we sending page numbers anyway? See below.

Where one of the flags is VRING_DESC_INDIRECT, which means that the memory within (address, length) is a bunch of descriptors instead of raw data.

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Rusty Russell

Quoted 3 lines · Avi Kivity

32 bits of page numbers give 44 bits of physical address on x86. That's 16TB per guest. Admittedly it's smaller on a VAX.

I like to feel that I make these mistakes to ensure others are paying attention. However, it does mean that I can just put an address in there and increase the length field to 32 bits. Much rejoicing.

Quoted 2 lines

Why are we sending page numbers anyway? See below.

Perhaps I was unclear. I already changed to a 64-bit address. I haven't send out another set of patches because I'm changing to Arnd's explicit virtio bus too. Will send out a new set tomorrow at this rate.

Quoted 4 lines

Where one of the flags is VRING_DESC_INDIRECT, which means that the memory within (address, length) is a bunch of descriptors instead of raw data.

If that's all we wanted, it's fairly easy to do as a future extension even if we didn't change it today. My concern was the allocation and management of those sg pages; hence my desire for a patch 8)

Quoted 11 lines · Avi Kivity

Quoted 9 lines · Rusty Russell

Quoted 2 lines · Avi Kivity

32 bits of page numbers give 44 bits of physical address on x86. That's 16TB per guest. Admittedly it's smaller on a VAX.

I like to feel that I make these mistakes to ensure others are paying attention. However, it does mean that I can just put an address in there and increase the length field to 32 bits. Much rejoicing.

Quoted 2 lines

Why are we sending page numbers anyway? See below.

Perhaps I was unclear. I already changed to a 64-bit address. I haven't send out another set of patches because I'm changing to Arnd's explicit virtio bus too. Will send out a new set tomorrow at this rate.

Quoted 2 lines

Where one of the flags is VRING_DESC_INDIRECT, which means that the memory within (address, length) is a bunch of descriptors instead of raw data.

If that's all we wanted, it's fairly easy to do as a future extension even if we didn't change it today. My concern was the allocation and management of those sg pages; hence my desire for a patch 8)

Quoted 8 lines · Rusty Russell

Quoted 2 lines · Avi Kivity

32 bits of page numbers give 44 bits of physical address on x86. That's 16TB per guest. Admittedly it's smaller on a VAX.

I like to feel that I make these mistakes to ensure others are paying attention. However, it does mean that I can just put an address in there and increase the length field to 32 bits. Much rejoicing.

Why are we sending page numbers anyway? See below.

Where one of the flags is VRING_DESC_INDIRECT, which means that the memory within (address, length) is a bunch of descriptors instead of raw data.

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Rusty Russell

These helper routines supply most of the virtqueue_ops for hypervisors which want to use a ring for virtio. Unlike the previous lguest implementation:

3) The page numbers are always 64 bit (PAE anyone?)

Quoted 2 lines

32 bits of page numbers give 44 bits of physical address on x86. That's 16TB per guest. Admittedly it's smaller on a VAX.

I like to feel that I make these mistakes to ensure others are paying attention. However, it does mean that I can just put an address in there and increase the length field to 32 bits. Much rejoicing.

Quoted 2 lines

I don't like the chaining and would prefer the descriptor to refer to an array of page descriptors. However I can't complain since this isn't common code.

The intent is for kvm to use it. I'll certainly consider your patches, although I suspect that managing descriptor pages will make things ugly enough to cause you to reconsider.

Quoted 6 lines · Rusty Russell

These helper routines supply most of the virtqueue_ops for hypervisors which want to use a ring for virtio. Unlike the previous lguest implementation:

3) The page numbers are always 64 bit (PAE anyone?)

32 bits of page numbers give 44 bits of physical address on x86. That's 16TB per guest. Admittedly it's smaller on a VAX.

I don't like the chaining and would prefer the descriptor to refer to an array of page descriptors. However I can't complain since this isn't common code.

These helper routines supply most of the virtqueue_ops for hypervisors which want to use a ring for virtio. Unlike the previous lguest implementation:

3) The page numbers are always 64 bit (PAE anyone?)

The block driver uses scatter-gather lists with sg[0] being the request information (struct virtio_blk_outhdr) with the type, sector and inbuf id. The next N sg entries are the bio itself, then the last sg is the status byte. Whether the N entries are in or out depends on whether it's a read or a write.

We accept the normal (SCSI) ioctls: they get handed through to the other side which can then handle it or reply that it's unsupported. It's not clear that this actually works in general, since I don't know if blk_pc_request() requests have an accurate rq_data_dir().

Although we try to reply -ENOTTY on unsupported commands, the block layer in its infinite wisdom suppressed the error so ioctl(fd, CDROMEJECT) returns success to userspace.

Quoted 6 lines · Rusty Russell

The block driver uses scatter-gather lists with sg[0] being the request information (struct virtio_blk_outhdr) with the type, sector and inbuf id. The next N sg entries are the bio itself, then the last sg is the status byte. Whether the N entries are in or out depends on whether it's a read or a write.

Quoted 4 lines

We accept the normal (SCSI) ioctls: they get handed through to the other side which can then handle it or reply that it's unsupported. It's not clear that this actually works in general, since I don't know if blk_pc_request() requests have an accurate rq_data_dir().

They should, if they imply a data transfer.

Quoted 3 lines

Although we try to reply -ENOTTY on unsupported commands, the block layer in its infinite wisdom suppressed the error so ioctl(fd, CDROMEJECT) returns success to userspace.

How about ever submitting a patch for that, instead of just repeatedly complaining about it?

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Rusty Russell

The block driver uses scatter-gather lists with sg[0] being the request information (struct virtio_blk_outhdr) with the type, sector and inbuf id. The next N sg entries are the bio itself, then the last sg is the status byte. Whether the N entries are in or out depends on whether it's a read or a write.

We accept the normal (SCSI) ioctls: they get handed through to the other side which can then handle it or reply that it's unsupported. It's not clear that this actually works in general, since I don't know if blk_pc_request() requests have an accurate rq_data_dir().

Quoted 1 line

They should, if they imply a data transfer.

OK, good. We rely on that to mark input vs output sg elements. I was wondering if there was some weird requests which did both input and output.

Quoted 6 lines

Quoted 3 lines

Although we try to reply -ENOTTY on unsupported commands, the block layer in its infinite wisdom suppressed the error so ioctl(fd, CDROMEJECT) returns success to userspace.

How about ever submitting a patch for that, instead of just repeatedly complaining about it?

To be fair, I've left the complaint in that same patch, you're just reading it repeatedly 8)

I shall look through the code and see if I can figure out how to fix it. I'm assuming from your response that there's not some strange reason to preserve current behaviour.

Quoted 3 lines · Rusty Russell

I shall look through the code and see if I can figure out how to fix it. I'm assuming from your response that there's not some strange reason to preserve current behaviour.

It surely sounds like a bug, if you issue ioctl(fd, CDROMEJECT), the driver sees it and returns -ENOTTY, but userland sees a 0 retval. So if you have time, please do poke at it a bit.

Quoted 3 lines · Rusty Russell

I shall look through the code and see if I can figure out how to fix it. I'm assuming from your response that there's not some strange reason to preserve current behaviour.

It surely sounds like a bug, if you issue ioctl(fd, CDROMEJECT), the driver sees it and returns -ENOTTY, but userland sees a 0 retval. So if you have time, please do poke at it a bit.

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Rusty Russell

The block driver uses scatter-gather lists with sg[0] being the request information (struct virtio_blk_outhdr) with the type, sector and inbuf id. The next N sg entries are the bio itself, then the last sg is the status byte. Whether the N entries are in or out depends on whether it's a read or a write.

We accept the normal (SCSI) ioctls: they get handed through to the other side which can then handle it or reply that it's unsupported. It's not clear that this actually works in general, since I don't know if blk_pc_request() requests have an accurate rq_data_dir().

Quoted 1 line

They should, if they imply a data transfer.

OK, good. We rely on that to mark input vs output sg elements. I was wondering if there was some weird requests which did both input and output.

Quoted 6 lines

Quoted 3 lines

Although we try to reply -ENOTTY on unsupported commands, the block layer in its infinite wisdom suppressed the error so ioctl(fd, CDROMEJECT) returns success to userspace.

How about ever submitting a patch for that, instead of just repeatedly complaining about it?

To be fair, I've left the complaint in that same patch, you're just reading it repeatedly 8)

I shall look through the code and see if I can figure out how to fix it. I'm assuming from your response that there's not some strange reason to preserve current behaviour.

Quoted 6 lines · Rusty Russell

The block driver uses scatter-gather lists with sg[0] being the request information (struct virtio_blk_outhdr) with the type, sector and inbuf id. The next N sg entries are the bio itself, then the last sg is the status byte. Whether the N entries are in or out depends on whether it's a read or a write.

Quoted 4 lines

We accept the normal (SCSI) ioctls: they get handed through to the other side which can then handle it or reply that it's unsupported. It's not clear that this actually works in general, since I don't know if blk_pc_request() requests have an accurate rq_data_dir().

They should, if they imply a data transfer.

Quoted 3 lines

Although we try to reply -ENOTTY on unsupported commands, the block layer in its infinite wisdom suppressed the error so ioctl(fd, CDROMEJECT) returns success to userspace.

How about ever submitting a patch for that, instead of just repeatedly complaining about it?

(quoted diff) +6 -0

+static void end_dequeued_request(struct request *req,
+				 struct request_queue *q, int uptodate)
+{
+	/* And so the insanity of the block layer infects us here. */
+	int nsectors = req->hard_nr_sectors;
+

Quoted 10 lines · Rusty Russell

(quoted diff) +10 -0

+static void end_dequeued_request(struct request *req,
+				 struct request_queue *q, int uptodate)
+{
+	/* And so the insanity of the block layer infects us here. */
+	int nsectors = req->hard_nr_sectors;
+
+	if (blk_pc_request(req)) {
+		nsectors = (req->data_len + 511) >> 9;
+		if (!nsectors)
+			nsectors = 1;

Review

Quoted 6 lines

(quoted diff) +6 -0

+	}
+	if (end_that_request_first(req, uptodate, nsectors))
+		BUG();
+	add_disk_randomness(req->rq_disk);
+	end_that_request_last(req, uptodate);
+}

We have end_queued_request(), lets add end_dequeued_request(). Below.

Review

Quoted 4 lines

(quoted diff) +4 -0

+	vblk->sg[0].page = virt_to_page(&vbr->out_hdr);
+	vblk->sg[0].offset = offset_in_page(&vbr->out_hdr);
+	vblk->sg[0].length = sizeof(vbr->out_hdr);
+	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);

This wont work for chained sglists, but I gather (I'm so funny) that it wont be an issue for you. How large are your sglists?

Review

Quoted 5 lines

(quoted diff) +5 -0

+		if (!do_req(q, vblk, req)) {
+			/* Queue full?  Wait. */
+			blk_stop_queue(q);
+			break;
+		}

Personally I think this bool stuff is foul. You return false/true, but still use ! to test. That is just more confusing than the canonical 0/1 good/bad return imho.

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Rusty Russell

(quoted diff) +10 -0

+static void end_dequeued_request(struct request *req,
+				 struct request_queue *q, int uptodate)
+{
+	/* And so the insanity of the block layer infects us here. */
+	int nsectors = req->hard_nr_sectors;
+
+	if (blk_pc_request(req)) {
+		nsectors = (req->data_len + 511) >> 9;
+		if (!nsectors)
+			nsectors = 1;

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+	}
+	if (end_that_request_first(req, uptodate, nsectors))
+		BUG();
+	add_disk_randomness(req->rq_disk);
+	end_that_request_last(req, uptodate);
+}

We have end_queued_request(), lets add end_dequeued_request(). Below.

OK, thanks, I'll throw that in the mix and test...

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	vblk->sg[0].page = virt_to_page(&vbr->out_hdr);
+	vblk->sg[0].offset = offset_in_page(&vbr->out_hdr);
+	vblk->sg[0].length = sizeof(vbr->out_hdr);
+	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);

This wont work for chained sglists, but I gather (I'm so funny) that it wont be an issue for you. How large are your sglists?

Hmm, potentially extremely large. What do I need to do for chained sglists?

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		if (!do_req(q, vblk, req)) {
+			/* Queue full?  Wait. */
+			blk_stop_queue(q);
+			break;
+		}

Personally I think this bool stuff is foul. You return false/true, but still use ! to test. That is just more confusing than the canonical 0/1 good/bad return imho.

Except "0/1" is not canonical in the kernel. Arguably, "-errno/0" is canonical. OTOH, bool is clear.

if do_req() fails, we assume the queue is full. I shall change the comment to that effect.

Quoted 4 lines · Rusty Russell

Quoted 1 line · Jens Axboe

We have end_queued_request(), lets add end_dequeued_request(). Below.

OK, thanks, I'll throw that in the mix and test...

I've queued it for 2.6.24 as well, so should be in mainline in that time frame.

Quoted 10 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	vblk->sg[0].page = virt_to_page(&vbr->out_hdr);
+	vblk->sg[0].offset = offset_in_page(&vbr->out_hdr);
+	vblk->sg[0].length = sizeof(vbr->out_hdr);
+	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);

This wont work for chained sglists, but I gather (I'm so funny) that it wont be an issue for you. How large are your sglists?

Hmm, potentially extremely large. What do I need to do for chained sglists?

Not a lot, actually. You snipped the problematic part in your reply, though:

        vblk->sg[num+1].page = virt_to_page(&vbr->in_hdr);

which assumes that sg is a contigious piece of memory, for chained sg lists that isn't true. sg chaining will be in 2.6.24, so if you really do need large sglists, then that's the way to go.

Quoted 2 lines

Except "0/1" is not canonical in the kernel. Arguably, "-errno/0" is canonical. OTOH, bool is clear.

-errno/0, then. 1 is typically used for 'failure without specific error number' when -Exx doesn't apply. Like the above :-)

Quoted 4 lines · Rusty Russell

Quoted 1 line · Jens Axboe

We have end_queued_request(), lets add end_dequeued_request(). Below.

OK, thanks, I'll throw that in the mix and test...

I've queued it for 2.6.24 as well, so should be in mainline in that time frame.

Quoted 10 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	vblk->sg[0].page = virt_to_page(&vbr->out_hdr);
+	vblk->sg[0].offset = offset_in_page(&vbr->out_hdr);
+	vblk->sg[0].length = sizeof(vbr->out_hdr);
+	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);

This wont work for chained sglists, but I gather (I'm so funny) that it wont be an issue for you. How large are your sglists?

Hmm, potentially extremely large. What do I need to do for chained sglists?

Not a lot, actually. You snipped the problematic part in your reply, though:

        vblk->sg[num+1].page = virt_to_page(&vbr->in_hdr);

which assumes that sg is a contigious piece of memory, for chained sg lists that isn't true. sg chaining will be in 2.6.24, so if you really do need large sglists, then that's the way to go.

Quoted 2 lines

Except "0/1" is not canonical in the kernel. Arguably, "-errno/0" is canonical. OTOH, bool is clear.

-errno/0, then. 1 is typically used for 'failure without specific error number' when -Exx doesn't apply. Like the above :-)

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Rusty Russell

(quoted diff) +10 -0

+static void end_dequeued_request(struct request *req,
+				 struct request_queue *q, int uptodate)
+{
+	/* And so the insanity of the block layer infects us here. */
+	int nsectors = req->hard_nr_sectors;
+
+	if (blk_pc_request(req)) {
+		nsectors = (req->data_len + 511) >> 9;
+		if (!nsectors)
+			nsectors = 1;

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+	}
+	if (end_that_request_first(req, uptodate, nsectors))
+		BUG();
+	add_disk_randomness(req->rq_disk);
+	end_that_request_last(req, uptodate);
+}

We have end_queued_request(), lets add end_dequeued_request(). Below.

OK, thanks, I'll throw that in the mix and test...

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	vblk->sg[0].page = virt_to_page(&vbr->out_hdr);
+	vblk->sg[0].offset = offset_in_page(&vbr->out_hdr);
+	vblk->sg[0].length = sizeof(vbr->out_hdr);
+	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);

This wont work for chained sglists, but I gather (I'm so funny) that it wont be an issue for you. How large are your sglists?

Hmm, potentially extremely large. What do I need to do for chained sglists?

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		if (!do_req(q, vblk, req)) {
+			/* Queue full?  Wait. */
+			blk_stop_queue(q);
+			break;
+		}

Personally I think this bool stuff is foul. You return false/true, but still use ! to test. That is just more confusing than the canonical 0/1 good/bad return imho.

Except "0/1" is not canonical in the kernel. Arguably, "-errno/0" is canonical. OTOH, bool is clear.

if do_req() fails, we assume the queue is full. I shall change the comment to that effect.

The block driver uses scatter-gather lists with sg[0] being the request information (struct virtio_blk_outhdr) with the type, sector and inbuf id. The next N sg entries are the bio itself, then the last sg is the status byte. Whether the N entries are in or out depends on whether it's a read or a write.

We accept the normal (SCSI) ioctls: they get handed through to the other side which can then handle it or reply that it's unsupported. It's not clear that this actually works in general, since I don't know if blk_pc_request() requests have an accurate rq_data_dir().

Although we try to reply -ENOTTY on unsupported commands, the block layer in its infinite wisdom suppressed the error so ioctl(fd, CDROMEJECT) returns success to userspace.

(quoted diff) +6 -0

+static void end_dequeued_request(struct request *req,
+				 struct request_queue *q, int uptodate)
+{
+	/* And so the insanity of the block layer infects us here. */
+	int nsectors = req->hard_nr_sectors;
+

The network driver uses *two* virtqueues: one for input packets and one for output packets. This has nice locking properties (ie. we don't do any for recv vs send).

3) Resolve freeing of old xmit skbs (someone sent patch IIRC?)

Am Donnerstag, 20. September 2007 schrieb Rusty Russell:

Quoted 3 lines

The network driver uses *two* virtqueues: one for input packets and one for output packets. This has nice locking properties (ie. we don't do any for recv vs send).

[...]

Quoted 1 line

3) Resolve freeing of old xmit skbs (someone sent patch IIRC?)

Yes, that was me. I am quite busy at the moment but I will send a refreshed patch soon. The most annoying fact of my current patch is, that I have to add locking. (Because of the only one operation per virtqueue at a time rule)

[...]

drivers/net/virtio_net.c line 35

Quoted 6 lines

drivers/net/virtio_net.c new file +6 -0

+struct virtnet_info
+{
+	struct virtqueue_ops *vq_ops;
+	struct virtqueue *vq_recv;
+	struct virtqueue *vq_send;
+	struct net_device *ndev;

This is only a matter of taste, but I like netdev or dev more than ndev.

Can't we just re-use the default xmit lock?

Please don't use LLTX in new drivers. We're trying to get rid of it since it's

1) unnecessary;
2) causes problems with AF_PACKET seeing things twice.

Am Freitag, 21. September 2007 schrieb Herbert Xu:

Quoted 5 lines

Please don't use LLTX in new drivers. We're trying to get rid of it since it's

1) unnecessary;
2) causes problems with AF_PACKET seeing things twice.

Ok, but then I cannot reuse the xmit lock in an interrupt handler. Otherwise deadlock can occur because hard_start_xmit has interrupts enabled.

Rusty, seems we need an additional lock.

Am Freitag, 21. September 2007 schrieb Herbert Xu:

Quoted 5 lines

Please don't use LLTX in new drivers. We're trying to get rid of it since it's

1) unnecessary;
2) causes problems with AF_PACKET seeing things twice.

Ok, but then I cannot reuse the xmit lock in an interrupt handler. Otherwise deadlock can occur because hard_start_xmit has interrupts enabled.

Rusty, seems we need an additional lock.

Please don't use LLTX in new drivers. We're trying to get rid of it since it's

1) unnecessary;
2) causes problems with AF_PACKET seeing things twice.

Please don't use LLTX in new drivers. We're trying to get rid of it since it's

1) unnecessary;
2) causes problems with AF_PACKET seeing things twice.

I suggest to document that LLTX is deprecated.

Signed-off-by Christian Borntraeger <[email protected]>

Quoted 4 lines

I suggest to document that LLTX is deprecated.

Signed-off-by Christian Borntraeger <[email protected]>

This looks good to me.

Can't we just re-use the default xmit lock?

Am Donnerstag, 20. September 2007 schrieb Rusty Russell:

Quoted 3 lines

The network driver uses *two* virtqueues: one for input packets and one for output packets. This has nice locking properties (ie. we don't do any for recv vs send).

[...]

Quoted 1 line

3) Resolve freeing of old xmit skbs (someone sent patch IIRC?)

Yes, that was me. I am quite busy at the moment but I will send a refreshed patch soon. The most annoying fact of my current patch is, that I have to add locking. (Because of the only one operation per virtqueue at a time rule)

[...]

drivers/net/virtio_net.c line 35

Quoted 6 lines

drivers/net/virtio_net.c new file +6 -0

+struct virtnet_info
+{
+	struct virtqueue_ops *vq_ops;
+	struct virtqueue *vq_recv;
+	struct virtqueue *vq_send;
+	struct net_device *ndev;

This is only a matter of taste, but I like netdev or dev more than ndev.

The network driver uses *two* virtqueues: one for input packets and one for output packets. This has nice locking properties (ie. we don't do any for recv vs send).

3) Resolve freeing of old xmit skbs (someone sent patch IIRC?)

Previous versions of virtio didn't commonalize probing. For every driver, every virtio implementation (KVM, lguest, etc) needed an in-kernel stub to join their bus to the probe code.

To solve this, we introduce a "virtio_config" mechanism, which is simply a set of [u8 type][u8 len][...data...] fields for each device. Some convenient wrapper functions make this fairly easy for drivers to unpack their configuration data themselves.

This configuration data could be PCI config space, or an unrelated bus (eg. lguest) or manufactured by the kernel itself. It's designed to be extensible: fields get marked as the device reads them so a host supporting some future extension can tell if the guest driver understood it. This also applies to bitfields: the guest explicitly acks the bits it understands.

There's also a simple status bitmask useful for low-level host analysis of what the guest is doing with the device.

Quoted 16 lines · Rusty Russell

Previous versions of virtio didn't commonalize probing. For every driver, every virtio implementation (KVM, lguest, etc) needed an in-kernel stub to join their bus to the probe code.

To solve this, we introduce a "virtio_config" mechanism, which is simply a set of [u8 type][u8 len][...data...] fields for each device. Some convenient wrapper functions make this fairly easy for drivers to unpack their configuration data themselves.

This configuration data could be PCI config space, or an unrelated bus (eg. lguest) or manufactured by the kernel itself. It's designed to be extensible: fields get marked as the device reads them so a host supporting some future extension can tell if the guest driver understood it. This also applies to bitfields: the guest explicitly acks the bits it understands.

Quoted 4 lines

There's also a simple status bitmask useful for low-level host analysis of what the guest is doing with the device.

Lovely.

Quoted 22 lines · Avi Kivity

Quoted 20 lines · Rusty Russell

Previous versions of virtio didn't commonalize probing. For every driver, every virtio implementation (KVM, lguest, etc) needed an in-kernel stub to join their bus to the probe code.

To solve this, we introduce a "virtio_config" mechanism, which is simply a set of [u8 type][u8 len][...data...] fields for each device. Some convenient wrapper functions make this fairly easy for drivers to unpack their configuration data themselves.

This configuration data could be PCI config space, or an unrelated bus (eg. lguest) or manufactured by the kernel itself. It's designed to be extensible: fields get marked as the device reads them so a host supporting some future extension can tell if the guest driver understood it. This also applies to bitfields: the guest explicitly acks the bits it understands.

There's also a simple status bitmask useful for low-level host analysis of what the guest is doing with the device.

Quoted 2 lines

Lovely.

A side effect of this is that Xen drivers can no longer use virtio.

Quoted 3 lines

A side effect of this is that Xen drivers can no longer use virtio.

I'm not so sure.

We were always assuming that Xen could do state management in its virtio layer. If this is not true, it implies we need hooks in the virtio drivers, and I don't think we've made it worse by making it translate xenbus config info into virtio_config.

Quoted 3 lines

A side effect of this is that Xen drivers can no longer use virtio.

I'm not so sure.

We were always assuming that Xen could do state management in its virtio layer. If this is not true, it implies we need hooks in the virtio drivers, and I don't think we've made it worse by making it translate xenbus config info into virtio_config.

Quoted 22 lines · Avi Kivity

Quoted 20 lines · Rusty Russell

Previous versions of virtio didn't commonalize probing. For every driver, every virtio implementation (KVM, lguest, etc) needed an in-kernel stub to join their bus to the probe code.

To solve this, we introduce a "virtio_config" mechanism, which is simply a set of [u8 type][u8 len][...data...] fields for each device. Some convenient wrapper functions make this fairly easy for drivers to unpack their configuration data themselves.

This configuration data could be PCI config space, or an unrelated bus (eg. lguest) or manufactured by the kernel itself. It's designed to be extensible: fields get marked as the device reads them so a host supporting some future extension can tell if the guest driver understood it. This also applies to bitfields: the guest explicitly acks the bits it understands.

There's also a simple status bitmask useful for low-level host analysis of what the guest is doing with the device.

Quoted 2 lines

Lovely.

A side effect of this is that Xen drivers can no longer use virtio.

Quoted 16 lines · Rusty Russell

Previous versions of virtio didn't commonalize probing. For every driver, every virtio implementation (KVM, lguest, etc) needed an in-kernel stub to join their bus to the probe code.

To solve this, we introduce a "virtio_config" mechanism, which is simply a set of [u8 type][u8 len][...data...] fields for each device. Some convenient wrapper functions make this fairly easy for drivers to unpack their configuration data themselves.

This configuration data could be PCI config space, or an unrelated bus (eg. lguest) or manufactured by the kernel itself. It's designed to be extensible: fields get marked as the device reads them so a host supporting some future extension can tell if the guest driver understood it. This also applies to bitfields: the guest explicitly acks the bits it understands.

Quoted 4 lines

There's also a simple status bitmask useful for low-level host analysis of what the guest is doing with the device.

Lovely.

Previous versions of virtio didn't commonalize probing. For every driver, every virtio implementation (KVM, lguest, etc) needed an in-kernel stub to join their bus to the probe code.

To solve this, we introduce a "virtio_config" mechanism, which is simply a set of [u8 type][u8 len][...data...] fields for each device. Some convenient wrapper functions make this fairly easy for drivers to unpack their configuration data themselves.

This configuration data could be PCI config space, or an unrelated bus (eg. lguest) or manufactured by the kernel itself. It's designed to be extensible: fields get marked as the device reads them so a host supporting some future extension can tell if the guest driver understood it. This also applies to bitfields: the guest explicitly acks the bits it understands.

There's also a simple status bitmask useful for low-level host analysis of what the guest is doing with the device.

(Changes:
- renamed sync to kick as Dor suggested
- added new_vq and free_vq hooks to create virtqueues
- define a simple virtio driver, which uses PCI ids
- provide register/unregister_virtio_driver hooks)

This attempts to implement a "virtual I/O" layer which should allow common drivers to be efficiently used across most virtual I/O mechanisms. It will no-doubt need further enhancement.

The virtio drivers add and get I/O buffers; as the buffers are consumed the driver "interrupt" callbacks are invoked.

It also provides driver register and unregister hooks, which are simply overridden at run time (eg. for a guest kernel which supports KVM paravirt and lguest).

+++ b/drivers/virtio/virtio.c

drivers/virtio/virtio.c line 4

Quoted 2 lines · Rusty Russell

drivers/virtio/virtio.c new file +2 -0

+struct virtio_backend_ops virtio_backend_ops;
+EXPORT_SYMBOL_GPL(virtio_backend_ops);

Suggest calling this virtio_transport_ops rather than the too-generic virtio_backend_ops. Especially since Xen uses backend for something completely different.

Quoted 7 lines

+/**
+ * virtqueue_ops - operations for virtqueue abstraction layer
+ * @new_vq: create a new virtqueue
+ * config: the virtio_config_space field describing the queue
+ * off: the offset in the config space of the queue configuration
+ * len: the length of the virtio_config_space field

'off, len' are really a magic cookie. Why does the interface care about their meaning?

Quoted 2 lines

+ * callback: the driver callback when the queue is used.

Missing callback return value description.

Quoted 4 lines

+ * @kick: update after add_buf
+ * vq: the struct virtqueue
+ * After one or more add_buf calls, invoke this to kick the virtio layer.

'the other side'

I'm not thrilled about reusing pci ids. Maybe the s390 can say whether this is a real issue.

+ * virtio_driver - operations for a virtio I/O driver
+ * @name: the name of the driver (KBUILD_MODNAME).
+ * @owner: the module which contains these routines (ie. THIS_MODULE).
+ * @id_table: the ids (we re-use PCI ids) serviced by this driver.
+ * @probe: the function to call when a device is found.  Returns a token for
+ *    remove, or PTR_ERR().
+ * @remove: the function when a device is removed.
+ */

include/linux/virtio.h new file +9 -0

+struct virtio_driver {
+       const char *name;
+       struct module *owner;
+       struct pci_device_id *id_table;
+       void *(*probe)(struct device *device,
+                      struct virtio_config_space *config,
+                      struct virtqueue_ops *vqops);
+       void (*remove)(void *dev);
+};

include/linux/virtio.h line 113

Quoted 9 lines · Rusty Russell

include/linux/virtio.h new file +9 -0

+int register_virtio_driver(struct virtio_driver *drv);
+void unregister_virtio_driver(struct virtio_driver *drv);
+
+/* The particular virtio backend supplies these. */
+struct virtio_backend_ops {
+       int (*register_driver)(struct virtio_driver *drv);
+       void (*unregister_driver)(struct virtio_driver *drv);
+};
+extern struct virtio_backend_ops virtio_backend_ops;

This still seems a little awkward. From what I understand, you register a virtio_driver, which leads to a pci_driver (or whatever you are based on) to be registered behind the covers, so that the pci_device can be used directly as the virtio device.

I think there should instead be a pci_driver that automatically binds to all PCI based virtio imlpementations and creates a child device for the actual virtio_device.

Hmm, I guess we could have a PCI driver which claims all VIRTIO vendor devices.

Then it can call virtio_find_driver() (?) at the top of its probe function to find if there's a matching virtio driver. This PCI driver would have to be initialized after all the virtio drivers are registered, but that's easy.

The virtio layer would simply maintain a linked list of drivers and implement the virtio_find_driver() matching function.

And since we've suppressed normal PCI driver request_module (since it always finds "the driver") then we can implement that in virtio_find_driver(), and not use a PCI MODULE_DEVICE_TABLE.  Then we don't need (full) PCI ids at all.

Quoted 2 lines · Rusty Russell

Hmm, I guess we could have a PCI driver which claims all VIRTIO vendor devices.

yes, that was the idea.

Quoted 4 lines

Then it can call virtio_find_driver() (?) at the top of its probe function to find if there's a matching virtio driver. This PCI driver would have to be initialized after all the virtio drivers are registered, but that's easy.

No, just use the driver model, instead of working against it:

But now each virtio device has two "struct device"s, not one.   And you've made up a fictional bus to do it.

Yet for PCI systems, it really is a PCI device; exposing a second bus to userspace just because we put a layer in our implementation is poor form.

Perhaps this is the easiest way of doing it.  But it's still wrong.

Quoted 8 lines · Rusty Russell

But now each virtio device has two "struct device"s, not one. And
you've made up a fictional bus to do it.

Yet for PCI systems, it really is a PCI device; exposing a second bus to userspace just because we put a layer in our implementation is poor form.

Perhaps this is the easiest way of doing it. But it's still wrong.

I think it's just a matter of perspective. In the model I'm advocating, the PCI device is not the same as the virtio device but rather a virtio host bridge, very much like USB or SATA works.

We could easily have multiple virtio devices behind one PCI device, but since virtual PCI devices are cheap, a one-to-one mapping makes sense for simplicity.

Quoted 11 lines · Arnd Bergmann

Quoted 9 lines · Rusty Russell

But now each virtio device has two "struct device"s, not one. And
you've made up a fictional bus to do it.

Yet for PCI systems, it really is a PCI device; exposing a second bus to userspace just because we put a layer in our implementation is poor form.

Perhaps this is the easiest way of doing it. But it's still wrong.

Quoted 8 lines

I think it's just a matter of perspective. In the model I'm advocating, the PCI device is not the same as the virtio device but rather a virtio host bridge, very much like USB or SATA works.

We could easily have multiple virtio devices behind one PCI device, but since virtual PCI devices are cheap, a one-to-one mapping makes sense for simplicity.

This is still retro-justification. The simplest way for PCI systems to represent a virtio device as a PCI device; this makes life easy for any guest OS.

I just know we're going to regret this...
Rusty.

Quoted 11 lines · Arnd Bergmann

Quoted 9 lines · Rusty Russell

But now each virtio device has two "struct device"s, not one. And
you've made up a fictional bus to do it.

Yet for PCI systems, it really is a PCI device; exposing a second bus to userspace just because we put a layer in our implementation is poor form.

Perhaps this is the easiest way of doing it. But it's still wrong.

Quoted 8 lines

I think it's just a matter of perspective. In the model I'm advocating, the PCI device is not the same as the virtio device but rather a virtio host bridge, very much like USB or SATA works.

We could easily have multiple virtio devices behind one PCI device, but since virtual PCI devices are cheap, a one-to-one mapping makes sense for simplicity.

This is still retro-justification. The simplest way for PCI systems to represent a virtio device as a PCI device; this makes life easy for any guest OS.

I just know we're going to regret this...
Rusty.

Quoted 8 lines · Rusty Russell

But now each virtio device has two "struct device"s, not one. And
you've made up a fictional bus to do it.

Yet for PCI systems, it really is a PCI device; exposing a second bus to userspace just because we put a layer in our implementation is poor form.

Perhaps this is the easiest way of doing it. But it's still wrong.

I think it's just a matter of perspective. In the model I'm advocating, the PCI device is not the same as the virtio device but rather a virtio host bridge, very much like USB or SATA works.

We could easily have multiple virtio devices behind one PCI device, but since virtual PCI devices are cheap, a one-to-one mapping makes sense for simplicity.

But now each virtio device has two "struct device"s, not one.   And you've made up a fictional bus to do it.

Yet for PCI systems, it really is a PCI device; exposing a second bus to userspace just because we put a layer in our implementation is poor form.

Perhaps this is the easiest way of doing it.  But it's still wrong.

Hmm, I guess we could have a PCI driver which claims all VIRTIO vendor devices.

Then it can call virtio_find_driver() (?) at the top of its probe function to find if there's a matching virtio driver. This PCI driver would have to be initialized after all the virtio drivers are registered, but that's easy.

The virtio layer would simply maintain a linked list of drivers and implement the virtio_find_driver() matching function.

And since we've suppressed normal PCI driver request_module (since it always finds "the driver") then we can implement that in virtio_find_driver(), and not use a PCI MODULE_DEVICE_TABLE.  Then we don't need (full) PCI ids at all.

include/linux/virtio.h line 113

Quoted 9 lines · Rusty Russell

include/linux/virtio.h new file +9 -0

+int register_virtio_driver(struct virtio_driver *drv);
+void unregister_virtio_driver(struct virtio_driver *drv);
+
+/* The particular virtio backend supplies these. */
+struct virtio_backend_ops {
+       int (*register_driver)(struct virtio_driver *drv);
+       void (*unregister_driver)(struct virtio_driver *drv);
+};
+extern struct virtio_backend_ops virtio_backend_ops;

This still seems a little awkward. From what I understand, you register a virtio_driver, which leads to a pci_driver (or whatever you are based on) to be registered behind the covers, so that the pci_device can be used directly as the virtio device.

I think there should instead be a pci_driver that automatically binds to all PCI based virtio imlpementations and creates a child device for the actual virtio_device.

(Changes:
- renamed sync to kick as Dor suggested
- added new_vq and free_vq hooks to create virtqueues
- define a simple virtio driver, which uses PCI ids
- provide register/unregister_virtio_driver hooks)

This attempts to implement a "virtual I/O" layer which should allow common drivers to be efficiently used across most virtual I/O mechanisms. It will no-doubt need further enhancement.

The virtio drivers add and get I/O buffers; as the buffers are consumed the driver "interrupt" callbacks are invoked.

It also provides driver register and unregister hooks, which are simply overridden at run time (eg. for a guest kernel which supports KVM paravirt and lguest).

+++ b/drivers/virtio/virtio.c

+ * virtio_driver - operations for a virtio I/O driver
+ * @name: the name of the driver (KBUILD_MODNAME).
+ * @owner: the module which contains these routines (ie. THIS_MODULE).
+ * @id_table: the ids (we re-use PCI ids) serviced by this driver.
+ * @probe: the function to call when a device is found.  Returns a token for
+ *    remove, or PTR_ERR().
+ * @remove: the function when a device is removed.
+ */

include/linux/virtio.h new file +9 -0

+struct virtio_driver {
+       const char *name;
+       struct module *owner;
+       struct pci_device_id *id_table;
+       void *(*probe)(struct device *device,
+                      struct virtio_config_space *config,
+                      struct virtqueue_ops *vqops);
+       void (*remove)(void *dev);
+};

Hi all,

This patch series attempts to come closer to unifying kvm and
lguest's
usage of virtio. As these two are the first implementations we've seen, I hope making them closer will make future ones closer too.

Drivers now unpack their own configuration: their probe()
methods are
uniform. The configuration mechanism is extensible and can be backed by PCI, a string of bytes, or something else.

I've abstracted out the lguest ring buffer code into a common
library.
The format has changed slightly (mainly because I had an epiphany about inter-guest I/O).

I also implemented a console (lguest needs one).

Finally, there is a working lguest implementation. Unfortunately,
lguest is being refactored for non-i386 ports, so the virtio patches sit at the end of the (quite long) for-2.6.24 patchqueue. Nonetheless, they can be found at http://lguest.ozlabs.org/patches (click on bz2 to get the series).

Cheers!
Rusty.

Quoted 27 lines · Rusty Russell

Hi all,

This patch series attempts to come closer to unifying kvm and
lguest's
usage of virtio. As these two are the first implementations we've seen, I hope making them closer will make future ones closer too.

Drivers now unpack their own configuration: their probe()
methods are
uniform. The configuration mechanism is extensible and can be backed by PCI, a string of bytes, or something else.

I've abstracted out the lguest ring buffer code into a common
library.
The format has changed slightly (mainly because I had an epiphany about inter-guest I/O).

I also implemented a console (lguest needs one).

Finally, there is a working lguest implementation.
Unfortunately,
lguest is being refactored for non-i386 ports, so the virtio patches sit at the end of the (quite long) for-2.6.24 patchqueue. Nonetheless, they can be found at http://lguest.ozlabs.org/patches (click on bz2 to get the series).

Quoted 3 lines

Cheers!
Rusty.

Superb job, it saved me the burden of try to merge the in-house virtio_backend.

I like the separation of the ring code, the improved descriptors and the notify too.
Regarding the pci config space, I rather see config_ops type of operations to let
the 390/xen/other implementations jump on our wagon.

Maybe change the offset/len type into a handle pointer and function pointers.
The best would be to let them comment on that. I glimpsed over xen netfront.c and I think that
the config space can be used without too many hassles Dor.

Quoted 27 lines · Rusty Russell

Hi all,

This patch series attempts to come closer to unifying kvm and
lguest's
usage of virtio. As these two are the first implementations we've seen, I hope making them closer will make future ones closer too.

Drivers now unpack their own configuration: their probe()
methods are
uniform. The configuration mechanism is extensible and can be backed by PCI, a string of bytes, or something else.

I've abstracted out the lguest ring buffer code into a common
library.
The format has changed slightly (mainly because I had an epiphany about inter-guest I/O).

I also implemented a console (lguest needs one).

Finally, there is a working lguest implementation.
Unfortunately,
lguest is being refactored for non-i386 ports, so the virtio patches sit at the end of the (quite long) for-2.6.24 patchqueue. Nonetheless, they can be found at http://lguest.ozlabs.org/patches (click on bz2 to get the series).

Quoted 3 lines

Cheers!
Rusty.

Superb job, it saved me the burden of try to merge the in-house virtio_backend.

I like the separation of the ring code, the improved descriptors and the notify too. Regarding the pci config space, I rather see config_ops type of operations to let the 390/xen/other implementations jump on our wagon.

Maybe change the offset/len type into a handle pointer and function pointers. The best would be to let them comment on that. I glimpsed over xen netfront.c and I think that the config space can be used without too many hassles
Dor.

Quoted 3 lines · Rusty Russell

Drivers now unpack their own configuration: their probe() methods are
uniform. The configuration mechanism is extensible and can be backed by PCI, a string of bytes, or something else.

I like the separation of the ring code, the improved descriptors and the notify too.
Regarding the pci config space, I rather see config_ops type of operations to let
the 390/xen/other implementations jump on our wagon.

Hi all,

This patch series attempts to come closer to unifying kvm and
lguest's
usage of virtio. As these two are the first implementations we've seen, I hope making them closer will make future ones closer too.

Drivers now unpack their own configuration: their probe()
methods are
uniform. The configuration mechanism is extensible and can be backed by PCI, a string of bytes, or something else.

I've abstracted out the lguest ring buffer code into a common
library.
The format has changed slightly (mainly because I had an epiphany about inter-guest I/O).

I also implemented a console (lguest needs one).

Finally, there is a working lguest implementation. Unfortunately,
lguest is being refactored for non-i386 ports, so the virtio patches sit at the end of the (quite long) for-2.6.24 patchqueue. Nonetheless, they can be found at http://lguest.ozlabs.org/patches (click on bz2 to get the series).

Cheers!
Rusty.