From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34842) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOH3I-0004XL-5R for qemu-devel@nongnu.org; Sun, 31 Aug 2014 22:09:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOH3C-0002iP-WA for qemu-devel@nongnu.org; Sun, 31 Aug 2014 22:09:04 -0400 Received: from mga11.intel.com ([192.55.52.93]:49693) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOH3C-0002i4-G4 for qemu-devel@nongnu.org; Sun, 31 Aug 2014 22:08:58 -0400 Message-ID: <5403D522.7030607@intel.com> Date: Mon, 01 Sep 2014 10:08:34 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1409130787-625-1-git-send-email-tiejun.chen@intel.com> <1409130787-625-2-git-send-email-tiejun.chen@intel.com> <20140827130354.GA17769@redhat.com> <53FE8961.3050608@intel.com> <53FFD624.9040603@intel.com> <20140831084806.GB1514@redhat.com> In-Reply-To: <20140831084806.GB1514@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, lcapitulino@redhat.com, alex.williamson@redhat.com, aliguori@amazon.com, pbonzini@redhat.com, lersek@redhat.com 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 >>>>> --- >>>>> 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 >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#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 >>>> >>> >>> >>> >