Skip to contentSkip to conversation

kvm: qemu: device-assignment: add_assigned_device() returns a pointer

PATCH In review Mark McLoughlin 3 participants 18 messages Nov 28, 2008 – Dec 10, 2008 KVM

lore
Share & tools
lore
#0 lore raw

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
index 37f2de7..c0ac9eb 100644
@@ -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
index 7d296c4..6f3339f 100644
@@ -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
#1 lore raw

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
index 9a790c6..eb2a73a 100644
@@ -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
index d6caa67..5a01d98 100644
@@ -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
index c0ac9eb..3e24c98 100644
@@ -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
index 6f3339f..6de460c 100644
@@ -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
#2 lore raw

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
index eb2a73a..8fbd66c 100644
@@ -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
#3 lore raw

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
index 8fbd66c..b5cf6ee 100644
@@ -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
#4 lore raw

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
index b5cf6ee..2b2ef68 100644
@@ -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
#5 lore raw

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
index 2b2ef68..b39617a 100644
@@ -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
#6 lore raw

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
index b39617a..fde17ac 100644
@@ -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
#7 lore raw

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
index fde17ac..2836059 100644
@@ -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
index 5a01d98..c8c47d3 100644
@@ -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
#8 lore raw

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
index ba1b161..0bcac60 100644
@@ -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
#9 lore raw

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
index 2836059..5f803b1 100644
@@ -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
#10 lore raw

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
index 5f803b1..1964fbc 100644
@@ -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
#11 lore raw

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
index 1964fbc..70640c9 100644
@@ -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
#12 lore raw

Re: [PATCH 06/12] kvm: qemu: device-assignment: cleanup irq assignment error messages

In reply to Mark McLoughlin
Quoted 12 lines · Mark McLoughlin

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 +0 -0
index 2b2ef68..b39617a 100644

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

#13 lore raw

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 a 

device "

Typo.

Thanks, updated.

Signature · 1 line
error compiling committee.c: too many arguments to function
#14 lore raw

PATCH 1/1 kvm: qemu: device-assignment: really exit if cmdline parsing fails

In reply to Mark McLoughlin
Quoted 13 lines · Mark McLoughlin

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 +0 -0
index eb2a73a..8fbd66c 100644

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
index 4a38a22..7a66665 100644
@@ -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
#15 lore raw

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
#16 lore raw
In reply to 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.

Cheers,
Mark.

#17 lore raw
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