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 <[EMAIL PROTECTED]>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
