Re: [PATCH 5/6] device assignment: support for assigning PCI devices to guests

[Muli Ben-Yehuda <muli@il.ibm.com>]

On Tue, Oct 28, 2008 at 04:55:22PM +0000, Mark McLoughlin wrote:
> On Tue, 2008-10-28 at 12:06 +0200, muli@il.ibm.com wrote:
> ...
> > +static int get_real_device(AssignedDevice *pci_dev, uint8_t r_bus,
> > +                           uint8_t r_dev, uint8_t r_func)
> > +{
> > +    char dir[128], name[128];
> > +    int fd, r = 0;
> > +    FILE *f;
> > +    unsigned long long start, end, size, flags;
> > +    PCIRegion *rp;
> > +    PCIDevRegions *dev = &pci_dev->real_device;
> > +
> > +    dev->region_number = 0;
> > +
> > +    snprintf(dir, sizeof(dir), "/sys/bus/pci/devices/0000:%02x:%02x.%x/",
> > +	     r_bus, r_dev, r_func);
> > +
> > +    snprintf(name, sizeof(name), "%sconfig", dir);
> > +
> > +    fd = open(name, O_RDWR);
> > +    if (fd == -1) {
> > +        fprintf(stderr, "%s: %s: %m\n", __func__, name);
> > +        return 1;
> > +    }
> > +    dev->config_fd = fd;
> > +again:
> > +    r = read(fd, pci_dev->dev.config, sizeof(pci_dev->dev.config));
> > +    if (r < 0) {
> > +        if (errno == EINTR || errno == EAGAIN)
> > +            goto again;
> > +        fprintf(stderr, "%s: read failed, errno = %d\n", __func__, errno);
> > +    }
> > +
> > +    snprintf(name, sizeof(name), "%sresource", dir);
> > +
> > +    f = fopen(name, "r");
> > +    if (f == NULL) {
> > +        fprintf(stderr, "%s: %s: %m\n", __func__, name);
> > +        return 1;
> > +    }
> > +    r = -1;
> > +    while (fscanf(f, "%lli %lli %lli\n", &start, &end, &flags) == 3) {
> > +        r++;
> > +        rp = dev->regions + r;
> 
> Could, in theory, overflow dev->regions here. Suggest:
> 
> +    for (r = 0; r < MAX_IO_REGIONS; r++) {
> +        if (fscanf(f, "%lli %lli %lli\n", &start, &end, &flags) != 3)
> +            break;

Fixed, thanks Mark. I think it also uncovered a buglet where we would
skip the last region with the original code, which should be ok now.

> > +        rp->valid = 0;
> > +        size = end - start + 1;
> > +        flags &= IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH;
> > +        if (size == 0 || (flags & ~IORESOURCE_PREFETCH) == 0)
> > +            continue;
> > +        if (flags & IORESOURCE_MEM) {
> > +            flags &= ~IORESOURCE_IO;
> > +	    snprintf(name, sizeof(name), "%sresource%d", dir, r);
> > +            fd = open(name, O_RDWR);
> > +            if (fd == -1)
> > +                continue;       /* probably ROM */
> > +            rp->resource_fd = fd;
> > +        } else
> > +            flags &= ~IORESOURCE_PREFETCH;
> > +
> > +        rp->type = flags;
> > +        rp->valid = 1;
> > +        rp->base_addr = start;
> > +        rp->size = size;
> > +        DEBUG("region %d size %d start 0x%x type %d resource_fd %d\n",
> > +              r, rp->size, start, rp->type, rp->resource_fd);
> > +    }
> > +    fclose(f);
> > +
> > +    dev->region_number = r;
> > +    return 0;
> > +}
> > +
> > +static int disable_iommu;
> 
> Why is this global?
> 
> The flag is set per-device on the command-line and only affects whether
> we pass KVM_DEV_ASSIGN_ENABLE_IOMMU to kvm_assign_pci_device()

Made per-device by moving it to struct AssignedDevInfo.

> > +int nr_assigned_devices;
> > +static LIST_HEAD(, AssignedDevInfo) adev_head;
> > +
> > +static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn)
> > +{
> > +    return (uint32_t)bus << 8 | (uint32_t)devfn;
> > +}
> > +
> > +static AssignedDevice *register_real_device(PCIBus *e_bus,
> > +                                            const char *e_dev_name,
> > +                                            int e_devfn, uint8_t r_bus,
> > +                                            uint8_t r_dev, uint8_t r_func)
> > +{
> > +    int r;
> > +    AssignedDevice *pci_dev;
> > +    uint8_t e_device, e_intx;
> > +
> > +    DEBUG("Registering real physical device %s (devfn=0x%x)\n",
> > +          e_dev_name, e_devfn);
> > +
> > +    pci_dev = (AssignedDevice *)
> > +        pci_register_device(e_bus, e_dev_name, sizeof(AssignedDevice),
> > +                            e_devfn, assigned_dev_pci_read_config,
> > +                            assigned_dev_pci_write_config);
> > +    if (NULL == pci_dev) {
> > +        fprintf(stderr, "%s: Error: Couldn't register real device %s\n",
> > +                __func__, e_dev_name);
> > +        return NULL;
> > +    }
> > +    if (get_real_device(pci_dev, r_bus, r_dev, r_func)) {
> > +        fprintf(stderr, "%s: Error: Couldn't get real device (%s)!\n",
> > +                __func__, e_dev_name);
> > +        goto out;
> > +    }
> > +
> > +    /* handle real device's MMIO/PIO BARs */
> > +    if (assigned_dev_register_regions(pci_dev->real_device.regions,
> > +                                      pci_dev->real_device.region_number,
> > +                                      pci_dev))
> > +        goto out;
> > +
> > +    /* handle interrupt routing */
> > +    e_device = (pci_dev->dev.devfn >> 3) & 0x1f;
> > +    e_intx = pci_dev->dev.config[0x3d] - 1;
> > +    pci_dev->intpin = e_intx;
> > +    pci_dev->run = 0;
> > +    pci_dev->girq = 0;
> > +    pci_dev->h_busnr = r_bus;
> > +    pci_dev->h_devfn = PCI_DEVFN(r_dev, r_func);
> > +
> > +#ifdef KVM_CAP_DEVICE_ASSIGNMENT
> > +    if (kvm_enabled()) {
> > +        struct kvm_assigned_pci_dev assigned_dev_data;
> > +
> > +        memset(&assigned_dev_data, 0, sizeof(assigned_dev_data));
> > +        assigned_dev_data.assigned_dev_id  =
> > +            calc_assigned_dev_id(pci_dev->h_busnr,
> > +                                 (uint32_t)pci_dev->h_devfn);
> > +        assigned_dev_data.busnr = pci_dev->h_busnr;
> > +        assigned_dev_data.devfn = pci_dev->h_devfn;
> > +
> > +#ifdef KVM_CAP_IOMMU
> > +        /* We always enable the IOMMU if present
> > +         * (or when not disabled on the command line)
> > +         */
> > +        r = kvm_check_extension(kvm_context, KVM_CAP_IOMMU);
> > +        if (r && !disable_iommu)
> > +            assigned_dev_data.flags |= KVM_DEV_ASSIGN_ENABLE_IOMMU;
> > +#endif
> > +        r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
> > +        if (r < 0) {
> > +            fprintf(stderr, "Could not notify kernel about "
> > +                "assigned device \"%s\"\n", e_dev_name);
> > +            perror("register_real_device");
> > +            goto out;
> > +        }
> > +    }
> > +#endif
> > +    term_printf("Registered host PCI device %02x:%02x.%1x "
> > +		"(\"%s\") as guest device %02x:%02x.%1x\n",
> > +		r_bus, r_dev, r_func, e_dev_name,
> > +		pci_bus_num(e_bus), e_device, r_func);
> > +
> > +    return pci_dev;
> > +out:
> > +/*    pci_unregister_device(&pci_dev->dev); */
> > +    return NULL;
> > +}
> > +
> > +#ifdef KVM_CAP_DEVICE_ASSIGNMENT
> > +/* The pci config space got updated. Check if irq numbers have changed
> > + * for our devices
> > + */
> > +void assigned_dev_update_irq(PCIDevice *d)
> > +{
> > +    int irq, r;
> > +    AssignedDevice *assigned_dev;
> > +    AssignedDevInfo *adev;
> > +
> > +    LIST_FOREACH(adev, &adev_head, next) {
> > +        assigned_dev = adev->assigned_dev;
> > +        irq = pci_map_irq(&assigned_dev->dev, assigned_dev->intpin);
> > +        irq = piix_get_irq(irq);
> > +
> > +        if (irq != assigned_dev->girq) {
> > +            struct kvm_assigned_irq assigned_irq_data;
> > +
> > +            memset(&assigned_irq_data, 0, sizeof(assigned_irq_data));
> > +            assigned_irq_data.assigned_dev_id  =
> > +                calc_assigned_dev_id(assigned_dev->h_busnr,
> > +                                     (uint8_t) assigned_dev->h_devfn);
> > +            assigned_irq_data.guest_irq = irq;
> > +            assigned_irq_data.host_irq = assigned_dev->real_device.irq;
> > +            r = kvm_assign_irq(kvm_context, &assigned_irq_data);
> > +            if (r < 0) {
> > +                perror("assigned_dev_update_irq");
> > +                fprintf(stderr, "Are you assigning a device "
> > +                        "that shares IRQ with some other device?\n");
> > +                pci_unregister_device(&assigned_dev->dev);
> > +                /* FIXME: Delete node from list */
> > +                continue;
> > +            }
> > +            assigned_dev->girq = irq;
> > +        }
> > +    }
> > +}
> > +#endif
> > +
> > +struct PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus)
> > +{
> > +    adev->assigned_dev = register_real_device(bus,
> > +                                              adev->name, -1,
> > +                                              adev->bus,
> > +                                              adev->dev,
> > +                                              adev->func);
> > +    return &adev->assigned_dev->dev;
> > +}
> 
> This looks unnecessary - register_real_device() isn't used anywhere
> else.
> 
> Why not just move register_real_device() into init_assigned_device()
> ?

Done.

> 
> > +int init_all_assigned_devices(PCIBus *bus)
> > +{
> > +    struct AssignedDevInfo *adev;
> > +
> > +    LIST_FOREACH(adev, &adev_head, next)
> > +        if (init_assigned_device(adev, bus) == NULL)
> > +            return -1;
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * Syntax to assign device:
> > + *
> > + * -pcidevice dev=bus:dev.func,dma=dma
>                  ^^^                  ^^^
> 
> Should be:
> 
>   -pcidevice host=bus:dev.func[,dma=none][,name=string]

Fixed (as well as the other -pcidevice bogosities you pointed).

> 
> > + *
> > + * Example:
> > + * -pcidevice host=00:13.0,dma=pvdma
>                                   ^^^^^
> 
> Should be:
> 
>   -pcidevice host=00:13.0,dma=none,name=Foo
>   
> > + *
> > + * dma can currently only be 'none' to disable iommu support.
> > + */
> > +AssignedDevInfo *add_assigned_device(const char *arg)
> > +{
> > +    char *cp, *cp1;
> > +    char device[8];
> > +    char dma[6];
> > +    int r;
> > +    AssignedDevInfo *adev;
> > +
> > +    adev = qemu_mallocz(sizeof(AssignedDevInfo));
> > +    if (adev == NULL) {
> > +        fprintf(stderr, "%s: Out of memory\n", __func__);
> > +        return NULL;
> > +    }
> > +    r = get_param_value(device, sizeof(device), "host", arg);
> > +    r = get_param_value(adev->name, sizeof(adev->name), "name", arg);
> > +    if (!r)
> > +	snprintf(adev->name, sizeof(adev->name), "%s", device);
> > +
> > +#ifdef KVM_CAP_IOMMU
> > +    r = get_param_value(dma, sizeof(dma), "dma", arg);
> > +    if (r && !strncmp(dma, "none", 4))
> > +        disable_iommu = 1;
> > +#endif
> > +    cp = device;
> > +    adev->bus = strtoul(cp, &cp1, 16);
> > +    if (*cp1 != ':')
> > +        goto bad;
> > +    cp = cp1 + 1;
> > +
> > +    adev->dev = strtoul(cp, &cp1, 16);
> > +    if (*cp1 != '.')
> > +        goto bad;
> > +    cp = cp1 + 1;
> > +
> > +    adev->func = strtoul(cp, &cp1, 16);
> > +
> > +    nr_assigned_devices++;
> 
> nr_assigned_devices isn't actually used anywhere.

Nuked.

> 
> > +    LIST_INSERT_HEAD(&adev_head, adev, next);
> > +    return adev;
> > +bad:
> > +    fprintf(stderr, "pcidevice argument parse error; "
> > +            "please check the help text for usage\n");
> > +    qemu_free(adev);
> > +    return NULL;
> > +}
> > diff --git a/qemu/hw/device-assignment.h b/qemu/hw/device-assignment.h
> > new file mode 100644
> > index 0000000..ebc0b50
> > --- /dev/null
> > +++ b/qemu/hw/device-assignment.h
> > @@ -0,0 +1,117 @@
> > +/*
> > + * Copyright (c) 2007, Neocleus Corporation.
> > + * Copyright (c) 2007, Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
> > + * Place - Suite 330, Boston, MA 02111-1307 USA.
> > + *
> > + *  Data structures for storing PCI state
> > + *
> > + *  Adapted to kvm by Qumranet
> > + *
> > + *  Copyright (c) 2007, Neocleus, Alex Novik (alex@neocleus.com)
> > + *  Copyright (c) 2007, Neocleus, Guy Zana (guy@neocleus.com)
> > + *  Copyright (C) 2008, Qumranet, Amit Shah (amit.shah@qumranet.com)
> > + *  Copyright (C) 2008, Red Hat, Amit Shah (amit.shah@redhat.com)
> > + */
> > +
> > +#ifndef __DEVICE_ASSIGNMENT_H__
> > +#define __DEVICE_ASSIGNMENT_H__
> > +
> > +#include <sys/mman.h>
> > +#include "qemu-common.h"
> > +#include "sys-queue.h"
> > +#include "pci.h"
> > +
> > +/* From include/linux/pci.h in the kernel sources */
> > +#define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> > +
> > +#define MAX_IO_REGIONS (6)
> 
> Perhaps a comment to say this is the number of BARs in the config space
> header?

Sure, comments are cheap.

> 
> > +typedef struct {
> > +    int type;           /* Memory or port I/O */
> > +    int valid;
> > +    uint32_t base_addr;
> > +    uint32_t size;    /* size of the region */
> > +    int resource_fd;
> > +} PCIRegion;
> > +
> > +typedef struct {
> > +    uint8_t bus, dev, func; /* Bus inside domain, device and function */
> > +    int irq;                /* IRQ number */
> > +    uint16_t region_number; /* number of active regions */
> > +
> > +    /* Port I/O or MMIO Regions */
> > +    PCIRegion regions[MAX_IO_REGIONS];
> > +    int config_fd;
> > +} PCIDevRegions;
> > +
> > +typedef struct {
> > +    target_phys_addr_t e_physbase;
> > +    uint32_t memory_index;
> > +    union {
> > +        void *r_virtbase;    /* mmapped access address for memory regions */
> > +        uint32_t r_baseport; /* the base guest port for I/O regions */
> > +    } u;
> > +    int num;            /* our index within v_addrs[] */
> > +    uint32_t e_size;    /* emulated size of region in bytes */
> > +    uint32_t r_size;    /* real size of region in bytes */
> > +} AssignedDevRegion;
> > +
> > +typedef struct {
> > +    PCIDevice dev;
> > +    int intpin;
> > +    uint8_t debug_flags;
> > +    AssignedDevRegion v_addrs[PCI_NUM_REGIONS];
> > +    PCIDevRegions real_device;
> > +    int run;
> > +    int girq;
> > +    unsigned char h_busnr;
> > +    unsigned int h_devfn;
> > +    int bound;
> > +} AssignedDevice;
> > +
> > +typedef struct AssignedDevInfo AssignedDevInfo;
> > +
> > +struct AssignedDevInfo {
> > +    char name[15];
> > +    int bus;
> > +    int dev;
> > +    int func;
> > +    AssignedDevice *assigned_dev;
> > +    LIST_ENTRY(AssignedDevInfo) next;
> > +};
> > +
> > +PCIDevice *init_assigned_device(AssignedDevInfo *adev, PCIBus *bus);
> > +AssignedDevInfo *add_assigned_device(const char *arg);
> > +void assigned_dev_set_vector(int irq, int vector);
> > +void assigned_dev_ack_mirq(int vector);
> > +
> > +
> > +#ifdef USE_KVM
> > +int init_all_assigned_devices(PCIBus *bus);
> > +#else /* not using kvm */
> > +static inline int init_all_assigned_devices(PCIBus *bus)
> > +{
> > +    return 0;
> > +}
> > +#endif /* !USE_KVM */
> > +
> > +
> > +#define MAX_DEV_ASSIGN_CMDLINE 8
> > +
> > +extern int device_assignment_enabled;
> 
> > +extern const char *assigned_devices[MAX_DEV_ASSIGN_CMDLINE];
> > +extern int assigned_devices_index;
> 
> Neither of these two are implemented anywhere.

Actually they are, we use them to pass the arguments from main to
pc.c.

> 
> > +#endif              /* __DEVICE_ASSIGNMENT_H__ */
> > diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
> > index d559f0c..5fdb726 100644
> > --- a/qemu/hw/pc.c
> > +++ b/qemu/hw/pc.c
> > @@ -33,6 +33,7 @@
> >  #include "boards.h"
> >  #include "console.h"
> >  #include "fw_cfg.h"
> > +#include "device-assignment.h"
> >  
> >  #include "qemu-kvm.h"
> >  
> > @@ -1157,6 +1158,21 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
> >  
> >      if (pci_enabled)
> >          virtio_balloon_init(pci_bus);
> > +
> > +    if (kvm_enabled() && device_assignment_enabled) {
> 
> The device_assignment_enabled flag looks like it shouldn't be needed.
> 
> If assigned_devices_index remains zero, nothing should happen
> anyway.

Nuked.

> 
> > +	int i;
> > +        for (i = 0; i < assigned_devices_index; i++) {
> > +            if (add_assigned_device(assigned_devices[i]) < 0) {
> > +                fprintf(stderr, "Warning: could not add assigned device %s\n",
> > +                        assigned_devices[i]);
> > +            }
> > +        }
> > +
> > +	if (init_all_assigned_devices(pci_bus)) {
> > +	    fprintf(stderr, "Failed to initialize assigned devices\n");
> > +	    exit (1);
> > +	}
> > +    }
> >  }
> >  
> >  static void pc_init_pci(ram_addr_t ram_size, int vga_ram_size,
> > diff --git a/qemu/hw/pci.c b/qemu/hw/pci.c
> > index c82cd20..f86a8a7 100644
> > --- a/qemu/hw/pci.c
> > +++ b/qemu/hw/pci.c
> > @@ -50,6 +50,7 @@ struct PCIBus {
> >  
> >  static void pci_update_mappings(PCIDevice *d);
> >  static void pci_set_irq(void *opaque, int irq_num, int level);
> > +void assigned_dev_update_irq(PCIDevice *d);
> >  
> >  target_phys_addr_t pci_mem_base;
> >  static int pci_irq_index;
> > @@ -453,6 +454,12 @@ void pci_default_write_config(PCIDevice *d,
> >          val >>= 8;
> >      }
> >  
> > +#ifdef KVM_CAP_DEVICE_ASSIGNMENT
> > +    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel() &&
> > +        address >= 0x60 && address <= 0x63)
> > +        assigned_dev_update_irq(d);
> > +#endif
> 
> Outside of the context of piix_pci.c, it's difficult to figure out what
> the 0x60-0x63 register range relates to - i.e. you need to know to go
> digging in the PIIX spec.
> 
> How about something like in qemu/hw/pc.h:
> 
>   +/* config space register for IRQ routing */
>   +#define PIIX_CONFIG_IRQ_ROUTE 0x60
> 
> then:
> 
>      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel() &&
>          address >= PIIX_CONFIG_IRQ_ROUTE &&
>          address < PIIX_CONFIG_IRQ_ROUTE + 4)

Ok. #define's are cheap too.

Cheers,
Muli
-- 
The First Workshop on I/O Virtualization (WIOV '08)
Dec 2008, San Diego, CA, http://www.usenix.org/wiov08/
                       <->
SYSTOR 2009---The Israeli Experimental Systems Conference
http://www.haifa.il.ibm.com/conferences/systor2009/