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

[Mark McLoughlin <markmc@redhat.com>]

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.