Re: [Qemu-devel] [RFC PATCH] vfio: VFIO PCI driver for Qemu

[Alex Williamson]

On Fri, 2012-07-27 at 19:22 +0000, Blue Swirl wrote:
> On Wed, Jul 25, 2012 at 5:03 PM, Alex Williamson
> > diff --git a/hw/vfio_pci.c b/hw/vfio_pci.c
> > new file mode 100644
> > index 0000000..e9ae421
> > --- /dev/null
> > +++ b/hw/vfio_pci.c
> > @@ -0,0 +1,2030 @@
> > +/*
> > + * vfio based device assignment support
> > + *
> > + * Copyright Red Hat, Inc. 2012
> > + *
> > + * Authors:
> > + *  Alex Williamson <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> 
> GPLv2only?

Much of this is derived from KVM assignment, which is v2 only.
...
> > +
> > +/*
> > + * INTx
> > + */
> > +static inline void vfio_unmask_intx(VFIODevice *vdev)
> 
> 'inline' may be premature optimization.

ok

...
> > +
> > +struct vfio_irq_set_fd {
> > +    struct vfio_irq_set irq_set;
> > +    int32_t fd;
> > +} QEMU_PACKED;
> 
> Why is this structure not defined in kernel headers?

The kernel header defines struct vfio_irq_set, the data that follows is
specified by the flags and count values in that struct.  It's just
convenient for us to have a locally defined version that tacks on a
single fd since we use that a couple times.

> > +
> > +static void vfio_enable_intx_kvm(VFIODevice *vdev)
> > +{
> > +#ifdef CONFIG_KVM
> 
> These shouldn't be needed. The device will not be useful without KVM,
> so the file shouldn't be compiled for non-KVM case at all.

Actually we do support qemu-only device assignment, so this is an
accelerator.  I can't guarantee that TCG is going to do atomic memory
updates on control structures, which might make devices unhappy, but
otherwise it should work.  There's no way to get INTx EOIs in qemu-only,
but I hope that's a temporary problem.

...
> > +/*
> > + * Interrupt setup
> > + */
> > +static void vfio_disable_interrupts(VFIODevice *vdev)
> > +{
> > +    switch (vdev->interrupt) {
> > +    case INT_INTx:
> > +        vfio_disable_intx(vdev);
> > +        break;
> > +    case INT_MSI:
> > +        vfio_disable_msi_x(vdev, false);
> > +        break;
> > +    case INT_MSIX:
> > +        vfio_disable_msi_x(vdev, true);
> 
> I'd add 'break' here, maybe also to default cases earlier. That way if
> somebody adds code after this he won't introduce a fall through
> accidentally.

ok

...
> > +static void vfio_map_bar(VFIODevice *vdev, int nr, uint8_t type)
> > +{
> > +    VFIOBAR *bar = &vdev->bars[nr];
> > +    unsigned size = bar->size;
> > +    char name[64];
> > +
> > +    sprintf(name, "VFIO %04x:%02x:%02x.%x BAR %d", vdev->host.domain,
> > +            vdev->host.bus, vdev->host.slot, vdev->host.function, nr);
> 
> snprintf, please.

ok, and I'll fix all of them

> > +
> > +    /* A "slow" read/write mapping underlies all BARs */
> > +    memory_region_init_io(&bar->mem, &vfio_bar_ops, bar, name, size);
> > +    pci_register_bar(&vdev->pdev, nr, type, &bar->mem);
> > +
> > +    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> > +        return; /* IO space is only slow, don't expect high perf here */
> > +    }
> > +
> > +    if (size & ~TARGET_PAGE_MASK) {
> > +        error_report("%s is too small to mmap, this may affect 
> > performance.\n",
> > +                     name);
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * We can't mmap areas overlapping the MSIX vector table, so we
> > +     * potentially insert a direct-mapped subregion before and after it.
> > +     */
> > +    if (vdev->msix && vdev->msix->table_bar == nr) {
> > +        size = vdev->msix->table_offset & TARGET_PAGE_MASK;
> > +    }
> > +
> > +    if (size) {
> > +        strcat(name, " mmap");
> 
> strncat + trailing NUL.

ok, assume you mean strncat(name, " mmap", sizeof(name) - strlen(name) - 1).
I'll fix all of these too.

...
> > +static int vfio_load_rom(VFIODevice *vdev)
> > +{
> > +    uint64_t size = vdev->rom_size;
> > +    const VMStateDescription *vmsd;
> > +    char name[32];
> > +    off_t off = 0, voff = vdev->rom_offset;
> > +    ssize_t bytes;
> > +    void *ptr;
> > +
> > +    /* If loading ROM from file, pci handles it */
> > +    if (vdev->pdev.romfile || !vdev->pdev.rom_bar || !size)
> 
> Braces.

d'oh

...
> > +
> > +static int __vfio_get_device(VFIOGroup *group,
> > +                             const char *name, VFIODevice *vdev)
> 
> Please remove leading underscores.

yup, rolled this into the caller since it's only used once.

...
> > diff --git a/hw/vfio_pci.h b/hw/vfio_pci.h
> > new file mode 100644
> > index 0000000..9fb27ce
> > --- /dev/null
> > +++ b/hw/vfio_pci.h
> > @@ -0,0 +1,100 @@
> > +#ifndef __VFIO_H__
> > +#define __VFIO_H__
> 
> HW_VFIO_PCI_H

ok

Thanks for the review!

Alex