[PATCH 4/4] [v2] kvm: qemu: Assign irq in init_assigned_device

[PATCH 4/4] [v2] kvm: qemu: Assign irq in init_assigned_device

[Han, Weidong]

assign_dev_update_irqs may not be invoked when hot add a device, so need to assign irq after assign device in init_assigned_device.

Additionally, clean up assign_dev_update_irqs code, and free the assigned device in init_assigned_device when it fails.

Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 qemu/hw/device-assignment.c |  135 +++++++++++++++++++++++++-----------------
 qemu/hw/device-hotplug.c    |    1 -
 2 files changed, 80 insertions(+), 56 deletions(-)

diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
index 03a52e6..160f001 100644
--- a/qemu/hw/device-assignment.c
+++ b/qemu/hw/device-assignment.c
@@ -490,6 +490,68 @@ static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn)
     return (uint32_t)bus << 8 | (uint32_t)devfn;
 }
 
+static int assign_device(AssignedDevInfo *adev)
+{
+    struct kvm_assigned_pci_dev assigned_dev_data;
+    AssignedDevice *dev = adev->assigned_dev;
+    int r;
+
+    memset(&assigned_dev_data, 0, sizeof(assigned_dev_data));
+    assigned_dev_data.assigned_dev_id  =
+	calc_assigned_dev_id(dev->h_busnr, dev->h_devfn);
+    assigned_dev_data.busnr = dev->h_busnr;
+    assigned_dev_data.devfn = dev->h_devfn;
+
+#ifdef KVM_CAP_IOMMU
+    /* We always enable the IOMMU if present
+     * (or when not disabled on the command line)
+     */
+    r = kvm_check_extension(kvm_context, KVM_CAP_IOMMU);
+    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, "Failed to assign device \"%s\" : %s\n",
+                adev->name, strerror(-r));
+    return r;
+}
+
+static int assign_irq(AssignedDevInfo *adev)
+{
+    struct kvm_assigned_irq assigned_irq_data;
+    AssignedDevice *dev = adev->assigned_dev;
+    int irq, r = 0;
+
+    irq = pci_map_irq(&dev->dev, dev->intpin);
+    irq = piix_get_irq(irq);
+
+#ifdef TARGET_IA64
+    irq = ipf_map_irq(&dev->dev, irq);
+#endif
+
+    if (dev->girq == irq)
+        return r;
+
+    memset(&assigned_irq_data, 0, sizeof(assigned_irq_data));
+    assigned_irq_data.assigned_dev_id =
+        calc_assigned_dev_id(dev->h_busnr, dev->h_devfn);
+    assigned_irq_data.guest_irq = irq;
+    assigned_irq_data.host_irq = dev->real_device.irq;
+    r = kvm_assign_irq(kvm_context, &assigned_irq_data);
+    if (r < 0) {
+        fprintf(stderr, "Failed to assign irq for \"%s\": %s\n",
+                adev->name, strerror(-r));
+        fprintf(stderr, "Perhaps you are assigning a device "
+                "that shares an IRQ with another device?\n");
+        return r;
+    }
+
+    dev->girq = irq;
+    return r;
+}
+
 /* The pci config space got updated. Check if irq numbers have changed
  * for our devices
  */
@@ -499,40 +561,13 @@ void assigned_dev_update_irqs()
 
     adev = LIST_FIRST(&adev_head);
     while (adev) {
-        AssignedDevInfo *next = LIST_NEXT(adev, next);
-        AssignedDevice *assigned_dev = adev->assigned_dev;
-        int irq, r;
+        int r;
+            
+        r = assign_irq(adev);
+        if (r < 0)
+            free_assigned_device(adev);
 
-        irq = pci_map_irq(&assigned_dev->dev, assigned_dev->intpin);
-        irq = piix_get_irq(irq);
-
-#ifdef TARGET_IA64
-	irq = ipf_map_irq(&assigned_dev->dev, irq);
-#endif
-
-        if (irq != assigned_dev->girq) {
-            struct kvm_assigned_irq assigned_irq_data;
-
-            memset(&assigned_irq_data, 0, sizeof(assigned_irq_data));
-            assigned_irq_data.assigned_dev_id  =
-                calc_assigned_dev_id(assigned_dev->h_busnr,
-                                     (uint8_t) assigned_dev->h_devfn);
-            assigned_irq_data.guest_irq = irq;
-            assigned_irq_data.host_irq = assigned_dev->real_device.irq;
-            r = kvm_assign_irq(kvm_context, &assigned_irq_data);
-            if (r < 0) {
-                fprintf(stderr, "Failed to assign irq for \"%s\": %s\n",
-                        adev->name, strerror(-r));
-                fprintf(stderr, "Perhaps you are assigning a device "
-                        "that shares an IRQ with another device?\n");
-                free_assigned_device(adev);
-                adev = next;
-                continue;
-            }
-            assigned_dev->girq = irq;
-        }
-
-        adev = next;
+        adev = LIST_NEXT(adev, next);
     }
 }
 
@@ -561,14 +596,14 @@ struct PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus)
     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);
-        return NULL;
+        goto out;
     }
 
     /* handle real device's MMIO/PIO BARs */
     if (assigned_dev_register_regions(dev->real_device.regions,
                                       dev->real_device.region_number,
                                       dev))
-        return NULL;
+        goto out;
 
     /* handle interrupt routing */
     e_device = (dev->dev.devfn >> 3) & 0x1f;
@@ -579,29 +614,19 @@ struct PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus)
     dev->h_busnr = adev->bus;
     dev->h_devfn = PCI_DEVFN(adev->dev, adev->func);
 
-    memset(&assigned_dev_data, 0, sizeof(assigned_dev_data));
-    assigned_dev_data.assigned_dev_id  =
-	calc_assigned_dev_id(dev->h_busnr, (uint32_t)dev->h_devfn);
-    assigned_dev_data.busnr = dev->h_busnr;
-    assigned_dev_data.devfn = dev->h_devfn;
+    r = assign_device(adev);
+    if (r < 0)
+        goto out;
 
-#ifdef KVM_CAP_IOMMU
-    /* We always enable the IOMMU if present
-     * (or when not disabled on the command line)
-     */
-    r = kvm_check_extension(kvm_context, KVM_CAP_IOMMU);
-    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, "Failed to assign device \"%s\" : %s\n",
-                adev->name, strerror(-r));
-	return NULL;
-    }
+    r = assign_irq(adev);
+    if (r < 0)
+        goto out;
 
     return &dev->dev;
+
+out:
+    free_assigned_device(adev);
+    return NULL;
 }
 
 /*
diff --git a/qemu/hw/device-hotplug.c b/qemu/hw/device-hotplug.c
index 0bcac60..dd67179 100644
--- a/qemu/hw/device-hotplug.c
+++ b/qemu/hw/device-hotplug.c
@@ -50,7 +50,6 @@ 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;
     }
 
-- 
1.6.0.4

Re: [PATCH 4/4] [v2] kvm: qemu: Assign irq in init_assigned_device

[Mark McLoughlin]

On Wed, 2008-12-10 at 21:23 +0800, Han, Weidong wrote:
> diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
> index 03a52e6..160f001 100644
> --- a/qemu/hw/device-assignment.c
> +++ b/qemu/hw/device-assignment.c
> @@ -499,40 +561,13 @@ void assigned_dev_update_irqs()
>  
>      adev = LIST_FIRST(&adev_head);
>      while (adev) {
> -        AssignedDevInfo *next = LIST_NEXT(adev, next);
...
> +        r = assign_irq(adev);
> +        if (r < 0)
> +            free_assigned_device(adev);
...
> -        adev = next;
> +        adev = LIST_NEXT(adev, next);
>      }
>  }

You're introducing the "use after free" issue here again.

Cheers,
Mark.