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

[Michael S. Tsirkin]

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 <address@hidden>
> >>---
> 
> [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
> >
> >