Re: [Qemu-devel] [RFC PATCH] virtio-mmio: support for multiple irqs

[Shannon Zhao]

On 2014/11/6 19:09, Michael S. Tsirkin wrote:
> On Thu, Nov 06, 2014 at 05:54:54PM +0800, Shannon Zhao wrote:
>> On 2014/11/6 17:34, Michael S. Tsirkin wrote:
>>> On Tue, Nov 04, 2014 at 05:35:12PM +0800, Shannon Zhao wrote:
>>>> As the current virtio-mmio only support single irq,
>>>> so some advanced features such as vhost-net with irqfd
>>>> are not supported. And the net performance is not
>>>> the best without vhost-net and irqfd supporting.
>>>>
>>>> This patch support virtio-mmio to request multiple
>>>> irqs like virtio-pci. With this patch and qemu assigning
>>>> multiple irqs for virtio-mmio device, it's ok to use
>>>> vhost-net with irqfd on arm/arm64.
>>>>
>>>> As arm doesn't support msi-x now, we use GSI for
>>>> multiple irq. In this patch we use "vm_try_to_find_vqs"
>>>> to check whether multiple irqs are supported like
>>>> virtio-pci.
>>>>
>>>> Is this the right direction? is there other ways to
>>>> make virtio-mmio support multiple irq? Hope for feedback.
>>>> Thanks.
>>>>
>>>> Signed-off-by: Shannon Zhao <address@hidden>
>>>
>>>
>>> So how does guest discover whether host device supports multiple IRQs?
>>
>> Guest uses vm_try_to_find_vqs to check whether it can get multiple IRQs
>> like virtio-pci uses vp_try_to_find_vqs. And within function
>> vm_request_multiple_irqs, guest check whether the number of IRQs host
>> device gives is equal to the number we want.
> 
> OK but how does host specify the number of IRQs for a device?
> for pci this is done through the MSI-X capability register.
> 
For example, qemu command line of a typical virtio-net device on arm is as 
followed:

        -device virtio-net-device,netdev=net0,mac="52:54:00:12:34:55",vectors=3 
\
        -netdev 
type=tap,id=net0,script=/home/v2r1/qemu-ifup,downscript=no,vhost=on,queues=1 \

When qemu create a virtio-mmio device, qemu get the number of IRQs through the 
"vectors"
and according to this qemu will connect "vectors" IRQ lines between virtio-mmio 
and GIC.

The patch about how qemu create a virtio-mmio device with multiple IRQs is as 
followed:

        driver = qemu_opt_get(opts, "driver");
        if (strncmp(driver, "virtio-", 7) != 0) {
            continue;
        }
        vectors = qemu_opt_get(opts, "vectors");
        if (vectors == NULL) {
            nvector = 1;
        } else {
            nvector = atoi(vectors);
        }

        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;
        dev = qdev_create(NULL, "virtio-mmio");
        qdev_prop_set_uint32(dev, "nvector", nvector);
        s = SYS_BUS_DEVICE(dev);
        qdev_init_nofail(dev);
        if (base != (hwaddr)-1) {
            sysbus_mmio_map(s, 0, base);
        }

/* This is the code that qemu connect multiple IRQs between virtio-mmio and GIC 
*/
        for (n = 0; n < nvector; n++) {
            sysbus_connect_irq(s, n, pic[irq+n]);
        }

        char *nodename;

        nodename = g_strdup_printf("/address@hidden" PRIx64, base);
        qemu_fdt_add_subnode(vbi->fdt, nodename);
        qemu_fdt_setprop_string(vbi->fdt, nodename,
                                "compatible", "virtio,mmio");
        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
                                     2, base, 2, size);
        int qdt_size = nvector * sizeof(uint32_t) * 3;
        uint32_t *qdt_tmp = (uint32_t *)malloc(qdt_size);

/* This is the code that qemu prepare the dts for virtio-mmio with multiple 
IRQs */
        for (n = 0; n < nvector; n++) {
            *(qdt_tmp+n*3) = cpu_to_be32(GIC_FDT_IRQ_TYPE_SPI);
            *(qdt_tmp+n*3+1) = cpu_to_be32(irq+n);
            *(qdt_tmp+n*3+2) = cpu_to_be32(GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
        }
        qemu_fdt_setprop(vbi->fdt, nodename, "interrupts", qdt_tmp, qdt_size);
        g_free(nodename);



>>      for (i = 0; i < nirqs; i++) {
>>              irq = platform_get_irq(vm_dev->pdev, i);
>>              if (irq == -ENXIO)
>>                      goto error;
>>      }
>>
>> If we can't get the expected number of IRQs, return error and this try
>> fails. Then guest will try two IRQS and single IRQ like virtio-pci.
>>
>>> Could you please document the new interface?
>>> E.g. send a patch for virtio spec.
>>
>> Ok, I'll send it later. Thank you very much :)
>>
>> Shannon
>>
>>> I think this really should be controlled by hypervisor, per device.
>>> I'm also tempted to make this a virtio 1.0 only feature.
>>>
>>>
>>>
>>>> ---
>>>>  drivers/virtio/virtio_mmio.c |  234 
>>>> ++++++++++++++++++++++++++++++++++++------
>>>>  1 files changed, 203 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>>>> index c600ccf..2b7d935 100644
>>>> --- a/drivers/virtio/virtio_mmio.c
>>>> +++ b/drivers/virtio/virtio_mmio.c
>>>> @@ -122,6 +122,15 @@ struct virtio_mmio_device {
>>>>    /* a list of queues so we can dispatch IRQs */
>>>>    spinlock_t lock;
>>>>    struct list_head virtqueues;
>>>> +
>>>> +  /* multiple irq support */
>>>> +  int single_irq_enabled;
>>>> +  /* Number of available irqs */
>>>> +  unsigned num_irqs;
>>>> +  /* Used number of irqs */
>>>> +  int used_irqs;
>>>> +  /* Name strings for interrupts. */
>>>> +  char (*vm_vq_names)[256];
>>>>  };
>>>>  
>>>>  struct virtio_mmio_vq_info {
>>>> @@ -229,33 +238,53 @@ static bool vm_notify(struct virtqueue *vq)
>>>>    return true;
>>>>  }
>>>>  
>>>> +/* Handle a configuration change: Tell driver if it wants to know. */
>>>> +static irqreturn_t vm_config_changed(int irq, void *opaque)
>>>> +{
>>>> +  struct virtio_mmio_device *vm_dev = opaque;
>>>> +  struct virtio_driver *vdrv = container_of(vm_dev->vdev.dev.driver,
>>>> +          struct virtio_driver, driver);
>>>> +
>>>> +  if (vdrv && vdrv->config_changed)
>>>> +          vdrv->config_changed(&vm_dev->vdev);
>>>> +  return IRQ_HANDLED;
>>>> +}
>>>> +
>>>>  /* Notify all virtqueues on an interrupt. */
>>>> -static irqreturn_t vm_interrupt(int irq, void *opaque)
>>>> +static irqreturn_t vm_vring_interrupt(int irq, void *opaque)
>>>>  {
>>>>    struct virtio_mmio_device *vm_dev = opaque;
>>>>    struct virtio_mmio_vq_info *info;
>>>> -  struct virtio_driver *vdrv = container_of(vm_dev->vdev.dev.driver,
>>>> -                  struct virtio_driver, driver);
>>>> -  unsigned long status;
>>>> +  irqreturn_t ret = IRQ_NONE;
>>>>    unsigned long flags;
>>>> +
>>>> +  spin_lock_irqsave(&vm_dev->lock, flags);
>>>> +  list_for_each_entry(info, &vm_dev->virtqueues, node) {
>>>> +          if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
>>>> +                  ret = IRQ_HANDLED;
>>>> +  }
>>>> +  spin_unlock_irqrestore(&vm_dev->lock, flags);
>>>> +
>>>> +  return ret;
>>>> +}
>>>> +
>>>> +/* Notify all virtqueues and handle a configuration
>>>> + * change on an interrupt. */
>>>> +static irqreturn_t vm_interrupt(int irq, void *opaque)
>>>> +{
>>>> +  struct virtio_mmio_device *vm_dev = opaque;
>>>> +  unsigned long status;
>>>>    irqreturn_t ret = IRQ_NONE;
>>>>  
>>>>    /* Read and acknowledge interrupts */
>>>>    status = readl(vm_dev->base + VIRTIO_MMIO_INTERRUPT_STATUS);
>>>>    writel(status, vm_dev->base + VIRTIO_MMIO_INTERRUPT_ACK);
>>>>  
>>>> -  if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)
>>>> -                  && vdrv && vdrv->config_changed) {
>>>> -          vdrv->config_changed(&vm_dev->vdev);
>>>> -          ret = IRQ_HANDLED;
>>>> -  }
>>>> +  if (unlikely(status & VIRTIO_MMIO_INT_CONFIG))
>>>> +          return vm_config_changed(irq, opaque);
>>>>  
>>>> -  if (likely(status & VIRTIO_MMIO_INT_VRING)) {
>>>> -          spin_lock_irqsave(&vm_dev->lock, flags);
>>>> -          list_for_each_entry(info, &vm_dev->virtqueues, node)
>>>> -                  ret |= vring_interrupt(irq, info->vq);
>>>> -          spin_unlock_irqrestore(&vm_dev->lock, flags);
>>>> -  }
>>>> +  if (likely(status & VIRTIO_MMIO_INT_VRING))
>>>> +          return vm_vring_interrupt(irq, opaque);
>>>>  
>>>>    return ret;
>>>>  }
>>>> @@ -284,18 +313,98 @@ static void vm_del_vq(struct virtqueue *vq)
>>>>    kfree(info);
>>>>  }
>>>>  
>>>> -static void vm_del_vqs(struct virtio_device *vdev)
>>>> +static void vm_free_irqs(struct virtio_device *vdev)
>>>>  {
>>>> +  int i;
>>>>    struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
>>>> +
>>>> +  if (vm_dev->single_irq_enabled) {
>>>> +          free_irq(platform_get_irq(vm_dev->pdev, 0), vm_dev);
>>>> +          vm_dev->single_irq_enabled = 0;
>>>> +  }
>>>> +
>>>> +  for (i = 0; i < vm_dev->used_irqs; ++i)
>>>> +          free_irq(platform_get_irq(vm_dev->pdev, i), vm_dev);
>>>> +
>>>> +  vm_dev->num_irqs = 0;
>>>> +  vm_dev->used_irqs = 0;
>>>> +  kfree(vm_dev->vm_vq_names);
>>>> +  vm_dev->vm_vq_names = NULL;
>>>> +}
>>>> +
>>>> +static void vm_del_vqs(struct virtio_device *vdev)
>>>> +{
>>>>    struct virtqueue *vq, *n;
>>>>  
>>>>    list_for_each_entry_safe(vq, n, &vdev->vqs, list)
>>>>            vm_del_vq(vq);
>>>>  
>>>> -  free_irq(platform_get_irq(vm_dev->pdev, 0), vm_dev);
>>>> +  vm_free_irqs(vdev);
>>>> +}
>>>> +
>>>> +static int vm_request_multiple_irqs(struct virtio_device *vdev, int nirqs,
>>>> +          bool per_vq_irq)
>>>> +{
>>>> +  int err = -ENOMEM;
>>>> +  struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
>>>> +  unsigned i, v;
>>>> +  int irq = 0;
>>>> +
>>>> +  vm_dev->num_irqs = nirqs;
>>>> +  vm_dev->used_irqs = 0;
>>>> +
>>>> +  vm_dev->vm_vq_names = kmalloc_array(nirqs, sizeof(*vm_dev->vm_vq_names),
>>>> +                               GFP_KERNEL);
>>>> +  if (!vm_dev->vm_vq_names)
>>>> +          goto error;
>>>> +
>>>> +  for (i = 0; i < nirqs; i++) {
>>>> +          irq = platform_get_irq(vm_dev->pdev, i);
>>>> +          if (irq == -ENXIO)
>>>> +                  goto error;
>>>> +  }
>>>> +
>>>> +  /* Set the irq used for configuration */
>>>> +  v = vm_dev->used_irqs;
>>>> +  snprintf(vm_dev->vm_vq_names[v], sizeof(*vm_dev->vm_vq_names),
>>>> +           "%s-config", dev_name(&vdev->dev));
>>>> +  irq = platform_get_irq(vm_dev->pdev, v);
>>>> +  err = request_irq(irq, vm_config_changed, 0,
>>>> +          vm_dev->vm_vq_names[v], vm_dev);
>>>> +  ++vm_dev->used_irqs;
>>>> +  if (err)
>>>> +          goto error;
>>>> +
>>>> +  if (!per_vq_irq) {
>>>> +          /* Shared irq for all VQs */
>>>> +          v = vm_dev->used_irqs;
>>>> +          snprintf(vm_dev->vm_vq_names[v], sizeof(*vm_dev->vm_vq_names),
>>>> +                   "%s-virtqueues", dev_name(&vdev->dev));
>>>> +          irq = platform_get_irq(vm_dev->pdev, v);
>>>> +          err = request_irq(irq, vm_vring_interrupt, 0,
>>>> +                  vm_dev->vm_vq_names[v], vm_dev);
>>>> +          if (err)
>>>> +                  goto error;
>>>> +          ++vm_dev->used_irqs;
>>>> +  }
>>>> +  return 0;
>>>> +error:
>>>> +  vm_free_irqs(vdev);
>>>> +  return err;
>>>>  }
>>>>  
>>>> +static int vm_request_single_irq(struct virtio_device *vdev)
>>>> +{
>>>> +  int err;
>>>> +  struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
>>>> +  int irq = platform_get_irq(vm_dev->pdev, 0);
>>>>  
>>>> +  err = request_irq(irq, vm_interrupt, IRQF_SHARED,
>>>> +          dev_name(&vdev->dev), vm_dev);
>>>> +  if (!err)
>>>> +          vm_dev->single_irq_enabled = 1;
>>>> +  return err;
>>>> +}
>>>>  
>>>>  static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned 
>>>> index,
>>>>                              void (*callback)(struct virtqueue *vq),
>>>> @@ -392,29 +501,92 @@ error_available:
>>>>    return ERR_PTR(err);
>>>>  }
>>>>  
>>>> -static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>>>> -                 struct virtqueue *vqs[],
>>>> -                 vq_callback_t *callbacks[],
>>>> -                 const char *names[])
>>>> +static int vm_try_to_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>>>> +                        struct virtqueue *vqs[],
>>>> +                        vq_callback_t *callbacks[],
>>>> +                        const char *names[],
>>>> +                        bool use_multiple_irq,
>>>> +                        bool per_vq_irq)
>>>>  {
>>>>    struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
>>>> -  unsigned int irq = platform_get_irq(vm_dev->pdev, 0);
>>>> -  int i, err;
>>>> +  int i, err, nirqs, irq;
>>>> +
>>>> +  if (!use_multiple_irq) {
>>>> +          /* Old style: one normal interrupt for change and all vqs. */
>>>> +          err = vm_request_single_irq(vdev);
>>>> +          if (err)
>>>> +                  goto error_request;
>>>> +  } else {
>>>> +          if (per_vq_irq) {
>>>> +                  /* Best option: one for change interrupt, one per vq. */
>>>> +                  nirqs = 1;
>>>> +                  for (i = 0; i < nvqs; ++i)
>>>> +                          if (callbacks[i])
>>>> +                                  ++nirqs;
>>>> +          } else {
>>>> +                  /* Second best: one for change, shared for all vqs. */
>>>> +                  nirqs = 2;
>>>> +          }
>>>>  
>>>> -  err = request_irq(irq, vm_interrupt, IRQF_SHARED,
>>>> -                  dev_name(&vdev->dev), vm_dev);
>>>> -  if (err)
>>>> -          return err;
>>>> +          err = vm_request_multiple_irqs(vdev, nirqs, per_vq_irq);
>>>> +          if (err)
>>>> +                  goto error_request;
>>>> +  }
>>>>  
>>>> -  for (i = 0; i < nvqs; ++i) {
>>>> +  for (i = 0; i < nvqs; i++) {
>>>> +          if (!names[i]) {
>>>> +                  vqs[i] = NULL;
>>>> +                  continue;
>>>> +          }
>>>>            vqs[i] = vm_setup_vq(vdev, i, callbacks[i], names[i]);
>>>>            if (IS_ERR(vqs[i])) {
>>>> -                  vm_del_vqs(vdev);
>>>> -                  return PTR_ERR(vqs[i]);
>>>> +                  err = PTR_ERR(vqs[i]);
>>>> +                  goto error_find;
>>>> +          }
>>>> +          if (!per_vq_irq || !callbacks[i])
>>>> +                  continue;
>>>> +          /* allocate per-vq irq if available and necessary */
>>>> +          snprintf(vm_dev->vm_vq_names[vm_dev->used_irqs],
>>>> +                  sizeof(*vm_dev->vm_vq_names),
>>>> +                  "%s-%s",
>>>> +                  dev_name(&vm_dev->vdev.dev), names[i]);
>>>> +          irq = platform_get_irq(vm_dev->pdev, vm_dev->used_irqs);
>>>> +          err = request_irq(irq, vring_interrupt, 0,
>>>> +                  vm_dev->vm_vq_names[vm_dev->used_irqs], vqs[i]);
>>>> +          if (err) {
>>>> +                  vm_del_vq(vqs[i]);
>>>> +                  goto error_find;
>>>>            }
>>>> +          ++vm_dev->used_irqs;
>>>>    }
>>>> -
>>>>    return 0;
>>>> +error_find:
>>>> +  vm_del_vqs(vdev);
>>>> +
>>>> +error_request:
>>>> +  return err;
>>>> +}
>>>> +
>>>> +static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>>>> +                 struct virtqueue *vqs[],
>>>> +                 vq_callback_t *callbacks[],
>>>> +                 const char *names[])
>>>> +{
>>>> +  int err;
>>>> +
>>>> +  /* Try multiple irqs with one irq per queue. */
>>>> +  err = vm_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names, true, true);
>>>> +  if (!err)
>>>> +          return 0;
>>>> +  /* Fallback: multiple irqs with one irq for config,
>>>> +   * one shared for queues. */
>>>> +  err = vm_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
>>>> +                           true, false);
>>>> +  if (!err)
>>>> +          return 0;
>>>> +  /* Finally fall back to regular single interrupts. */
>>>> +  return vm_try_to_find_vqs(vdev, nvqs, vqs, callbacks, names,
>>>> +                            false, false);
>>>>  }
>>>>  
>>>>  static const char *vm_bus_name(struct virtio_device *vdev)
>>>> -- 
>>>> 1.7.1
>>>
>>> .
>>>
>>
>>
>> -- 
>> Shannon
> 
> .
> 


-- 
Shannon