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.c...@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. > > > > > >>+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 > > > >
