Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/8/27 21:03, Michael S. Tsirkin wrote:
> 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 <tiejun.chen@intel.com>
>> ---
>>   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.

So just leave these structs and macros definition here,

diff --git a/include/hw/pci/pci-assign.h b/include/hw/pci/pci-assign.h
new file mode 100644
index 0000000..2778e8b
--- /dev/null
+++ b/include/hw/pci/pci-assign.h
@@ -0,0 +1,123 @@
+/*
+ * 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
+
+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;
+
+int dev_load_option_rom(PCIDevice *dev, struct Object *owner, void *ptr,
+                        unsigned int domain, unsigned int bus,
+                        unsigned int slot, unsigned int function);
+#endif /* PCI_ASSIGN_H */

> Also it should be pci-assign not pci_assign.

Okay, I can rename this as pci-assign.h.

But,

$ ls include/hw/pci/
msi.h   pci_bus.h   pcie.h       pcie_port.h  pci.h       pci_ids.h   shpc.h
msix.h  pci_bridge.h  pcie_aer.h  pcie_host.h  pcie_regs.h  pci_host.h 
pci_regs.h  slotid_cap.h

And you also can see some files are named with xxx_xxx.c/.h in many places.

So what is the rule to name a file in qemu?

Thanks
Tiejun

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