* [PATCH 4/4] kvm: qemu: Assign irq in init_assigned_device
@ 2008-12-10 9:45 Han, Weidong
2008-12-10 10:20 ` Mark McLoughlin
0 siblings, 1 reply; 3+ messages in thread
From: Han, Weidong @ 2008-12-10 9:45 UTC (permalink / raw)
To: 'Avi Kivity'; +Cc: kvm@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 4876 bytes --]
assign_dev_update_irq may not be invoked when hot add a device, so need to assign irq after assign device in init_assigned_device.
Signed-off-by: Weidong Han <weidong.han@intel.com>
---
qemu/hw/device-assignment.c | 99 ++++++++++++++++++++++++++++--------------
1 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
index 20618a4..557beb8 100644
--- a/qemu/hw/device-assignment.c
+++ b/qemu/hw/device-assignment.c
@@ -490,6 +490,65 @@ 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
+
+ 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
*/
@@ -511,20 +570,8 @@ void assigned_dev_update_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);
+ r = assign_irq(adev);
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;
@@ -579,27 +626,13 @@ 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;
-
-#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 = assign_device(adev);
+ if (r < 0)
+ return NULL;
- 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)
+ return NULL;
return &dev->dev;
}
--
1.6.0.4
[-- Attachment #2: 0004-Assign-irq-in-init_assigned_device.patch --]
[-- Type: application/octet-stream, Size: 4946 bytes --]
From 04ee5a808ad900c3768061d4bbd06f83c4dd2a03 Mon Sep 17 00:00:00 2001
From: Weidong Han <weidong.han@intel.com>
Date: Wed, 10 Dec 2008 17:23:45 +0800
Subject: [PATCH] Assign irq in init_assigned_device
assign_dev_update_irq may not be invoked when hot add a device, so need to assign irq after assign device in init_assigned_device.
Signed-off-by: Weidong Han <weidong.han@intel.com>
---
qemu/hw/device-assignment.c | 99 ++++++++++++++++++++++++++++--------------
1 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
index 20618a4..557beb8 100644
--- a/qemu/hw/device-assignment.c
+++ b/qemu/hw/device-assignment.c
@@ -490,6 +490,65 @@ 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
+
+ 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
*/
@@ -511,20 +570,8 @@ void assigned_dev_update_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);
+ r = assign_irq(adev);
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;
@@ -579,27 +626,13 @@ 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;
-
-#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 = assign_device(adev);
+ if (r < 0)
+ return NULL;
- 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)
+ return NULL;
return &dev->dev;
}
--
1.6.0.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 4/4] kvm: qemu: Assign irq in init_assigned_device
2008-12-10 9:45 [PATCH 4/4] kvm: qemu: Assign irq in init_assigned_device Han, Weidong
@ 2008-12-10 10:20 ` Mark McLoughlin
2008-12-10 12:18 ` Han, Weidong
0 siblings, 1 reply; 3+ messages in thread
From: Mark McLoughlin @ 2008-12-10 10:20 UTC (permalink / raw)
To: Han, Weidong; +Cc: 'Avi Kivity', kvm@vger.kernel.org
On Wed, 2008-12-10 at 17:45 +0800, Han, Weidong wrote:
> assign_dev_update_irq may not be invoked when hot add a device, so
> need to assign irq after assign device in init_assigned_device.
Makes sense, but ...
> Signed-off-by: Weidong Han <weidong.han@intel.com>
> ---
> qemu/hw/device-assignment.c | 99 ++++++++++++++++++++++++++++--------------
> 1 files changed, 66 insertions(+), 33 deletions(-)
>
> diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
> index 20618a4..557beb8 100644
> --- a/qemu/hw/device-assignment.c
> +++ b/qemu/hw/device-assignment.c
> @@ -490,6 +490,65 @@ static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn)
> return (uint32_t)bus << 8 | (uint32_t)devfn;
> }
>
...
> +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 you added here:
if (irq == dev->girq)
return;
then ...
> + 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
> */
> @@ -511,20 +570,8 @@ void assigned_dev_update_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);
> + r = assign_irq(adev);
> 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;
.... you end up with:
void assigned_dev_update_irq(PCIDevice *d)
{
AssignedDevInfo *adev;
adev = LIST_FIRST(&adev_head);
while (adev) {
AssignedDevInfo *next = LIST_NEXT(adev, next);
int r;
r = assign_irq(adev);
if (r < 0)
free_assigned_device(adev);
adev = next;
}
which is much nicer.
> @@ -579,27 +626,13 @@ 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;
> -
> -#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 = assign_device(adev);
> + if (r < 0)
> + return NULL;
>
> - 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)
> + return NULL;
Hmm, why are we doing no cleanup if these fail?
Cheers,
Mark.
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH 4/4] kvm: qemu: Assign irq in init_assigned_device
2008-12-10 10:20 ` Mark McLoughlin
@ 2008-12-10 12:18 ` Han, Weidong
0 siblings, 0 replies; 3+ messages in thread
From: Han, Weidong @ 2008-12-10 12:18 UTC (permalink / raw)
To: 'Mark McLoughlin'
Cc: 'Avi Kivity', 'kvm@vger.kernel.org'
Mark McLoughlin wrote:
> On Wed, 2008-12-10 at 17:45 +0800, Han, Weidong wrote:
>
>> assign_dev_update_irq may not be invoked when hot add a device, so
>> need to assign irq after assign device in init_assigned_device.
>
> Makes sense, but ...
>
>> Signed-off-by: Weidong Han <weidong.han@intel.com>
>> ---
>> qemu/hw/device-assignment.c | 99
>> ++++++++++++++++++++++++++++-------------- 1 files changed, 66
>> insertions(+), 33 deletions(-)
>>
>> diff --git a/qemu/hw/device-assignment.c
>> b/qemu/hw/device-assignment.c
>> index 20618a4..557beb8 100644
>> --- a/qemu/hw/device-assignment.c
>> +++ b/qemu/hw/device-assignment.c
>> @@ -490,6 +490,65 @@ static uint32_t calc_assigned_dev_id(uint8_t
>> bus, uint8_t devfn) return (uint32_t)bus << 8 | (uint32_t)devfn;
>> }
>>
> ...
>> +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 you added here:
>
> if (irq == dev->girq)
> return;
>
> then ...
>
>> + 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
>> */
>> @@ -511,20 +570,8 @@ void assigned_dev_update_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);
>> + r = assign_irq(adev);
>> 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;
>
> .... you end up with:
>
> void assigned_dev_update_irq(PCIDevice *d)
> {
> AssignedDevInfo *adev;
>
> adev = LIST_FIRST(&adev_head);
> while (adev) {
> AssignedDevInfo *next = LIST_NEXT(adev, next);
> int r;
>
> r = assign_irq(adev);
> if (r < 0)
> free_assigned_device(adev);
>
> adev = next;
> }
>
> which is much nicer.
I will clean up it.
>
>> @@ -579,27 +626,13 @@ 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;
>> -
>> -#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 = assign_device(adev);
>> + if (r < 0)
>> + return NULL;
>>
>> - 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)
>> + return NULL;
>
> Hmm, why are we doing no cleanup if these fail?
>
Currently, free_assigned_device() will be called when init_assigned_device() fails (e.g. in qemu_system_hot_assign_device()). But I think it is more reasonable to do cleanup in init_assigned_device().
Regards,
Weidong
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-12-10 12:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-10 9:45 [PATCH 4/4] kvm: qemu: Assign irq in init_assigned_device Han, Weidong
2008-12-10 10:20 ` Mark McLoughlin
2008-12-10 12:18 ` Han, Weidong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.