From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NuqXW-0006Nq-ON for qemu-devel@nongnu.org; Thu, 25 Mar 2010 13:08:14 -0400 Received: from [140.186.70.92] (port=58689 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NuqXM-0006Mo-Fu for qemu-devel@nongnu.org; Thu, 25 Mar 2010 13:08:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1NuqXF-0008Fq-6W for qemu-devel@nongnu.org; Thu, 25 Mar 2010 13:08:03 -0400 Received: from mail-px0-f203.google.com ([209.85.216.203]:58316) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1NuqXE-0008Fh-Sh for qemu-devel@nongnu.org; Thu, 25 Mar 2010 13:07:57 -0400 Received: by pxi41 with SMTP id 41so650086pxi.23 for ; Thu, 25 Mar 2010 10:07:55 -0700 (PDT) MIME-Version: 1.0 Sender: camm@ualberta.ca In-Reply-To: <20100325163439.GC16235@redhat.com> References: <1269497376-21903-1-git-send-email-cam@cs.ualberta.ca> <20100325090513.GA11153@redhat.com> <8286e4ee1003250930m4a9d2216xafccf556234085f6@mail.gmail.com> <20100325163439.GC16235@redhat.com> Date: Thu, 25 Mar 2010 11:07:46 -0600 Message-ID: <8286e4ee1003251007o2bd8d9f2h6ba1f0c3ee8dda7d@mail.gmail.com> From: Cam Macdonell Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH v3 1/1] Shared memory uio_pci driver List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org On Thu, Mar 25, 2010 at 10:34 AM, Michael S. Tsirkin wrote= : > On Thu, Mar 25, 2010 at 10:30:42AM -0600, Cam Macdonell wrote: >> On Thu, Mar 25, 2010 at 3:05 AM, Michael S. Tsirkin wro= te: >> > On Thu, Mar 25, 2010 at 12:09:36AM -0600, Cam Macdonell wrote: >> >> This patch adds a driver for my shared memory PCI device using the ui= o_pci >> >> interface. =A0The driver has three memory regions. =A0The first memor= y region is for >> >> device registers for sending interrupts. The second BAR is for receiv= ing MSI-X >> >> interrupts and the third memory region maps the shared memory. =A0The= device only >> >> exports the first and third memory regions to userspace. >> >> >> >> This driver supports MSI-X and regular pin interrupts. =A0Currently, = the number of >> >> MSI vectors is set to 4 which could be increased, but the driver will= work with >> >> fewer vectors. =A0If MSI is not available, then regular interrupts wi= ll be used. >> >> --- >> >> =A0drivers/uio/Kconfig =A0 =A0 =A0 | =A0 =A08 ++ >> >> =A0drivers/uio/Makefile =A0 =A0 =A0| =A0 =A01 + >> >> =A0drivers/uio/uio_ivshmem.c | =A0235 +++++++++++++++++++++++++++++++= ++++++++++++++ >> >> =A03 files changed, 244 insertions(+), 0 deletions(-) >> >> =A0create mode 100644 drivers/uio/uio_ivshmem.c >> >> >> >> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig >> >> index 1da73ec..b92cded 100644 >> >> --- a/drivers/uio/Kconfig >> >> +++ b/drivers/uio/Kconfig >> >> @@ -74,6 +74,14 @@ config UIO_SERCOS3 >> >> >> >> =A0 =A0 =A0 =A0 If you compile this as a module, it will be called ui= o_sercos3. >> >> >> >> +config UIO_IVSHMEM >> >> + =A0 =A0 tristate "KVM shared memory PCI driver" >> >> + =A0 =A0 default n >> >> + =A0 =A0 help >> >> + =A0 =A0 =A0 Userspace I/O interface for the KVM shared memory devic= e. =A0This >> >> + =A0 =A0 =A0 driver will make available two memory regions, the firs= t is >> >> + =A0 =A0 =A0 registers and the second is a region for sharing betwee= n VMs. >> >> + >> >> =A0config UIO_PCI_GENERIC >> >> =A0 =A0 =A0 tristate "Generic driver for PCI 2.3 and PCI Express card= s" >> >> =A0 =A0 =A0 depends on PCI >> >> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile >> >> index 18fd818..25c1ca5 100644 >> >> --- a/drivers/uio/Makefile >> >> +++ b/drivers/uio/Makefile >> >> @@ -6,3 +6,4 @@ obj-$(CONFIG_UIO_AEC) +=3D uio_aec.o >> >> =A0obj-$(CONFIG_UIO_SERCOS3) =A0 =A0+=3D uio_sercos3.o >> >> =A0obj-$(CONFIG_UIO_PCI_GENERIC) =A0 =A0 =A0 =A0+=3D uio_pci_generic.= o >> >> =A0obj-$(CONFIG_UIO_NETX) =A0 =A0 =A0 +=3D uio_netx.o >> >> +obj-$(CONFIG_UIO_IVSHMEM) +=3D uio_ivshmem.o >> >> diff --git a/drivers/uio/uio_ivshmem.c b/drivers/uio/uio_ivshmem.c >> >> new file mode 100644 >> >> index 0000000..607435b >> >> --- /dev/null >> >> +++ b/drivers/uio/uio_ivshmem.c >> >> @@ -0,0 +1,235 @@ >> >> +/* >> >> + * UIO IVShmem Driver >> >> + * >> >> + * (C) 2009 Cam Macdonell >> >> + * based on Hilscher CIF card driver (C) 2007 Hans J. Koch >> >> + * >> >> + * Licensed under GPL version 2 only. >> >> + * >> >> + */ >> >> + >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> + >> >> +#include >> >> + >> >> +#define IntrStatus 0x04 >> >> +#define IntrMask 0x00 >> >> + >> >> +struct ivshmem_info { >> >> + =A0 =A0struct uio_info *uio; >> >> + =A0 =A0struct pci_dev *dev; >> >> + =A0 =A0char (*msix_names)[256]; >> >> + =A0 =A0struct msix_entry *msix_entries; >> >> + =A0 =A0int nvectors; >> >> +}; >> >> + >> >> +static irqreturn_t ivshmem_handler(int irq, struct uio_info *dev_inf= o) >> >> +{ >> >> + >> >> + =A0 =A0void __iomem *plx_intscr =3D dev_info->mem[0].internal_addr >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0+ IntrStatus; >> >> + =A0 =A0u32 val; >> >> + >> >> + =A0 =A0val =3D readl(plx_intscr); >> >> + =A0 =A0if (val =3D=3D 0) >> >> + =A0 =A0 =A0 =A0return IRQ_NONE; >> >> + >> >> + =A0 =A0printk(KERN_INFO "Regular interrupt (val =3D %d)\n", val); >> >> + =A0 =A0return IRQ_HANDLED; >> >> +} >> >> + >> >> +static irqreturn_t ivshmem_msix_handler(int irq, void *opaque) >> >> +{ >> >> + >> >> + =A0 =A0struct uio_info * dev_info =3D (struct uio_info *) opaque; >> >> + >> >> + =A0 =A0/* we have to do this explicitly when using MSI-X */ >> >> + =A0 =A0uio_event_notify(dev_info); >> > >> > How does userspace know which vector was triggered? >> >> Right now, it doesn't. =A0If a user had a particular need they would >> need to write their own uio driver. =A0I guess this leads to a >> discussion of MSI support in UIO and how they would work with the >> userspace. > > So why request more than one vector then? No strong reason, to some extent an artifact of my playing with uio and MSI together. But, since interrupts on vector 0 are raised on guest joins/exits, a user could modify the driver to ignore those if they wanted. Also, this way the device doesn't need to know if the guest is using 1 or 2 vectors. > >> > >> >> + =A0 =A0printk(KERN_INFO "MSI-X interrupt (%d)\n", irq); >> >> + =A0 =A0return IRQ_HANDLED; >> >> + >> > >> > extra empty line >> > >> >> +} >> >> + >> >> +static int request_msix_vectors(struct ivshmem_info *ivs_info, int n= vectors) >> >> +{ >> >> + =A0 =A0int i, err; >> >> + =A0 =A0const char *name =3D "ivshmem"; >> >> + >> >> + =A0 =A0printk(KERN_INFO "devname is %s\n", name); >> > >> > These KERN_INFO messages need to be cleaned up, they would be >> > look confusing in the log. >> >> Agreed. =A0I will clean most of these out. >> >> > >> >> + =A0 =A0ivs_info->nvectors =3D nvectors; >> >> + >> >> + >> >> + =A0 =A0ivs_info->msix_entries =3D kmalloc(nvectors * sizeof *ivs_in= fo->msix_entries, >> >> + =A0 =A0 =A0 =A0 =A0 =A0GFP_KERNEL); >> >> + =A0 =A0ivs_info->msix_names =3D kmalloc(nvectors * sizeof *ivs_info= ->msix_names, >> >> + =A0 =A0 =A0 =A0 =A0 =A0GFP_KERNEL); >> >> + >> > >> > need to handle errors >> > >> >> + =A0 =A0for (i =3D 0; i < nvectors; ++i) >> >> + =A0 =A0 =A0 =A0ivs_info->msix_entries[i].entry =3D i; >> >> + >> >> + =A0 =A0err =3D pci_enable_msix(ivs_info->dev, ivs_info->msix_entrie= s, >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ivs_info->nvectors); >> >> + =A0 =A0if (err > 0) { >> >> + =A0 =A0 =A0 =A0ivs_info->nvectors =3D err; /* msi-x positive error = code >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 returns the number available*/ >> >> + =A0 =A0 =A0 =A0err =3D pci_enable_msix(ivs_info->dev, ivs_info->msi= x_entries, >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ivs_info->nvectors); >> >> + =A0 =A0 =A0 =A0if (err > 0) { >> >> + =A0 =A0 =A0 =A0 =A0 =A0printk(KERN_INFO "no MSI (%d). Back to INTx.= \n", err); >> >> + =A0 =A0 =A0 =A0 =A0 =A0return -ENOSPC; >> >> + =A0 =A0 =A0 =A0} >> >> + =A0 =A0} >> > >> > we can also get err < 0. >> > >> >> + >> >> + =A0 =A0printk(KERN_INFO "err is %d\n", err); >> >> + =A0 =A0if (err) return err; >> > >> > coding style rule violation >> > >> >> + >> >> + =A0 =A0for (i =3D 0; i < ivs_info->nvectors; i++) { >> >> + >> >> + =A0 =A0 =A0 =A0snprintf(ivs_info->msix_names[i], sizeof *ivs_info->= msix_names, >> >> + =A0 =A0 =A0 =A0 =A0 =A0"%s-config", name); >> >> + >> >> + =A0 =A0 =A0 =A0ivs_info->msix_entries[i].entry =3D i; >> >> + =A0 =A0 =A0 =A0err =3D request_irq(ivs_info->msix_entries[i].vector= , >> >> + =A0 =A0 =A0 =A0 =A0 =A0ivshmem_msix_handler, 0, >> >> + =A0 =A0 =A0 =A0 =A0 =A0ivs_info->msix_names[i], ivs_info->uio); >> >> + >> >> + =A0 =A0 =A0 =A0if (err) { >> >> + =A0 =A0 =A0 =A0 =A0 =A0return -ENOSPC; >> > >> > coding style rule violation >> > no undo on error handling >> > why override error code with -ENOSPC? >> >> Ah, I think I've confused linux and qemu coding styles perhaps. =A0I'll >> fix these and others above. >> >> > >> >> + =A0 =A0 =A0 =A0} >> >> + =A0 =A0} >> >> + >> >> + =A0 =A0return 0; >> >> +} >> >> + >> >> +static int __devinit ivshmem_pci_probe(struct pci_dev *dev, >> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const struct pci_device_id *= id) >> >> +{ >> >> + =A0 =A0struct uio_info *info; >> >> + =A0 =A0struct ivshmem_info * ivshmem_info; >> >> + =A0 =A0int nvectors =3D 4; >> >> + >> >> + =A0 =A0info =3D kzalloc(sizeof(struct uio_info), GFP_KERNEL); >> >> + =A0 =A0if (!info) >> >> + =A0 =A0 =A0 =A0return -ENOMEM; >> >> + >> >> + =A0 =A0ivshmem_info =3D kzalloc(sizeof(struct ivshmem_info), GFP_KE= RNEL); >> >> + =A0 =A0if (!ivshmem_info) { >> >> + =A0 =A0 =A0 =A0kfree(info); >> >> + =A0 =A0 =A0 =A0return -ENOMEM; >> >> + =A0 =A0} >> >> + >> >> + =A0 =A0if (pci_enable_device(dev)) >> >> + =A0 =A0 =A0 =A0goto out_free; >> >> + >> >> + =A0 =A0if (pci_request_regions(dev, "ivshmem")) >> >> + =A0 =A0 =A0 =A0goto out_disable; >> >> + >> >> + =A0 =A0info->mem[0].addr =3D pci_resource_start(dev, 0); >> >> + =A0 =A0if (!info->mem[0].addr) >> >> + =A0 =A0 =A0 =A0goto out_release; >> >> + >> >> + =A0 =A0info->mem[0].size =3D pci_resource_len(dev, 0); >> >> + =A0 =A0info->mem[0].internal_addr =3D pci_ioremap_bar(dev, 0); >> >> + =A0 =A0if (!info->mem[0].internal_addr) { >> >> + =A0 =A0 =A0 =A0printk(KERN_INFO "got a null"); >> >> + =A0 =A0 =A0 =A0goto out_release; >> >> + =A0 =A0} >> >> + >> >> + =A0 =A0info->mem[0].memtype =3D UIO_MEM_PHYS; >> >> + >> >> + =A0 =A0info->mem[1].addr =3D pci_resource_start(dev, 2); >> >> + =A0 =A0if (!info->mem[1].addr) >> >> + =A0 =A0 =A0 =A0goto out_unmap; >> >> + =A0 =A0info->mem[1].internal_addr =3D pci_ioremap_bar(dev, 2); >> >> + =A0 =A0if (!info->mem[1].internal_addr) >> >> + =A0 =A0 =A0 =A0goto out_unmap; >> >> + >> >> + =A0 =A0info->mem[1].size =3D pci_resource_len(dev, 2); >> >> + =A0 =A0info->mem[1].memtype =3D UIO_MEM_PHYS; >> >> + >> >> + =A0 =A0ivshmem_info->uio =3D info; >> >> + =A0 =A0ivshmem_info->dev =3D dev; >> >> + >> >> + =A0 =A0if (request_msix_vectors(ivshmem_info, nvectors) !=3D 0) { >> >> + =A0 =A0 =A0 =A0printk(KERN_INFO "regular IRQs\n"); >> >> + =A0 =A0 =A0 =A0info->irq =3D dev->irq; >> >> + =A0 =A0 =A0 =A0info->irq_flags =3D IRQF_SHARED; >> >> + =A0 =A0 =A0 =A0info->handler =3D ivshmem_handler; >> >> + =A0 =A0 =A0 =A0writel(0xffffffff, info->mem[0].internal_addr + Intr= Mask); >> >> + =A0 =A0} else { >> >> + =A0 =A0 =A0 =A0printk(KERN_INFO "MSI-X enabled\n"); >> >> + =A0 =A0 =A0 =A0info->irq =3D -1; >> >> + =A0 =A0} >> >> + >> >> + =A0 =A0info->name =3D "ivshmem"; >> >> + =A0 =A0info->version =3D "0.0.1"; >> >> + >> >> + =A0 =A0if (uio_register_device(&dev->dev, info)) >> >> + =A0 =A0 =A0 =A0goto out_unmap2; >> >> + >> >> + =A0 =A0pci_set_drvdata(dev, info); >> >> + >> >> + >> >> + =A0 =A0return 0; >> >> +out_unmap2: >> >> + =A0 =A0iounmap(info->mem[2].internal_addr); >> >> +out_unmap: >> >> + =A0 =A0iounmap(info->mem[0].internal_addr); >> >> +out_release: >> >> + =A0 =A0pci_release_regions(dev); >> >> +out_disable: >> >> + =A0 =A0pci_disable_device(dev); >> >> +out_free: >> >> + =A0 =A0kfree (info); >> >> + =A0 =A0return -ENODEV; >> >> +} >> >> + >> >> +static void ivshmem_pci_remove(struct pci_dev *dev) >> >> +{ >> >> + =A0 =A0struct uio_info *info =3D pci_get_drvdata(dev); >> >> + >> >> + =A0 =A0uio_unregister_device(info); >> >> + =A0 =A0pci_release_regions(dev); >> >> + =A0 =A0pci_disable_device(dev); >> >> + =A0 =A0pci_set_drvdata(dev, NULL); >> >> + =A0 =A0iounmap(info->mem[0].internal_addr); >> >> + >> >> + =A0 =A0kfree (info); >> >> +} >> >> + >> >> +static struct pci_device_id ivshmem_pci_ids[] __devinitdata =3D { >> >> + =A0 =A0{ >> >> + =A0 =A0 =A0 =A0.vendor =3D =A0 =A00x1af4, >> >> + =A0 =A0 =A0 =A0.device =3D =A0 =A00x1110, >> > >> > vendor ids must be registered with PCI SIG. >> > this one does not seem to be registered. >> > >> >> + =A0 =A0 =A0 =A0.subvendor =3D =A0 =A0PCI_ANY_ID, >> >> + =A0 =A0 =A0 =A0.subdevice =3D =A0 =A0PCI_ANY_ID, >> >> + =A0 =A0}, >> >> + =A0 =A0{ 0, } >> >> +}; >> >> + >> >> +static struct pci_driver ivshmem_pci_driver =3D { >> >> + =A0 =A0.name =3D "uio_ivshmem", >> >> + =A0 =A0.id_table =3D ivshmem_pci_ids, >> >> + =A0 =A0.probe =3D ivshmem_pci_probe, >> >> + =A0 =A0.remove =3D ivshmem_pci_remove, >> >> +}; >> >> + >> >> +static int __init ivshmem_init_module(void) >> >> +{ >> >> + =A0 =A0return pci_register_driver(&ivshmem_pci_driver); >> >> +} >> >> + >> >> +static void __exit ivshmem_exit_module(void) >> >> +{ >> >> + =A0 =A0pci_unregister_driver(&ivshmem_pci_driver); >> >> +} >> >> + >> >> +module_init(ivshmem_init_module); >> >> +module_exit(ivshmem_exit_module); >> >> + >> >> +MODULE_DEVICE_TABLE(pci, ivshmem_pci_ids); >> >> +MODULE_LICENSE("GPL v2"); >> >> +MODULE_AUTHOR("Cam Macdonell"); >> >> -- >> >> 1.6.6.1 >> >> >> >> -- >> >> To unsubscribe from this list: send the line "unsubscribe kvm" in >> >> the body of a message to majordomo@vger.kernel.org >> >> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html >> > >> > > >