From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH v4 3/3] Inter-VM shared memory PCI device Date: Mon, 12 Apr 2010 23:56:15 +0300 Message-ID: <4BC388EF.8000706@redhat.com> References: <1270680720-8457-1-git-send-email-cam@cs.ualberta.ca> <1270680720-8457-2-git-send-email-cam@cs.ualberta.ca> <1270680720-8457-3-git-send-email-cam@cs.ualberta.ca> <1270680720-8457-4-git-send-email-cam@cs.ualberta.ca> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org To: Cam Macdonell Return-path: Received: from mx1.redhat.com ([209.132.183.28]:1026 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754071Ab0DLU4V (ORCPT ); Mon, 12 Apr 2010 16:56:21 -0400 In-Reply-To: <1270680720-8457-4-git-send-email-cam@cs.ualberta.ca> Sender: kvm-owner@vger.kernel.org List-ID: On 04/08/2010 01:52 AM, Cam Macdonell wrote: > Support an inter-vm shared memory device that maps a shared-memory object as a > PCI device in the guest. This patch also supports interrupts between guest by > communicating over a unix domain socket. This patch applies to the qemu-kvm > repository. > > -device ivshmem,size=[,shm=] > Can that be (2M, 4G, 19T, ...). > Interrupts are supported between multiple VMs by using a shared memory server > by using a chardev socket. > > -device ivshmem,size=[,shm=][,chardev=][,msi=on] > [,irqfd=on][,vectors=n] > -chardev socket,path=,id= > Do we need the irqfd parameter? Should be on by default. On the other hand, it may fail with older kernels with limited irqfd slots, so better keep it there. > Sample programs, init scripts and the shared memory server are available in a > git repo here: > > www.gitorious.org/nahanni > Please consider qemu.git/contrib. > --- > Makefile.target | 3 + > hw/ivshmem.c | 700 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qemu-char.c | 6 + > qemu-char.h | 3 + > qemu-doc.texi | 45 +++++++++++++ > 4 files changed, 712 insertions(+), 0 deletions(-) > create mode 100644 hw/ivshmem.c > > diff --git a/Makefile.target b/Makefile.target > index 1ffd802..bc9a681 100644 > --- a/Makefile.target > +++ b/Makefile.target > @@ -199,6 +199,9 @@ obj-$(CONFIG_USB_OHCI) += usb-ohci.o > obj-y += rtl8139.o > obj-y += e1000.o > > +# Inter-VM PCI shared memory > +obj-y += ivshmem.o > + > depends on CONFIG_PCI > # Hardware support > + > +#define PCI_COMMAND_IOACCESS 0x0001 > +#define PCI_COMMAND_MEMACCESS 0x0002 > Should be in pci.h? > + > +#define DEBUG_IVSHMEM > Disable for production. > + > +#define IVSHMEM_IRQFD 0 > +#define IVSHMEM_MSI 1 > +#define IVSHMEM_MAX_EVENTFDS 16 > Too low? why limit? > + > +struct eventfd_entry { > + PCIDevice *pdev; > + int vector; > +}; > Coding style: EventfdEntry, and a typedef. > + > +typedef struct IVShmemState { > + PCIDevice dev; > + uint32_t intrmask; > + uint32_t intrstatus; > + uint32_t doorbell; > + > + CharDriverState * chr; > + CharDriverState ** eventfd_chr; > + int ivshmem_mmio_io_addr; > + > + pcibus_t mmio_addr; > + uint8_t *ivshmem_ptr; > + unsigned long ivshmem_offset; > off_t > + unsigned int ivshmem_size; > ram_addr_t > + > +/* accessing registers - based on rtl8139 */ > +static void ivshmem_update_irq(IVShmemState *s, int val) > +{ > + int isr; > + isr = (s->intrstatus& s->intrmask)& 0xffffffff; > This is highly undocumented, but fits my suggested 'status is bitmap'. 'isr' needs to be uint32_t if you mask it like that. > + > + /* don't print ISR resets */ > + if (isr) { > + IVSHMEM_DPRINTF("Set IRQ to %d (%04x %04x)\n", > + isr ? 1 : 0, s->intrstatus, s->intrmask); > + } > + > + qemu_set_irq(s->dev.irq[0], (isr != 0)); > +} > + > + > > > + > +static void create_shared_memory_BAR(IVShmemState *s, int fd) { > + > + s->shm_fd = fd; > + > + s->ivshmem_offset = qemu_ram_mmap(s->shm_fd, s->ivshmem_size, > + MAP_SHARED, 0); > Where did the offset go? > + > + s->ivshmem_ptr = qemu_get_ram_ptr(s->ivshmem_offset); > Never used, please drop. > + > + /* region for shared memory */ > + pci_register_bar(&s->dev, 2, s->ivshmem_size, > + PCI_BASE_ADDRESS_SPACE_MEMORY, ivshmem_map); > Might be worthwhile to mark it as a 64-bit BAR. Please test with ridiculous shared memory sizes. > +} > + > +static int ivshmem_irqfd(PCIDevice* pdev, uint16_t vector, int fd) > +{ > + struct kvm_irqfd call = { }; > + int r; > + > + IVSHMEM_DPRINTF("inside irqfd\n"); > + if (vector>= pdev->msix_entries_nr) > + return -EINVAL; > + call.fd = fd; > + call.gsi = pdev->msix_irq_entries[vector].gsi; > + r = kvm_vm_ioctl(kvm_state, KVM_IRQFD,&call); > + if (r< 0) { > + IVSHMEM_DPRINTF("allocating irqfd failed %d\n", r); > + return r; > + } > + return 0; > +} > should be in kvm.c for reuse. > + > +static int ivshmem_ioeventfd(IVShmemState* s, int posn, int fd, int vector) > +{ > + > + int ret; > + struct kvm_ioeventfd iofd; > + > + iofd.datamatch = (posn<< 16) | vector; > + iofd.addr = s->mmio_addr + Doorbell; > + iofd.len = 4; > + iofd.flags = KVM_IOEVENTFD_FLAG_DATAMATCH; > + iofd.fd = fd; > + > + ret = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&iofd); > Ditto. > + > + if (ret< 0) { > + fprintf(stderr, "error assigning ioeventfd (%d)\n", ret); > + perror(strerror(ret)); > + } else { > + IVSHMEM_DPRINTF("success assigning ioeventfd (%d:%d)\n", posn, vector); > + } > + > + return ret; > +} > blank line here. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic.