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

[Mark McLoughlin <markmc@redhat.com>]

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;

> +        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()

> +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() ?

> +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]

> + *
> + * 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.

> +    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?

> +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.

> +#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.

> +	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)

> +
>      end = address + len;
>      if (end > PCI_COMMAND && address < (PCI_COMMAND + 2)) {
>          /* if the command register is modified, we must modify the mappings */
> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
> index c5f3f29..5e66832 100644
> --- a/qemu/qemu-kvm.c
> +++ b/qemu/qemu-kvm.c
> @@ -20,6 +20,7 @@ int kvm_pit = 1;
>  #include "console.h"
>  #include "block.h"
>  #include "compatfd.h"
> +#include "hw/device-assignment.h"
>  
>  #include "qemu-kvm.h"
>  #include <libkvm.h>
> @@ -27,6 +28,7 @@ int kvm_pit = 1;
>  #include <sys/utsname.h>
>  #include <sys/syscall.h>
>  #include <sys/mman.h>
> +#include <sys/io.h>
>  
>  #define bool _Bool
>  #define false 0
> @@ -1047,3 +1049,15 @@ int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
>  {
>      return kvm_unregister_coalesced_mmio(kvm_context, addr, size);
>  }
> +
> +static void kvm_do_ioperm(void *_data)
> +{
> +	struct ioperm_data *data = _data;
> +	ioperm(data->start_port, data->num, data->turn_on);
> +}
> +
> +void kvm_ioperm(CPUState *env, void *data)
> +{
> +	if (kvm_enabled() && qemu_system_ready)
> +		on_vcpu(env, kvm_do_ioperm, data);
> +}
> diff --git a/qemu/qemu-kvm.h b/qemu/qemu-kvm.h
> index a1d6646..92d921d 100644
> --- a/qemu/qemu-kvm.h
> +++ b/qemu/qemu-kvm.h
> @@ -93,6 +93,8 @@ int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
>  
>  void qemu_kvm_system_reset_request(void);
>  
> +void kvm_ioperm(CPUState *env, void *data);
> +
>  #ifdef TARGET_PPC
>  int handle_powerpc_dcr_read(int vcpu, uint32_t dcrn, uint32_t *data);
>  int handle_powerpc_dcr_write(int vcpu,uint32_t dcrn, uint32_t data);
> @@ -107,6 +109,12 @@ int handle_powerpc_dcr_write(int vcpu,uint32_t dcrn, uint32_t data);
>  extern int kvm_allowed;
>  extern kvm_context_t kvm_context;
>  
> +struct ioperm_data {
> +	unsigned long start_port;
> +	unsigned long num;
> +	int turn_on;
> +};
> +
>  #define kvm_enabled() (kvm_allowed)
>  #define qemu_kvm_irqchip_in_kernel() kvm_irqchip_in_kernel(kvm_context)
>  #define qemu_kvm_pit_in_kernel() kvm_pit_in_kernel(kvm_context)
> diff --git a/qemu/vl.c b/qemu/vl.c
> index 388e79d..9dda2f9 100644
> --- a/qemu/vl.c
> +++ b/qemu/vl.c
> @@ -38,6 +38,7 @@
>  #include "qemu-char.h"
>  #include "block.h"
>  #include "audio/audio.h"
> +#include "hw/device-assignment.h"
>  #include "migration.h"
>  #include "balloon.h"
>  #include "qemu-kvm.h"
> @@ -215,6 +216,9 @@ CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>  int win2k_install_hack = 0;
>  #endif
>  int usb_enabled = 0;
> +int device_assignment_enabled = 0;
> +const char *assigned_devices[MAX_DEV_ASSIGN_CMDLINE];
> +int assigned_devices_index;
>  static VLANState *first_vlan;
>  int smp_cpus = 1;
>  const char *vnc_display;
> @@ -8692,6 +8696,12 @@ static void help(int exitcode)
>  #endif
>  	   "-no-kvm-irqchip disable KVM kernel mode PIC/IOAPIC/LAPIC\n"
>  	   "-no-kvm-pit	    disable KVM kernel mode PIT\n"
> +#if defined(TARGET_I386) || defined(TARGET_X86_64) || defined(__linux__)
> +           "-pcidevice host=bus:dev.func[,dma=none][,name=\"string\"]\n"

name="string" isn't correct, is it? e.g. this won't work

  -pcidevice host=04:08.0,name="Foo Bar"

but this will:

  -pcidevice "host=04:08.0,name=Foo Bar"

...

Cheers,
Mark.