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

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

On 2014/9/1 16:27, Michael S. Tsirkin wrote:
> On Mon, Sep 01, 2014 at 10:07:19AM +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>
>> ---

[snip]

>> + */
>> +#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"
>
> Why are you pulling all these headers here?
> Please include the minimum required.

So just leave #include "hw/pci/pci.h".

>
>> +
>> +#define MSIX_PAGE_SIZE 0x1000
>> +
>> +/* From linux/ioport.h */
>> +#define IORESOURCE_IO       0x00000100  /* Resource type */
>> +#define IORESOURCE_MEM      0x00000200

[snip]

>> +    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;
>> +
>
> Why are you moving the above here?

As I said in the patch head description, I think this is a good 
beginning to unify pci-assign in both KVM and XEN. So I tried to move 
these common stuffs. Although we mightn't use them directly in the 
future, but I guess we still need to move them into this head file.

If you think we should do this on-demand exactly, I can move back them 
to pci-assign.c.

>
>
>> +int dev_load_option_rom(PCIDevice *dev, struct Object *owner, void *ptr,
>> +                        unsigned int domain, unsigned int bus,
>> +                        unsigned int slot, unsigned int function);
>
> Please use a header-specific prefix to avoid global namespace pollution.
> pci_assign_dev_load_option_rom?

Looks good so I will follow-up yours.

Thanks
Tiejun

>
>> +#endif /* PCI_ASSIGN_H */
>> --
>> 1.9.1
>
>