On 2014/8/31 16:48, Michael S. Tsirkin wrote:
On Fri, Aug 29, 2014 at 09:23:48AM +0800, Chen, Tiejun wrote:
On 2014/8/28 9:44, Chen, Tiejun wrote:
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.c...@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,


Michel,

Is the following change inline fine to you?

Your comments are appreciated in advance.

Thanks
Tiejun

Sorry I don't really understand. Please send a patch and
I can review.

Okay, will send v2 to address your two comments:

* v1 is making so much code inline, so try to move it to an out of line file.
* rename pci-assign not pci_assign.


diff --git a/include/hw/pci/pci-assign.h b/include/hw/pci/pci-assign.h

[snip]


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?

Anyway, if you can provide something about this, I'd like to unify these files as well.

Thanks
Tiejun


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






Reply via email to