From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43647) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XONsf-0007Xn-6w for qemu-devel@nongnu.org; Mon, 01 Sep 2014 05:26:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XONsZ-000455-Vu for qemu-devel@nongnu.org; Mon, 01 Sep 2014 05:26:33 -0400 Received: from mga11.intel.com ([192.55.52.93]:13238) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XONsZ-00044h-R9 for qemu-devel@nongnu.org; Mon, 01 Sep 2014 05:26:27 -0400 Message-ID: <54043BC0.3070309@intel.com> Date: Mon, 01 Sep 2014 17:26:24 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 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> In-Reply-To: <20140901082742.GA21269@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, aliguori@amazon.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 >> --- [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. > > >> +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 > >