On Mon, Sep 01, 2014 at 10:08:34AM +0800, Chen, Tiejun wrote: > 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?
In new code, - is prefered to _. > Anyway, if you can provide something about this, I'd like to unify these > files as well. So far we avoided pure coding style cleanups. We only touch what we have to about the file, this makes life easier for downstreams. > 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 > >>>> > >>> > >>> > >>> > >