From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51130) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOOIg-000378-I3 for qemu-devel@nongnu.org; Mon, 01 Sep 2014 05:53:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOOIb-0005h9-1f for qemu-devel@nongnu.org; Mon, 01 Sep 2014 05:53:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18371) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOOIa-0005gZ-Q4 for qemu-devel@nongnu.org; Mon, 01 Sep 2014 05:53:20 -0400 Date: Mon, 1 Sep 2014 12:53:14 +0300 From: "Michael S. Tsirkin" Message-ID: <20140901095314.GE21269@redhat.com> References: <1409537239-5962-1-git-send-email-tiejun.chen@intel.com> <1409537239-5962-2-git-send-email-tiejun.chen@intel.com> <20140901082742.GA21269@redhat.com> <54043BC0.3070309@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54043BC0.3070309@intel.com> Subject: Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Chen, Tiejun" Cc: qemu-devel@nongnu.org, aliguori@amazon.com 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 > >>--- > > [snip] > > >>+ */ > >>+#ifndef PCI_ASSIGN_H > >>+#define PCI_ASSIGN_H > >>+ > >>+#include > >>+#include > >>+#include > >>+#include > >>+#include > >>+#include > >>+#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 > > > >