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

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

On 2014/9/1 17:53, Michael S. Tsirkin wrote:
> On Mon, Sep 01, 2014 at 05:26:24PM +0800, Chen, Tiejun wrote:
>> 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.
>
> Yes, I think this is better on demand.

Okay, I'll restore this in next revision.

Thanks
Tiejun

>
>>>
>>>
>>>> +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
>>>
>>>
>