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

["Chen, Tiejun" <tiejun.chen@intel.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 <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,
>>>
>>
>> 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
>>>>
>>>
>>>
>>>
>