virtio: add guest MSI-X support

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

Am Monday 27 April 2009 14:31:36 schrieb Michael S. Tsirkin:

Quoted 4 lines

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

Quoted 4 lines

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

I dont know, if that is feasible for MSI, but the transport(virtio_pci) should already know the number of virtqueues, which should match the number of vectors, no?

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

Christian

Am Monday 27 April 2009 14:31:36 schrieb Michael S. Tsirkin:

Quoted 4 lines

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

Quoted 4 lines

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

I dont know, if that is feasible for MSI, but the transport(virtio_pci) should already know the number of virtqueues, which should match the number of vectors, no?

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

Christian

Quoted 3 lines

I dont know, if that is feasible for MSI, but the transport(virtio_pci) should already know the number of virtqueues, which should match the number of vectors, no?

I think no, the transport can find out the max number of vectors the device supports (pci_msix_table_size), but not how many virtqueues are needed by the driver.

As number of virtqueues <= number of vectors,
we could pre-allocate all vectors that host supports, but this seems a bit drastic as an MSI-X device could support up to 2K vectors.

Quoted 4 lines

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

Christian

So again, I think this is an upper bound supported by host. Right?

Quoted 3 lines

I dont know, if that is feasible for MSI, but the transport(virtio_pci) should already know the number of virtqueues, which should match the number of vectors, no?

I think no, the transport can find out the max number of vectors the device supports (pci_msix_table_size), but not how many virtqueues are needed by the driver.

As number of virtqueues <= number of vectors,
we could pre-allocate all vectors that host supports, but this seems a bit drastic as an MSI-X device could support up to 2K vectors.

Quoted 4 lines

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

Christian

So again, I think this is an upper bound supported by host. Right?

Quoted 7 lines

As number of virtqueues <= number of vectors,
we could pre-allocate all vectors that host supports, but this seems a bit drastic as an MSI-X device could support up to 2K vectors.

Quoted 3 lines

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

Quoted 3 lines

Quoted 1 line

Christian

So again, I think this is an upper bound supported by host. Right?

Not the upper bound, but the real available virtqueues. (With current qemu 3 for virtio-net, 2 for virtio-console etc.)

Since I use a different transport (drivers/s390/kvm/kvm_virtio.c), my motiviation is to keep the virtio interface as generic as possible. I dont really like the new interface, but I cannot give you silver bullet technical reasons - its more a gut feeling. The interface would work with lguest and s390.

Anyway. Avis suggestion to decouple MSI count and virtqueue count looks like a promising approach.

Quoted 7 lines

As number of virtqueues <= number of vectors,
we could pre-allocate all vectors that host supports, but this seems a bit drastic as an MSI-X device could support up to 2K vectors.

Quoted 3 lines

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

Quoted 3 lines

Quoted 1 line

Christian

So again, I think this is an upper bound supported by host. Right?

Not the upper bound, but the real available virtqueues. (With current qemu 3 for virtio-net, 2 for virtio-console etc.)

Since I use a different transport (drivers/s390/kvm/kvm_virtio.c), my motiviation is to keep the virtio interface as generic as possible. I dont really like the new interface, but I cannot give you silver bullet technical reasons - its more a gut feeling. The interface would work with lguest and s390.

Anyway. Avis suggestion to decouple MSI count and virtqueue count looks like a promising approach.

Am Monday 27 April 2009 14:31:36 schrieb Michael S. Tsirkin:

Quoted 9 lines

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

I dont know, if that is feasible for MSI, but the transport(virtio_pci) should already know the number of virtqueues, which should match the number of vectors, no?

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

Quoted 12 lines · Christian Borntraeger

Am Monday 27 April 2009 14:31:36 schrieb Michael S. Tsirkin:

Quoted 9 lines

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

Quoted 7 lines

I dont know, if that is feasible for MSI, but the transport(virtio_pci) should already know the number of virtqueues, which should match the number of vectors, no?

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

One thing I thought was to decouple the virtqueue count from the MSI entry count, and let the guest choose how many MSI entries it wants to use. This is useful if it wants several queues to share an interrupt, perhaps it wants both rx and tx rings on one cpu to share a single vector.

So the device could expose a large, constant number of MSI entries, and in addition expose a table mapping ring number to msi entry number. The guest could point all rings to one entry, or have each ring use a private entry, or anything in between.

That saves us the new API (at the expense of a lot more code, but with added flexibility).

Quoted 12 lines · Christian Borntraeger

Am Monday 27 April 2009 14:31:36 schrieb Michael S. Tsirkin:

Quoted 9 lines

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

Quoted 7 lines

I dont know, if that is feasible for MSI, but the transport(virtio_pci) should already know the number of virtqueues, which should match the number of vectors, no?

In fact, the transport has to have a way of getting the number of virtqeues because find_vq returns ENOENT on invalid index numbers.

One thing I thought was to decouple the virtqueue count from the MSI entry count, and let the guest choose how many MSI entries it wants to use. This is useful if it wants several queues to share an interrupt, perhaps it wants both rx and tx rings on one cpu to share a single vector.

So the device could expose a large, constant number of MSI entries, and in addition expose a table mapping ring number to msi entry number. The guest could point all rings to one entry, or have each ring use a private entry, or anything in between.

That saves us the new API (at the expense of a lot more code, but with added flexibility).

So we'll probably need to rename request_vqs to request_vectors, but we probably still need the driver to pass the number of vectors it wants to the transport. Right?

So we'll probably need to rename request_vqs to request_vectors, but we probably still need the driver to pass the number of vectors it wants to the transport. Right?

Am Monday 27 April 2009 17:39:36 schrieb Michael S. Tsirkin:

Quoted 3 lines

So we'll probably need to rename request_vqs to request_vectors, but we probably still need the driver to pass the number of vectors it wants to the transport. Right?

This might be a stupid idea, but would something like the following be sufficient for you?

Index: linux-2.6/drivers/net/virtio_net.c
===================================================================

drivers/net/virtio_net.c +1 -0

@@ -1021,6 +1021,7 @@ static unsigned int features[] = {
 static struct virtio_driver virtio_net = {
        .feature_table = features,
        .feature_table_size = ARRAY_SIZE(features),
+       .num_vq_max = 3,
        .driver.name =  KBUILD_MODNAME,
        .driver.owner = THIS_MODULE,
        .id_table =     id_table,

Index: linux-2.6/include/linux/virtio.h
===================================================================

Am Monday 27 April 2009 17:39:36 schrieb Michael S. Tsirkin:

Quoted 3 lines

So we'll probably need to rename request_vqs to request_vectors, but we probably still need the driver to pass the number of vectors it wants to the transport. Right?

This might be a stupid idea, but would something like the following be sufficient for you?

Index: linux-2.6/drivers/net/virtio_net.c
===================================================================

drivers/net/virtio_net.c +1 -0

@@ -1021,6 +1021,7 @@ static unsigned int features[] = {
 static struct virtio_driver virtio_net = {
        .feature_table = features,
        .feature_table_size = ARRAY_SIZE(features),
+       .num_vq_max = 3,
        .driver.name =  KBUILD_MODNAME,
        .driver.owner = THIS_MODULE,
        .id_table =     id_table,

Index: linux-2.6/include/linux/virtio.h
===================================================================

Quoted 3 lines

That saves us the new API (at the expense of a lot more code, but with added flexibility).

So we'll probably need to rename request_vqs to request_vectors, but we probably still need the driver to pass the number of vectors it wants to the transport. Right?

Quoted 2 lines

That saves us the new API (at the expense of a lot more code, but with added flexibility).

So we'll probably need to rename request_vqs to request_vectors, but we probably still need the driver to pass the number of vectors it wants to the transport. Right?

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

That saves us the new API (at the expense of a lot more code, but with added flexibility).

Quoted 5 lines

So we'll probably need to rename request_vqs to request_vectors, but we probably still need the driver to pass the number of vectors it wants to the transport. Right?

I don't think so - virtio will provide the number of interrupts it supports, and virtio-net will tell it to bind ring X to interrupt Y.

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

That saves us the new API (at the expense of a lot more code, but with added flexibility).

Quoted 5 lines

So we'll probably need to rename request_vqs to request_vectors, but we probably still need the driver to pass the number of vectors it wants to the transport. Right?

I don't think so - virtio will provide the number of interrupts it supports, and virtio-net will tell it to bind ring X to interrupt Y.

This does not work for MSIX - in linux, you must map all MSI-X entries to interrupt vectors upfront.

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

This does not work for MSIX - in linux, you must map all MSI-X entries to interrupt vectors upfront.

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Quoted 3 lines · Michael S. Tsirkin

This does not work for MSIX - in linux, you must map all MSI-X entries to interrupt vectors upfront.

What? that's very inflexible.

Can you point me at the code?

Quoted 7 lines

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

Quoted 3 lines

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Isn't that the same thing? Please explain the flow.

Quoted 3 lines · Michael S. Tsirkin

This does not work for MSIX - in linux, you must map all MSI-X entries to interrupt vectors upfront.

What? that's very inflexible.

Can you point me at the code?

Quoted 7 lines

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

Quoted 3 lines

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Isn't that the same thing? Please explain the flow.

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

and then find_vq as usual.

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

and then find_vq as usual.

Quoted 8 lines · Michael S. Tsirkin

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

Quoted 2 lines

and then find_vq as usual.

Is it possible to just delay the msix enablement until after the queues have been finalized (IOW in virtio-pci.c:vp_finalize_features)?

Quoted 7 lines

Quoted 7 lines

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

Quoted 6 lines

Quoted 4 lines

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Isn't that the same thing? Please explain the flow.

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

and then find_vq as usual.

Quoted 7 lines

Quoted 7 lines

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

Quoted 6 lines

Quoted 4 lines

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Isn't that the same thing? Please explain the flow.

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

and then find_vq as usual.

Quoted 15 lines · Michael S. Tsirkin

Quoted 7 lines

Quoted 7 lines

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

Quoted 5 lines

Quoted 3 lines

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Isn't that the same thing? Please explain the flow.

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

Quoted 8 lines

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

and then find_vq as usual.

Yes, that works.

Given that pci_enable_msix() can fail, we can put the retry loop in virtio-pci, and instead of a static mapping, supply a dynamic mapping:

   static void get_vq_interrupt(..., int nr_interrupts, int vq)
   {
       /* reserve interrupt 0 to config changes; round-robin vqs to  
interrupts */
       return 1 + (vq % (nr_interrupts - 1));

}

   driver_init()
   {
       map_vqs_to_interrupt(dev, get_vq_interrupt);
   }

map_vqs_to_interrupts() would call get_vq_interrupt() for each vq, assuming the maximum nr_interrupts, and retry with smaller nr_interrupts on failure.

Quoted 15 lines · Michael S. Tsirkin

Quoted 7 lines

Quoted 7 lines

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

Quoted 5 lines

Quoted 3 lines

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Isn't that the same thing? Please explain the flow.

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

Quoted 8 lines

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

and then find_vq as usual.

Yes, that works.

Given that pci_enable_msix() can fail, we can put the retry loop in virtio-pci, and instead of a static mapping, supply a dynamic mapping:

   static void get_vq_interrupt(..., int nr_interrupts, int vq)
   {
       /* reserve interrupt 0 to config changes; round-robin vqs to  
interrupts */
       return 1 + (vq % (nr_interrupts - 1));

}

   driver_init()
   {
       map_vqs_to_interrupt(dev, get_vq_interrupt);
   }

map_vqs_to_interrupts() would call get_vq_interrupt() for each vq, assuming the maximum nr_interrupts, and retry with smaller nr_interrupts on failure.

Quoted 39 lines · Avi Kivity

Quoted 16 lines · Michael S. Tsirkin

Quoted 7 lines

Quoted 7 lines

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

Quoted 7 lines

Quoted 4 lines

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Isn't that the same thing? Please explain the flow.

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

Quoted 9 lines

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

and then find_vq as usual.

Yes, that works.

Given that pci_enable_msix() can fail, we can put the retry loop in virtio-pci, and instead of a static mapping, supply a dynamic mapping:

   static void get_vq_interrupt(..., int nr_interrupts, int vq)
   {
       /* reserve interrupt 0 to config changes; round-robin vqs to  
interrupts */
       return 1 + (vq % (nr_interrupts - 1));

}

Quoted 9 lines

   driver_init()
   {
       map_vqs_to_interrupt(dev, get_vq_interrupt);
   }

map_vqs_to_interrupts() would call get_vq_interrupt() for each vq, assuming the maximum nr_interrupts, and retry with smaller nr_interrupts on failure.

Since guest drivers are going to do round-robin most of the time, I think the right thing to do is to make the API simple, along the lines proposed by Rusty, and make the guest/host ABI rich enough to support arbitrary mapping, along the lines proposed by you. We can always change the API, ABI is harder.

Quoted 43 lines · Avi Kivity

Quoted 16 lines · Michael S. Tsirkin

Quoted 7 lines

Quoted 7 lines

So what I see is transports providing something like:

struct virtio_interrupt_mapping {
	int virtqueue;
	int interrupt;
};

Quoted 7 lines

Quoted 4 lines

map_vqs_to_interrupt(dev, struct virtio_interrupt_mapping *, int nvirtqueues); unmap_vqs(dev);

Isn't that the same thing? Please explain the flow.

So to map vq 0 to vector 0, vq 1 to vector 1 and vq 2 to vector 2 the driver would do:

Quoted 9 lines

struct virtio_interrupt_mapping mapping[3] = { {0, 0}, {1, 1}, {2, 2} }; vec = map_vqs_to_interrupt(dev, mapping, 3);
if (vec) {
error handling
}

and then find_vq as usual.

Yes, that works.

Given that pci_enable_msix() can fail, we can put the retry loop in virtio-pci, and instead of a static mapping, supply a dynamic mapping:

   static void get_vq_interrupt(..., int nr_interrupts, int vq)
   {
       /* reserve interrupt 0 to config changes; round-robin vqs to  
interrupts */
       return 1 + (vq % (nr_interrupts - 1));

}

   driver_init()
   {
       map_vqs_to_interrupt(dev, get_vq_interrupt);
   }

Quoted 2 lines

map_vqs_to_interrupts() would call get_vq_interrupt() for each vq, assuming the maximum nr_interrupts, and retry with smaller nr_interrupts on failure.

Since guest drivers are going to do round-robin most of the time, I think the right thing to do is to make the API simple, along the lines proposed by Rusty, and make the guest/host ABI rich enough to support arbitrary mapping, along the lines proposed by you. We can always change the API, ABI is harder.

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Signed-off-by Michael S. Tsirkin <[email protected]>

Quoted 7 lines · Michael S. Tsirkin

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Quoted 2 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

Do you have userspace patches for testing?

Regards,

Anthony Liguori

Quoted 7 lines · Michael S. Tsirkin

Add optional MSI-X support: use a vector per virtqueue with fallback to a common vector and finally to regular interrupt. Teach all drivers to use it.

I added 2 new virtio operations: request_vqs/free_vqs because MSI needs to know the total number of vectors upfront.

Quoted 2 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

Do you have userspace patches for testing?

Regards,

Anthony Liguori