Enable SRIOV with KVM
cover Enable SRIOV with KVM
The first patch have been sent already. The other two deal with some minor problem of SRIOV's virtual function. With these three userspace patches, SRIOV can be enabled with KVM(of course, with MSI-X and my previous userspace patches.)
Thanks.
PATCH 1/3 kvm: fix irq 0 assignment
Shouldn't update assigned irq if host irq is 0, which means uninitialized or don't support INTx.
--- qemu/hw/device-assignment.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
qemu/hw/device-assignment.c +4 -0
@@ -574,6 +574,10 @@ static int assign_irq(AssignedDevInfo *adev)AssignedDevice *dev = adev->assigned_dev;int irq, r =0 ; +/* irq 0 means uninitialized */ +if (dev->real_device.irq ==0 ) +return 0 ; + irq =pci_map_irq (&dev->dev, dev->intpin); irq =piix_get_irq (irq);
Signature · 1 line
1.5.4.5
Re: [PATCH 1/3] kvm: fix irq 0 assignment
Quoted 2 lines · Sheng Yang
Shouldn't update assigned irq if host irq is 0, which means uninitialized or don't support INTx.
Is that generally true?
Re: [PATCH 1/3] kvm: fix irq 0 assignment
Quoted 5 lines · On Wednesday 04 March 2009 17:53:52 Chris Wedgwood wrote:
Quoted 2 lines · Sheng Yang
Shouldn't update assigned irq if host irq is 0, which means uninitialized or don't support INTx.
Is that generally true?
Host irq 0 is reserved for PIT timer, at least for x86.
Signature · 2 lines
regards Yang, Sheng
Re: [PATCH 1/3] kvm: fix irq 0 assignment
Quoted 8 lines · On Wednesday 04 March 2009 17:58:31 Sheng Yang wrote:
Quoted 5 lines · On Wednesday 04 March 2009 17:53:52 Chris Wedgwood wrote:
Quoted 2 lines · Sheng Yang
Shouldn't update assigned irq if host irq is 0, which means uninitialized or don't support INTx.
Is that generally true?
Host irq 0 is reserved for PIT timer, at least for x86.
Or in some other condition it would be used for pci/pcie device? And please notice that currently we only support assign with x86 and IA64. (I really feel there is some reason for you ask. :) )
Signature · 2 lines
regards Yang, Sheng
Re: [PATCH 1/3] kvm: fix irq 0 assignment
In reply to Sheng YangQuoted 13 lines · Sheng Yang
Shouldn't update assigned irq if host irq is 0, which means uninitialized or don't support INTx.
Signed-off-by Sheng Yang <[email protected]>--- qemu/hw/device-assignment.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)qemu/hw/device-assignment.c +0
-0
qemu/hw/device-assignment.c static int assign_irq(AssignedDevInfo *adev) · line 582
Quoted 9 lines
qemu/hw/device-assignment.c +4
-0@@ -574,6 +574,10 @@ static int assign_irq(AssignedDevInfo *adev)AssignedDevice *dev = adev->assigned_dev;int irq, r =0 ; +/* irq 0 means uninitialized */ +if (dev->real_device.irq ==0 ) +return 0 ; + irq =pci_map_irq (&dev->dev, dev->intpin); irq =piix_get_irq (irq);
Sheng,
Please do not special case irq 0. The fact that only x86/IA64 are supported at the moment does not mean other architectures can't use it.
Also, the kernel code to handle dev/irq assignment is quite messy. It should be only a mechanism, with userspace controlling the policy.
For example:
if (match->irq_requested_type & KVM_ASSIGNED_DEV_MSIX)
current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX;
else if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
(match->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_MSI))
current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSI;And then in userspace you have
+ if (*ctrl_word & PCI_MSIX_ENABLE) {
+ assigned_irq_data.flags = KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX;
+ if (assigned_dev_update_msix_mmio(pci_dev) < 0) {
+ perror("assigned_dev_update_msix_mmio");
+ }
+ }
We really need to fix this before merging anything else in this area.
So ideally the kernel only provides ioctl commands that do one thing at a time:
- assign host device
- unassign guest device
- assign host irq (INTx or MSI)
- assign dev irq (INTx or MSI)
- unassign host irq
- unassign dev irq
So you don't have to make policy decisions like
} else {
/*
* Guest require to disable device MSI, we disable MSI
* and
* re-enable INTx by default again. Notice it's only for
* non-msi2intx.
*/
assigned_device_update_intx(kvm, adev, airq);
return 0;
}Because you do in userspace. However, I do not think it is necessary to create new ioctl commands and deprecate the old ones (it seems possible to do this using the flags passed in "struct kvm_assigned_irq")
However, the current userspace code probably relies on implicit behaviour by the kernel part. IMHO it is fair to break older userspace. Better change it sooner than later.
For example, don't do this
if ((changed_flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION) ||
(msi2intx && match->dev->msi_enabled)) {But just enable MSI in either host or guest (one at a time). Return an error if you can't. And don't make such assumptions:
} else if (assigned_irq->host_irq == 0 && match->dev->irq == 0)
/* Host device IRQ 0 means don't support INTx */To use the current ioctl commands, maybe enforce the "one thing at a time" nature and fail otherwise.
Can you please investigate about this possibility? Because talk is cheap and I'm not sure whether you can always separate host/guest IRQ assignment, but probably as long as done carefully.
Am I missing any reason why policy can't live outside the kernel?
Re: [PATCH 1/3] kvm: fix irq 0 assignment
Quoted 3 lines · * Marcelo Tosatti ([email protected]) wrote:
Please do not special case irq 0. The fact that only x86/IA64 are supported at the moment does not mean other architectures can't use it.
Seems logical to use a flag instead of overloading the irq value.
Quoted 2 lines
Also, the kernel code to handle dev/irq assignment is quite messy. It should be only a mechanism, with userspace controlling the policy.
I really agree here. I think some work to simplify/clarify would be time well-spent!
Re: [PATCH 1/3] kvm: fix irq 0 assignment
Quoted 5 lines · On Thursday 05 March 2009 03:41:41 Chris Wright wrote:
Quoted 3 lines · * Marcelo Tosatti ([email protected]) wrote:
Please do not special case irq 0. The fact that only x86/IA64 are supported at the moment does not mean other architectures can't use it.
Quoted 7 lines
Seems logical to use a flag instead of overloading the irq value.
Quoted 2 lines
Also, the kernel code to handle dev/irq assignment is quite messy. It should be only a mechanism, with userspace controlling the policy.
I really agree here. I think some work to simplify/clarify would be time well-spent!
So do I... After discussion with Marcelo, I would work with him on this clean up. :)
Signature · 2 lines
regards Yang, Sheng
PATCH 2/3 KVM: Fill config with correct VID/DID
In reply to Sheng YangSRIOV's virtual function didn't show correct Vendor ID/Device ID in config, so we have to fill them manually according to device/vendor file in sysfs.
--- qemu/hw/device-assignment.c | 31 ++++++++++++++++++++++++++++++- 1 files changed, 30 insertions(+), 1 deletions(-)
qemu/hw/device-assignment.c +30 -1
@@ -317,7 +317,8 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,ssize_t ret;AssignedDevice *pci_dev =container_of (d, AssignedDevice, dev); -if ((address >=0x10 && address <=0x24 ) || address ==0x34 || +if (address <0x4 || + (address >=0x10 && address <=0x24 ) || address ==0x34 || address ==0x3c || address ==0x3d ||pci_access_cap_config (d, address, len)) { val =pci_default_read_config (d, address, len);
@@ -429,6 +430,7 @@ static int get_real_device(AssignedDevice *pci_dev, uint8_t r_bus,int fd, r =0 ;FILE *f;unsigned long long start, end, size, flags; +unsigned long id;PCIRegion *rp;PCIDevRegions *dev = &pci_dev->real_device;
@@ -488,6 +490,33 @@ again:DEBUG ("region %d size %d start 0x%llx type %d resource_fd %d\n" , r, rp->size, start, rp->type, rp->resource_fd); } + +fclose (f); + +/* read and fill device ID */ +snprintf (name,sizeof (name),"%svendor" , dir); + f =fopen (name,"r" ); +if (f ==NULL ) { +fprintf (stderr,"%s: %s: %m\n" , __func__, name); +return 1 ; + } +if (fscanf (f,"%li\n" , &id) ==1 ) { + pci_dev->dev.config[0 ] = id &0xff ; + pci_dev->dev.config[1 ] = (id &0xff00 ) >>8 ; + } +fclose (f); + +/* read and fill vendor ID */ +snprintf (name,sizeof (name),"%sdevice" , dir); + f =fopen (name,"r" ); +if (f ==NULL ) { +fprintf (stderr,"%s: %s: %m\n" , __func__, name); +return 1 ; + } +if (fscanf (f,"%li\n" , &id) ==1 ) { + pci_dev->dev.config[2 ] = id &0xff ; + pci_dev->dev.config[3 ] = (id &0xff00 ) >>8 ; + }fclose (f); dev->region_number = r;
Signature · 1 line
1.5.4.5
PATCH 3/3 kvm: emulate command register for SRIOV virtual function
In reply to Sheng YangMMIO enable byte would be checked when enabling virtual function, but in fact, the whole virtual function's command register is hard-wired to zero... So when guest read from command register it would only get 0, specially for MMIO enable bit.
Then we relay on QEmu to provide a reasonable command register content to guest.
--- qemu/hw/device-assignment.c | 13 ++++++++++++- qemu/hw/device-assignment.h | 1 + 2 files changed, 13 insertions(+), 1 deletions(-)
qemu/hw/device-assignment.c +12 -1
@@ -26,7 +26,10 @@* Copyright (C) 2008, IBM, Muli Ben-Yehuda ([email protected]) */ #include <stdio.h> +#include <unistd.h> #include <sys/io.h> +#include <sys/types.h> +#include <sys/stat.h> #include "qemu-kvm.h" #include "hw.h" #include "pc.h"
@@ -317,7 +320,7 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,ssize_t ret;AssignedDevice *pci_dev =container_of (d, AssignedDevice, dev); -if (address <0x4 || +if (address <0x4 || (pci_dev->need_emulate_cmd && address ==0x4 ) || (address >=0x10 && address <=0x24 ) || address ==0x34 || address ==0x3c || address ==0x3d ||pci_access_cap_config (d, address, len)) {
@@ -431,6 +434,7 @@ static int get_real_device(AssignedDevice *pci_dev, uint8_t r_bus,FILE *f;unsigned long long start, end, size, flags;unsigned long id; +struct stat statbuf;PCIRegion *rp;PCIDevRegions *dev = &pci_dev->real_device;
@@ -519,6 +523,13 @@ again: }fclose (f); +/* dealing with virtual function device */ +snprintf (name,sizeof (name),"%sphysfn/" , dir); +if (!stat (name, &statbuf)) + pci_dev->need_emulate_cmd =1 ; +else + pci_dev->need_emulate_cmd =0 ; + dev->region_number = r;return 0 ; }
qemu/hw/device-assignment.h +1 -0
@@ -94,6 +94,7 @@ typedef struct {void *msix_table_page;target_phys_addr_t msix_table_addr;int mmio_index; +int need_emulate_cmd; } AssignedDevice;typedef struct AssignedDevInfo AssignedDevInfo ;
Signature · 1 line
1.5.4.5