Re: [dpdk-dev] [PATCH 4/5] uio: new driver with MSI-X support

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 25 May 2015 14:01:14 +0800
"Liang, Cunming" <cunming.liang@intel.com> wrote:

> 
> 
> On 5/19/2015 1:40 AM, Stephen Hemminger wrote:
> > +
> > +/* set the mapping between vector # and existing eventfd. */
> > +static int set_irq_eventfd(struct uio_msi_pci_dev *udev, u32 vec, int fd)
> > +{
> > +	struct uio_msi_irq_ctx *ctx;
> > +	struct eventfd_ctx *trigger;
> > +	int irq, err;
> > +
> > +	if (vec >= udev->num_vectors) {
> > +		dev_notice(&udev->pdev->dev, "vec %u >= num_vec %u\n",
> > +			   vec, udev->num_vectors);
> > +		return -ERANGE;
> > +	}
> > +
> > +	irq = udev->msix[vec].vector;
> > +
> > +	/* Clearup existing irq mapping */
> > +	ctx = &udev->ctx[vec];
> > +	if (ctx->trigger) {
> > +		free_irq(irq, ctx->trigger);
> > +		eventfd_ctx_put(ctx->trigger);
> > +		ctx->trigger = NULL;
> > +	}
> > +
> > +	/* Passing -1 is used to disable interrupt */
> > +	if (fd < 0)
> > +		return 0;
> > +
> > +
> One unnecessary blank line here.
> > +	trigger = eventfd_ctx_fdget(fd);
> > +	if (IS_ERR(trigger)) {
> > +		err = PTR_ERR(trigger);
> > +		dev_notice(&udev->pdev->dev,
> > +			   "eventfd ctx get failed: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	err = request_irq(irq, uio_msi_irqhandler, 0, ctx->name, trigger);
> > +	if (err) {
> > +		dev_notice(&udev->pdev->dev,
> > +			   "request irq failed: %d\n", err);
> > +		eventfd_ctx_put(trigger);
> > +		return err;
> > +	}
> > +
> > +	dev_dbg(&udev->pdev->dev, "map vector %u to fd %d trigger %p\n",
> > +		  vec, fd, trigger);
> > +	ctx->trigger = trigger;
> > +	return 0;
> > +}
> > +
> > +static int
> > +uio_msi_ioctl(struct uio_info *info, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct uio_msi_pci_dev *udev
> > +		= container_of(info, struct uio_msi_pci_dev, info);
> > +	struct uio_msi_irq_set hdr;
> > +	int err;
> > +
> > +	switch (cmd) {
> > +	case UIO_MSI_IRQ_SET:
> > +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> > +			return -EFAULT;
> > +
> > +		mutex_lock(&udev->mutex);
> > +		err = set_irq_eventfd(udev, hdr.vec, hdr.fd);
> > +		mutex_unlock(&udev->mutex);
> > +		break;
> > +	default:
> > +		err = -EOPNOTSUPP;
> > +	}
> > +	return err;
> > +}
> "uio_msi_irq_set" defines in single pattern. Compare with the bulk set 
> in "vfio_irq_set", it requires additional syscall during uio_msix_enable().

The bulk operation is VFIO is actually a bad design,
It forces too many updates when manipulating individual vectors.
Personally, the whole VFIO API has some questionable design choices
that I did not want to repeat.

> > +static int uio_msi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +	struct uio_msi_pci_dev *udev;
> > +	int i, err, vectors;
> > +
> > +	udev = kzalloc(sizeof(struct uio_msi_pci_dev), GFP_KERNEL);
> > +	if (!udev)
> > +		return -ENOMEM;
> > +
> > +	err = pci_enable_device(pdev);
> > +	if (err != 0) {
> > +		dev_err(&pdev->dev, "cannot enable PCI device\n");
> > +		goto fail_free;
> > +	}
> > +
> > +	vectors = pci_msix_vec_count(pdev);
> > +	if (vectors < 0) {
> > +		dev_err(&pdev->dev, "device does not support MSI-X\n");
> > +		err = -EINVAL;
> > +		goto fail_disable;
> > +	}
> pci_msix_vec_count() is available since v3.14,  it requires a compatible 
> check.
> In order to support older version, probably a function 
> 'uio_msix_vec_count()' is necessary.
> 
> I've one overall question, is there a special reason not enhance igb_uio 
> but define a new uio_msi?  And the looks like the piece could be add 
> into igb_uio or uio_pci_generic as well. Do you have plan for the 
> latter? Thanks.

I wanted something that could go upstream. igb_uio has some other things
which make it unlikely to get accepted in current form, so seemed best to start
fresh.

Also, intentionally did not want to address any kernel version earlier
than the version where VFIO was added.