Re: [PATCH v4 3/3] Inter-VM shared memory PCI device

[Avi Kivity <avi@redhat.com>]

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=<size in MB>[,shm=<shm name>]
>    

Can that be <size in format accepted by -m> (2M, 4G, 19T, ...).

> Interrupts are supported between multiple VMs by using a shared memory server
> by using a chardev socket.
>
>      -device ivshmem,size=<size in MB>[,shm=<shm name>][,chardev=<id>][,msi=on]
>              [,irqfd=on][,vectors=n]
>      -chardev socket,path=<path>,id=<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.