[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c |
Date: |
Wed, 27 Aug 2014 15:03:54 +0200 |
On Wed, Aug 27, 2014 at 05:13:07PM +0800, Tiejun Chen wrote:
> We will try to reuse assign_dev_load_option_rom in xen side, and
> especially its a good beginning to unify pci assign codes both on
> kvm and xen in the future.
>
> Signed-off-by: Tiejun Chen <address@hidden>
> ---
> hw/i386/kvm/pci-assign.c | 170 +-----------------------------------
> include/hw/pci/pci_assign.h | 204
> ++++++++++++++++++++++++++++++++++++++++++++
Why are you making so much code inline?
Please move it to an out of line file.
Also it should be pci-assign not pci_assign.
> 2 files changed, 207 insertions(+), 167 deletions(-)
> create mode 100644 include/hw/pci/pci_assign.h
>
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index 17c7d6dc..a4fe2d6 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -37,109 +37,7 @@
> #include "hw/pci/pci.h"
> #include "hw/pci/msi.h"
> #include "kvm_i386.h"
> -
> -#define MSIX_PAGE_SIZE 0x1000
> -
> -/* From linux/ioport.h */
> -#define IORESOURCE_IO 0x00000100 /* Resource type */
> -#define IORESOURCE_MEM 0x00000200
> -#define IORESOURCE_IRQ 0x00000400
> -#define IORESOURCE_DMA 0x00000800
> -#define IORESOURCE_PREFETCH 0x00002000 /* No side effects */
> -#define IORESOURCE_MEM_64 0x00100000
> -
> -//#define DEVICE_ASSIGNMENT_DEBUG
> -
> -#ifdef DEVICE_ASSIGNMENT_DEBUG
> -#define DEBUG(fmt, ...) \
> - do { \
> - fprintf(stderr, "%s: " fmt, __func__ , __VA_ARGS__); \
> - } while (0)
> -#else
> -#define DEBUG(fmt, ...)
> -#endif
> -
> -typedef struct PCIRegion {
> - int type; /* Memory or port I/O */
> - int valid;
> - uint64_t base_addr;
> - uint64_t size; /* size of the region */
> - int resource_fd;
> -} PCIRegion;
> -
> -typedef struct PCIDevRegions {
> - 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[PCI_NUM_REGIONS - 1];
> - int config_fd;
> -} PCIDevRegions;
> -
> -typedef struct AssignedDevRegion {
> - MemoryRegion container;
> - MemoryRegion real_iomem;
> - union {
> - uint8_t *r_virtbase; /* mmapped access address for memory regions */
> - uint32_t r_baseport; /* the base guest port for I/O regions */
> - } u;
> - pcibus_t e_size; /* emulated size of region in bytes */
> - pcibus_t r_size; /* real size of region in bytes */
> - PCIRegion *region;
> -} AssignedDevRegion;
> -
> -#define ASSIGNED_DEVICE_PREFER_MSI_BIT 0
> -#define ASSIGNED_DEVICE_SHARE_INTX_BIT 1
> -
> -#define ASSIGNED_DEVICE_PREFER_MSI_MASK (1 << ASSIGNED_DEVICE_PREFER_MSI_BIT)
> -#define ASSIGNED_DEVICE_SHARE_INTX_MASK (1 << ASSIGNED_DEVICE_SHARE_INTX_BIT)
> -
> -typedef struct MSIXTableEntry {
> - uint32_t addr_lo;
> - uint32_t addr_hi;
> - uint32_t data;
> - uint32_t ctrl;
> -} MSIXTableEntry;
> -
> -typedef enum AssignedIRQType {
> - ASSIGNED_IRQ_NONE = 0,
> - ASSIGNED_IRQ_INTX_HOST_INTX,
> - ASSIGNED_IRQ_INTX_HOST_MSI,
> - ASSIGNED_IRQ_MSI,
> - ASSIGNED_IRQ_MSIX
> -} AssignedIRQType;
> -
> -typedef struct AssignedDevice {
> - PCIDevice dev;
> - PCIHostDeviceAddress host;
> - uint32_t dev_id;
> - uint32_t features;
> - int intpin;
> - AssignedDevRegion v_addrs[PCI_NUM_REGIONS - 1];
> - PCIDevRegions real_device;
> - PCIINTxRoute intx_route;
> - AssignedIRQType assigned_irq_type;
> - struct {
> -#define ASSIGNED_DEVICE_CAP_MSI (1 << 0)
> -#define ASSIGNED_DEVICE_CAP_MSIX (1 << 1)
> - uint32_t available;
> -#define ASSIGNED_DEVICE_MSI_ENABLED (1 << 0)
> -#define ASSIGNED_DEVICE_MSIX_ENABLED (1 << 1)
> -#define ASSIGNED_DEVICE_MSIX_MASKED (1 << 2)
> - uint32_t state;
> - } cap;
> - uint8_t emulate_config_read[PCI_CONFIG_SPACE_SIZE];
> - uint8_t emulate_config_write[PCI_CONFIG_SPACE_SIZE];
> - int msi_virq_nr;
> - int *msi_virq;
> - MSIXTableEntry *msix_table;
> - hwaddr msix_table_addr;
> - uint16_t msix_max;
> - MemoryRegion mmio;
> - char *configfd_name;
> - int32_t bootindex;
> -} AssignedDevice;
> +#include "hw/pci/pci_assign.h"
>
> static void assigned_dev_update_irq_routing(PCIDevice *dev);
>
> @@ -1891,72 +1789,10 @@ static void assign_register_types(void)
>
> type_init(assign_register_types)
>
> -/*
> - * Scan the assigned devices for the devices that have an option ROM, and
> then
> - * load the corresponding ROM data to RAM. If an error occurs while loading
> an
> - * option ROM, we just ignore that option ROM and continue with the next one.
> - */
> static void assigned_dev_load_option_rom(AssignedDevice *dev)
> {
> - char name[32], rom_file[64];
> - FILE *fp;
> - uint8_t val;
> - struct stat st;
> void *ptr;
>
> - /* If loading ROM from file, pci handles it */
> - if (dev->dev.romfile || !dev->dev.rom_bar) {
> - return;
> - }
> -
> - snprintf(rom_file, sizeof(rom_file),
> - "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",
> - dev->host.domain, dev->host.bus, dev->host.slot,
> - dev->host.function);
> -
> - if (stat(rom_file, &st)) {
> - return;
> - }
> -
> - if (access(rom_file, F_OK)) {
> - error_report("pci-assign: Insufficient privileges for %s", rom_file);
> - return;
> - }
> -
> - /* Write "1" to the ROM file to enable it */
> - fp = fopen(rom_file, "r+");
> - if (fp == NULL) {
> - return;
> - }
> - val = 1;
> - if (fwrite(&val, 1, 1, fp) != 1) {
> - goto close_rom;
> - }
> - fseek(fp, 0, SEEK_SET);
> -
> - snprintf(name, sizeof(name), "%s.rom",
> - object_get_typename(OBJECT(dev)));
> - memory_region_init_ram(&dev->dev.rom, OBJECT(dev), name, st.st_size);
> - vmstate_register_ram(&dev->dev.rom, &dev->dev.qdev);
> - ptr = memory_region_get_ram_ptr(&dev->dev.rom);
> - memset(ptr, 0xff, st.st_size);
> -
> - if (!fread(ptr, 1, st.st_size, fp)) {
> - error_report("pci-assign: Cannot read from host %s", rom_file);
> - error_printf("Device option ROM contents are probably invalid "
> - "(check dmesg).\nSkip option ROM probe with rombar=0, "
> - "or load from file with romfile=\n");
> - goto close_rom;
> - }
> -
> - pci_register_bar(&dev->dev, PCI_ROM_SLOT, 0, &dev->dev.rom);
> - dev->dev.has_rom = true;
> -close_rom:
> - /* Write "0" to disable ROM */
> - fseek(fp, 0, SEEK_SET);
> - val = 0;
> - if (!fwrite(&val, 1, 1, fp)) {
> - DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
> - }
> - fclose(fp);
> + dev_load_option_rom(&dev->dev, OBJECT(dev), ptr, dev->host.domain,
> + dev->host.bus, dev->host.slot, dev->host.function);
> }
> diff --git a/include/hw/pci/pci_assign.h b/include/hw/pci/pci_assign.h
> new file mode 100644
> index 0000000..b146fcd
> --- /dev/null
> +++ b/include/hw/pci/pci_assign.h
> @@ -0,0 +1,204 @@
> +/*
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + * Just split from hw/i386/kvm/pci-assign.c.
> + */
> +#ifndef PCI_ASSIGN_H
> +#define PCI_ASSIGN_H
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/io.h>
> +#include <sys/mman.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include "hw/hw.h"
> +#include "hw/i386/pc.h"
> +#include "qemu/error-report.h"
> +#include "ui/console.h"
> +#include "hw/loader.h"
> +#include "monitor/monitor.h"
> +#include "qemu/range.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/msi.h"
> +#include "kvm_i386.h"
> +
> +#define MSIX_PAGE_SIZE 0x1000
> +
> +/* From linux/ioport.h */
> +#define IORESOURCE_IO 0x00000100 /* Resource type */
> +#define IORESOURCE_MEM 0x00000200
> +#define IORESOURCE_IRQ 0x00000400
> +#define IORESOURCE_DMA 0x00000800
> +#define IORESOURCE_PREFETCH 0x00002000 /* No side effects */
> +#define IORESOURCE_MEM_64 0x00100000
> +
> +//#define DEVICE_ASSIGNMENT_DEBUG
> +
> +#ifdef DEVICE_ASSIGNMENT_DEBUG
> +#define DEBUG(fmt, ...) \
> + do { \
> + fprintf(stderr, "%s: " fmt, __func__ , __VA_ARGS__); \
> + } while (0)
> +#else
> +#define DEBUG(fmt, ...)
> +#endif
> +
> +typedef struct PCIRegion {
> + int type; /* Memory or port I/O */
> + int valid;
> + uint64_t base_addr;
> + uint64_t size; /* size of the region */
> + int resource_fd;
> +} PCIRegion;
> +
> +typedef struct PCIDevRegions {
> + 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[PCI_NUM_REGIONS - 1];
> + int config_fd;
> +} PCIDevRegions;
> +
> +typedef struct AssignedDevRegion {
> + MemoryRegion container;
> + MemoryRegion real_iomem;
> + union {
> + uint8_t *r_virtbase; /* mmapped access address for memory regions */
> + uint32_t r_baseport; /* the base guest port for I/O regions */
> + } u;
> + pcibus_t e_size; /* emulated size of region in bytes */
> + pcibus_t r_size; /* real size of region in bytes */
> + PCIRegion *region;
> +} AssignedDevRegion;
> +
> +#define ASSIGNED_DEVICE_PREFER_MSI_BIT 0
> +#define ASSIGNED_DEVICE_SHARE_INTX_BIT 1
> +
> +#define ASSIGNED_DEVICE_PREFER_MSI_MASK (1 << ASSIGNED_DEVICE_PREFER_MSI_BIT)
> +#define ASSIGNED_DEVICE_SHARE_INTX_MASK (1 << ASSIGNED_DEVICE_SHARE_INTX_BIT)
> +
> +typedef struct MSIXTableEntry {
> + uint32_t addr_lo;
> + uint32_t addr_hi;
> + uint32_t data;
> + uint32_t ctrl;
> +} MSIXTableEntry;
> +
> +typedef enum AssignedIRQType {
> + ASSIGNED_IRQ_NONE = 0,
> + ASSIGNED_IRQ_INTX_HOST_INTX,
> + ASSIGNED_IRQ_INTX_HOST_MSI,
> + ASSIGNED_IRQ_MSI,
> + ASSIGNED_IRQ_MSIX
> +} AssignedIRQType;
> +
> +typedef struct AssignedDevice {
> + PCIDevice dev;
> + PCIHostDeviceAddress host;
> + uint32_t dev_id;
> + uint32_t features;
> + int intpin;
> + AssignedDevRegion v_addrs[PCI_NUM_REGIONS - 1];
> + PCIDevRegions real_device;
> + PCIINTxRoute intx_route;
> + AssignedIRQType assigned_irq_type;
> + struct {
> +#define ASSIGNED_DEVICE_CAP_MSI (1 << 0)
> +#define ASSIGNED_DEVICE_CAP_MSIX (1 << 1)
> + uint32_t available;
> +#define ASSIGNED_DEVICE_MSI_ENABLED (1 << 0)
> +#define ASSIGNED_DEVICE_MSIX_ENABLED (1 << 1)
> +#define ASSIGNED_DEVICE_MSIX_MASKED (1 << 2)
> + uint32_t state;
> + } cap;
> + uint8_t emulate_config_read[PCI_CONFIG_SPACE_SIZE];
> + uint8_t emulate_config_write[PCI_CONFIG_SPACE_SIZE];
> + int msi_virq_nr;
> + int *msi_virq;
> + MSIXTableEntry *msix_table;
> + hwaddr msix_table_addr;
> + uint16_t msix_max;
> + MemoryRegion mmio;
> + char *configfd_name;
> + int32_t bootindex;
> +} AssignedDevice;
> +
> +/*
> + * Scan the assigned devices for the devices that have an option ROM, and
> then
> + * load the corresponding ROM data to RAM. If an error occurs while loading
> an
> + * option ROM, we just ignore that option ROM and continue with the next one.
> + */
> +static int dev_load_option_rom(PCIDevice *dev, struct Object *owner,
> + void *ptr, unsigned int domain,
> + unsigned int bus, unsigned int slot,
> + unsigned int function)
> +{
> + char name[32], rom_file[64];
> + FILE *fp;
> + uint8_t val;
> + struct stat st;
> + int size = 0;
> +
> + /* If loading ROM from file, pci handles it */
> + if (dev->romfile || !dev->rom_bar) {
> + return -1;
> + }
> +
> + snprintf(rom_file, sizeof(rom_file),
> + "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom",
> + domain, bus, slot, function);
> +
> + if (stat(rom_file, &st)) {
> + return -1;
> + }
> +
> + if (access(rom_file, F_OK)) {
> + error_report("pci-assign: Insufficient privileges for %s", rom_file);
> + return -1;
> + }
> +
> + /* Write "1" to the ROM file to enable it */
> + fp = fopen(rom_file, "r+");
> + if (fp == NULL) {
> + return -1;
> + }
> + val = 1;
> + if (fwrite(&val, 1, 1, fp) != 1) {
> + goto close_rom;
> + }
> + fseek(fp, 0, SEEK_SET);
> +
> + snprintf(name, sizeof(name), "%s.rom", object_get_typename(owner));
> + memory_region_init_ram(&dev->rom, owner, name, st.st_size);
> + vmstate_register_ram(&dev->rom, &dev->qdev);
> + ptr = memory_region_get_ram_ptr(&dev->rom);
> + memset(ptr, 0xff, st.st_size);
> +
> + if (!fread(ptr, 1, st.st_size, fp)) {
> + error_report("pci-assign: Cannot read from host %s", rom_file);
> + error_printf("Device option ROM contents are probably invalid "
> + "(check dmesg).\nSkip option ROM probe with rombar=0, "
> + "or load from file with romfile=\n");
> + goto close_rom;
> + }
> +
> + pci_register_bar(dev, PCI_ROM_SLOT, 0, &dev->rom);
> + dev->has_rom = true;
> + size = st.st_size;
> +close_rom:
> + /* Write "0" to disable ROM */
> + fseek(fp, 0, SEEK_SET);
> + val = 0;
> + if (!fwrite(&val, 1, 1, fp)) {
> + DEBUG("%s\n", "Failed to disable pci-sysfs rom file");
> + }
> + fclose(fp);
> +
> + return size;
> +}
> +#endif /* PCI_ASSIGN_H */
> --
> 1.9.1
- [Qemu-devel] [PATCH 0/1] qemu:pci-assign: try to pci-assign.c, Tiejun Chen, 2014/08/27
- [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c, Tiejun Chen, 2014/08/27
- Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c, Chen, Tiejun, 2014/08/27
- Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c, Chen, Tiejun, 2014/08/28
- Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c, Michael S. Tsirkin, 2014/08/31
- Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c, Chen, Tiejun, 2014/08/31
Re: [Qemu-devel] [PATCH 0/1] qemu:pci-assign: try to pci-assign.c, Laszlo Ersek, 2014/08/27