kvm: qemu: device-assignment: add_assigned_device() returns a pointer
PATCH 1/12 kvm: qemu: device-assignment: add_assigned_device() returns a pointer
add_assigned_device() returns a pointer, so don't check the return value is less than zero.
--- qemu/hw/ipf.c | 2 +- qemu/hw/pc.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
qemu/hw/ipf.c +1 -1
@@ -650,7 +650,7 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,if (kvm_enabled ()) {int i;for (i =0 ; i < assigned_devices_index; i++) { -if (add_assigned_device (assigned_devices[i]) <0 ) { +if (!add_assigned_device (assigned_devices[i])) {fprintf (stderr,"Warning: could not add assigned device %s\n" , assigned_devices[i]); }
qemu/hw/pc.c +1 -1
@@ -1187,7 +1187,7 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,if (kvm_enabled ()) {int i;for (i =0 ; i < assigned_devices_index; i++) { -if (add_assigned_device (assigned_devices[i]) <0 ) { +if (!add_assigned_device (assigned_devices[i])) {fprintf (stderr,"Warning: could not add assigned device %s\n" , assigned_devices[i]); }
Signature · 1 line
1.5.4.3
PATCH 2/12 kvm: qemu: device-assignment: introduce add_assigned_devices()
Merge two copies of the same code and cleanup with no functional changes.
fix error messages
--- qemu/hw/device-assignment.c | 32 +++++++++++++++++++++----------- qemu/hw/device-assignment.h | 2 +- qemu/hw/ipf.c | 16 ++-------------- qemu/hw/pc.c | 16 ++-------------- 4 files changed, 26 insertions(+), 40 deletions(-)
qemu/hw/device-assignment.c +21 -11
@@ -554,17 +554,6 @@ struct PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus)return &dev->dev; } -int init_all_assigned_devices (PCIBus *bus) -{ -struct AssignedDevInfo *adev; - - LIST_FOREACH(adev, &adev_head, next) -if (init_assigned_device (adev, bus) ==NULL ) -return -1 ; - -return 0 ; -} -/* * Syntax to assign device: *
@@ -619,3 +608,24 @@ bad:qemu_free (adev);return NULL ; } + +void add_assigned_devices (PCIBus *bus,const char **devices,int n_devices) +{ +int i; + +for (i =0 ; i < n_devices; i++) { +struct AssignedDevInfo *adev; + + adev =add_assigned_device (devices[i]); +if (!adev) { +fprintf (stderr,"Could not add assigned device %s\n" , devices[i]); +continue ; + } + +if (!init_assigned_device (adev, bus)) { +fprintf (stderr,"Failed to initialize assigned device %s\n" , + devices[i]); +exit (1 ); + } + } +}
qemu/hw/device-assignment.h +1 -1
@@ -96,7 +96,7 @@ struct AssignedDevInfo {PCIDevice *init_assigned_device (AssignedDevInfo *adev,PCIBus *bus);AssignedDevInfo *add_assigned_device (const char *arg); -int init_all_assigned_devices (PCIBus *bus); +void add_assigned_devices (PCIBus *bus,const char **devices,int n_devices);#define MAX_DEV_ASSIGN_CMDLINE 8
qemu/hw/ipf.c +2 -14
@@ -647,20 +647,8 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size, }#ifdef USE_KVM_DEVICE_ASSIGNMENT -if (kvm_enabled ()) { -int i; -for (i =0 ; i < assigned_devices_index; i++) { -if (!add_assigned_device (assigned_devices[i])) { -fprintf (stderr,"Warning: could not add assigned device %s\n" , - assigned_devices[i]); - } - } - -if (init_all_assigned_devices (pci_bus)) { -fprintf (stderr,"Failed to initialize assigned devices\n" ); -exit (1 ); - } - } +if (kvm_enabled ()) +add_assigned_devices (pci_bus, assigned_devices, assigned_devices_index);#endif /* USE_KVM_DEVICE_ASSIGNMENT */ }
qemu/hw/pc.c +2 -14
@@ -1184,20 +1184,8 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,virtio_balloon_init (pci_bus);#ifdef USE_KVM_DEVICE_ASSIGNMENT -if (kvm_enabled ()) { -int i; -for (i =0 ; i < assigned_devices_index; i++) { -if (!add_assigned_device (assigned_devices[i])) { -fprintf (stderr,"Warning: could not add assigned device %s\n" , - assigned_devices[i]); - } - } - -if (init_all_assigned_devices (pci_bus)) { -fprintf (stderr,"Failed to initialize assigned devices\n" ); -exit (1 ); - } - } +if (kvm_enabled ()) +add_assigned_devices (pci_bus, assigned_devices, assigned_devices_index);#endif /* USE_KVM_DEVICE_ASSIGNMENT */ }
Signature · 1 line
1.5.4.3
PATCH 3/12 kvm: qemu: device-assignment: exit if cmdline parsing fails
It's standard practice in qemu to exit if command line parameter fails, so do that here too.
--- qemu/hw/device-assignment.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
qemu/hw/device-assignment.c +1 -0
@@ -620,6 +620,7 @@ void add_assigned_devices(PCIBus *bus, const char **devices, int n_devices)if (!adev) {fprintf (stderr,"Could not add assigned device %s\n" , devices[i]);continue ; +exit (1 ); }if (!init_assigned_device (adev, bus)) {
Signature · 1 line
1.5.4.3
PATCH 4/12 kvm: qemu: device-assignment: unregister device if assignment fails
Fixes a segfault in assigned_dev_update_irq() if assigning a device previously failed.
--- qemu/hw/device-assignment.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
qemu/hw/device-assignment.c +4 -1
@@ -550,8 +550,11 @@ struct PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus) } adev->assigned_dev = dev; - out:return &dev->dev; + +out: +pci_unregister_device (&dev->dev); +return NULL ; }/*
Signature · 1 line
1.5.4.3
PATCH 5/12 kvm: qemu: device-assignment: fixup error reporting
If kvm_assign_pci_device(), there's no need for us to print two lines of error messages. The string translation of errno is very useful though, so include that in the message using strerror().
--- qemu/hw/device-assignment.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
qemu/hw/device-assignment.c +3 -4
@@ -540,12 +540,11 @@ struct PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus)if (r && !adev->disable_iommu) assigned_dev_data.flags |=KVM_DEV_ASSIGN_ENABLE_IOMMU ;#endif - + r =kvm_assign_pci_device (kvm_context, &assigned_dev_data);if (r <0 ) { -fprintf (stderr,"Could not notify kernel about " -"assigned device \"%s\"\n" , adev->name); -perror ("register_real_device" ); +fprintf (stderr,"Failed to assign device \"%s\" : %s\n" , + adev->name,strerror (-r)); goto out; }
Signature · 1 line
1.5.4.3
PATCH 6/12 kvm: qemu: device-assignment: cleanup irq assignment error messages
Replace perror() usage with sane error message.
--- qemu/hw/device-assignment.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
qemu/hw/device-assignment.c +4 -3
@@ -473,9 +473,10 @@ void assigned_dev_update_irq(PCIDevice *d) assigned_irq_data.host_irq = assigned_dev->real_device.irq; r =kvm_assign_irq (kvm_context, &assigned_irq_data);if (r <0 ) { -perror ("assigned_dev_update_irq" ); -fprintf (stderr,"Are you assigning a device " -"that shares IRQ with some other device?\n" ); +fprintf (stderr,"Failed to assign irq for \"%s\": %s\n" , + adev->name,strerror (-r)); +fprintf (stderr,"Perhaps you re you assigning a device " +"that shares IRQ with another device?\n" );pci_unregister_device (&assigned_dev->dev);/* FIXME: Delete node from list */ continue ;
Signature · 1 line
1.5.4.3
PATCH 7/12 kvm: qemu: device-assignment: remove device if irq assignment fails
Fix blatant issue where we leave a freed assigned device node on the list if irq assignment fails.
--- qemu/hw/device-assignment.c | 15 ++++++++++----- 1 files changed, 10 insertions(+), 5 deletions(-)
qemu/hw/device-assignment.c +10 -5
@@ -449,12 +449,14 @@ static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn)*/ void assigned_dev_update_irq (PCIDevice *d) { -int irq, r; -AssignedDevice *assigned_dev;AssignedDevInfo *adev; - LIST_FOREACH(adev, &adev_head, next) { - assigned_dev = adev->assigned_dev; + adev =LIST_FIRST (&adev_head); +while (adev) { +AssignedDevInfo *next =LIST_NEXT (adev, next); +AssignedDevice *assigned_dev = adev->assigned_dev; +int irq, r; + irq =pci_map_irq (&assigned_dev->dev, assigned_dev->intpin); irq =piix_get_irq (irq);
@@ -477,12 +479,15 @@ void assigned_dev_update_irq(PCIDevice *d) adev->name, strerror(-r));fprintf (stderr,"Perhaps you re you assigning a device " "that shares IRQ with another device?\n" ); +LIST_REMOVE (adev, next);pci_unregister_device (&assigned_dev->dev); -/* FIXME: Delete node from list */ + adev = next;continue ; } assigned_dev->girq = irq; } + + adev = next; } }
Signature · 1 line
1.5.4.3
PATCH 8/12 kvm: qemu: device-assignment: introduce free_assigned_device()
Add a function to fully free the AssignedDevice and AssignedDevInfo and remove from the list.
--- qemu/hw/device-assignment.c | 16 ++++++++++++++-- qemu/hw/device-assignment.h | 1 + 2 files changed, 15 insertions(+), 2 deletions(-)
qemu/hw/device-assignment.c +14 -2
@@ -439,6 +439,19 @@ again: static LIST_HEAD(, AssignedDevInfo) adev_head; +void free_assigned_device (AssignedDevInfo *adev) +{ +AssignedDevice *dev = adev->assigned_dev; + +if (dev) { +pci_unregister_device (&dev->dev); + adev->assigned_dev = dev =NULL ; + } + +LIST_REMOVE (adev, next); +qemu_free (adev); +} +static uint32_t calc_assigned_dev_id (uint8_t bus,uint8_t devfn) {return (uint32_t )bus <<8 | (uint32_t )devfn;
@@ -479,8 +492,7 @@ void assigned_dev_update_irq(PCIDevice *d) adev->name, strerror(-r));fprintf (stderr,"Perhaps you re you assigning a device " "that shares IRQ with another device?\n" ); -LIST_REMOVE (adev, next); -pci_unregister_device (&assigned_dev->dev); +free_assigned_device (adev); adev = next;continue ; }
qemu/hw/device-assignment.h +1 -0
@@ -94,6 +94,7 @@ struct AssignedDevInfo {int disable_iommu; }; +void free_assigned_device (AssignedDevInfo *adev);PCIDevice *init_assigned_device (AssignedDevInfo *adev,PCIBus *bus);AssignedDevInfo *add_assigned_device (const char *arg);void add_assigned_devices (PCIBus *bus,const char **devices,int n_devices);
Signature · 1 line
1.5.4.3
PATCH 9/12 kvm: qemu: device-assignment: free device if hotplug fails
--- qemu/hw/device-hotplug.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
qemu/hw/device-hotplug.c +5 -0
@@ -48,6 +48,11 @@ static PCIDevice *qemu_system_hot_assign_device(const char *opts, int bus_nr) } ret =init_assigned_device (adev, pci_bus); +if (ret ==NULL ) { +term_printf ("Failed to assign device\n" ); +free_assigned_device (adev); +return NULL ; + } term_printf("Registered host PCI device %02x:%02x.%1x " "(\"%s\") as guest device %02x:%02x.%1x\n",
Signature · 1 line
1.5.4.3
PATCH 10/12 kvm: qemu: device-assignment: close PCIDevRegions::config_fd
--- qemu/hw/device-assignment.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
qemu/hw/device-assignment.c +5 -0
@@ -444,6 +444,11 @@ void free_assigned_device(AssignedDevInfo *adev)AssignedDevice *dev = adev->assigned_dev;if (dev) { +if (dev->real_device.config_fd) { +close (dev->real_device.config_fd); + dev->real_device.config_fd =0 ; + } +pci_unregister_device (&dev->dev); adev->assigned_dev = dev =NULL ; }
Signature · 1 line
1.5.4.3
PATCH 11/12 kvm: qemu: device-assignment: munmap() mmio regions
Undo the effects of assigned_dev_register_regions(); made unneccessarily difficult by the twisty data structures.
--- qemu/hw/device-assignment.c | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)
qemu/hw/device-assignment.c +20 -0
@@ -330,6 +330,7 @@ static int assigned_dev_register_regions(PCIRegion *io_regions, cur_region->resource_fd, (off_t) 0);if (pci_dev->v_addrs[i].u.r_virtbase ==MAP_FAILED ) { + pci_dev->v_addrs[i].u.r_virtbase =NULL ;fprintf (stderr,"%s: Error: Couldn't mmap 0x%x!" "\n" , __func__, (uint32_t ) (cur_region->base_addr));
@@ -444,6 +445,25 @@ void free_assigned_device(AssignedDevInfo *adev)AssignedDevice *dev = adev->assigned_dev;if (dev) { +int i; + +for (i =0 ; i < dev->real_device.region_number; i++) { +PCIRegion *pci_region = &dev->real_device.regions[i]; +AssignedDevRegion *region = &dev->v_addrs[i]; + +if (!pci_region->valid || !(pci_region->type &IORESOURCE_MEM )) +continue ; + +if (region->u.r_virtbase) { +int ret =munmap (region->u.r_virtbase, + (pci_region->size +0xFFF ) &0xFFFFF000 ); +if (ret !=0 ) +fprintf (stderr, +"Failed to unmap assigned device region: %s\n" , +strerror (errno)); + } + } +if (dev->real_device.config_fd) {close (dev->real_device.config_fd); dev->real_device.config_fd =0 ;
Signature · 1 line
1.5.4.3
PATCH 12/12 kvm: qemu: device-assignment: init_assigned_device() error handling
Associate the AssignedDevice with the AssignedDevInfo as soon as we have allocated it and don't try and free it in init_assigned_device() if an error occurs.
This ensures that the mmio region is unmapped by free_assigned_device() if init_assigned_device() fails.
--- qemu/hw/device-assignment.c | 13 +++++-------- 1 files changed, 5 insertions(+), 8 deletions(-)
qemu/hw/device-assignment.c +5 -8
@@ -548,17 +548,19 @@ struct PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus)return NULL ; } + adev->assigned_dev = dev; +if (get_real_device (dev, adev->bus, adev->dev, adev->func)) {fprintf (stderr,"%s: Error: Couldn't get real device (%s)!\n" , __func__, adev->name); - goto out; +return NULL ; }/* handle real device's MMIO/PIO BARs */ if (assigned_dev_register_regions (dev->real_device.regions, dev->real_device.region_number, dev)) - goto out; +return NULL ;/* handle interrupt routing */ e_device = (dev->dev.devfn >>3 ) &0x1f ;
@@ -588,15 +590,10 @@ struct PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus)if (r <0 ) {fprintf (stderr,"Failed to assign device \"%s\" : %s\n" , adev->name,strerror (-r)); - goto out; +return NULL ; } - adev->assigned_dev = dev;return &dev->dev; - -out: -pci_unregister_device (&dev->dev); -return NULL ; }/*
Signature · 1 line
1.5.4.3
Re: [PATCH 06/12] kvm: qemu: device-assignment: cleanup irq assignment error messages
In reply to Mark McLoughlinQuoted 12 lines · Mark McLoughlin
Replace
perror()usage with sane error message.Signed-off-by Mark McLoughlin <[email protected]>--- qemu/hw/device-assignment.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)qemu/hw/device-assignment.c +0
-0
qemu/hw/device-assignment.c void assigned_dev_update_irq(PCIDevice *d) · line 478
Quoted 9 lines
qemu/hw/device-assignment.c +3
-3@@ -473,9 +473,10 @@ void assigned_dev_update_irq(PCIDevice *d) assigned_irq_data.host_irq = assigned_dev->real_device.irq; r =kvm_assign_irq (kvm_context, &assigned_irq_data);if (r <0 ) { -perror ("assigned_dev_update_irq" ); -fprintf (stderr,"Are you assigning a device " -"that shares IRQ with some other device?\n" ); +fprintf (stderr,"Failed to assign irq for \"%s\": %s\n" , + adev->name,strerror (-r)); + fprintf(stderr, "Perhaps you re you assigning a device "
Typo.
qemu/hw/device-assignment.c void assigned_dev_update_irq(PCIDevice *d) · line 482
Quoted 4 lines
qemu/hw/device-assignment.c +1
-0+ "that shares IRQ with another device?\n");pci_unregister_device (&assigned_dev->dev);/* FIXME: Delete node from list */ continue ;
-John
Re: [PATCH 06/12] kvm: qemu: device-assignment: cleanup irq assignment error messages
Quoted 10 lines · John Rousseau
Quoted 8 lines
qemu/hw/device-assignment.c +1
-3-perror ("assigned_dev_update_irq" ); -fprintf (stderr,"Are you assigning a device " -"that shares IRQ with some other device?\n" ); + fprintf(stderr, "Failed to assign irq for \"%s\":%s\n",
(quoted diff) +2
-0+ adev->name, strerror(-r)); + fprintf(stderr, "Perhaps you re you assigning adevice "
Typo.
Thanks, updated.
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH 1/1 kvm: qemu: device-assignment: really exit if cmdline parsing fails
In reply to Mark McLoughlinQuoted 13 lines · Mark McLoughlin
It's standard practice in qemu to exit if command line parameter fails, so do that here too.
Signed-off-by Mark McLoughlin <[email protected]>--- qemu/hw/device-assignment.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)qemu/hw/device-assignment.c +0
-0
qemu/hw/device-assignment.c void add_assigned_devices(PCIBus *bus, const char **devices, int n_devices) · line 623
Quoted 4 lines
qemu/hw/device-assignment.c +1
-0@@ -620,6 +620,7 @@ void add_assigned_devices(PCIBus *bus, const char **devices, int n_devices)if (!adev) {fprintf (stderr,"Could not add assigned device %s\n" , devices[i]);continue ; +exit (1 );
Um, that's a rather embarrassing thinko.
Cheers,
Mark.
From: Mark McLoughlin <[email protected]>
Subject: [PATCH] kvm: qemu: device-assignment: really exit if cmdline parsing fails
--- qemu/hw/device-assignment.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
qemu/hw/device-assignment.c +0 -1
@@ -668,7 +668,6 @@ void add_assigned_devices(PCIBus *bus, const char **devices, int n_devices) adev =add_assigned_device (devices[i]);if (!adev) {fprintf (stderr,"Could not add assigned device %s\n" , devices[i]); -continue ;exit (1 ); }
Signature · 1 line
1.5.4.3
Re: [PATCH] kvm: qemu: device-assignment: really exit if cmdline parsing fails
Quoted 7 lines · Mark McLoughlin
Quoted 4 lines
fprintf(stderr, "Could not add assigned device %s\n", devices[i]); continue;+ exit(1);
Um, that's a rather embarrassing thinko.
For the reviewer as well; applied.
Signature · 1 line
error compiling committee.c: too many arguments to function
Hi Avi,
Sorry ... forgot to 'git send-email --compose' :-)
The patchset is just a bunch of fixes to the device-assignment error handling code. Shouldn't be anything too controversial there.
Cheers,
Mark.
Quoted 7 lines · Mark McLoughlin
Hi Avi,
Sorry ... forgot to 'git send-email --compose' :-)
The patchset is just a bunch of fixes to the device-assignment error handling code. Shouldn't be anything too controversial there.
No, all applied. Patch 2 looks like two patches munged together; I applied it anyway as it wasn't worth the effort to respin.
Signature · 1 line
error compiling committee.c: too many arguments to function