From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58116) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOOut-0005k5-Aw for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:33:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOOuo-00017v-4H for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:32:55 -0400 Received: from mga11.intel.com ([192.55.52.93]:14711) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOOun-00017q-Qk for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:32:50 -0400 Message-ID: <54044B4E.1070000@intel.com> Date: Mon, 01 Sep 2014 18:32:46 +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> <54043BC0.3070309@intel.com> <20140901095314.GE21269@redhat.com> In-Reply-To: <20140901095314.GE21269@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 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 >>>> --- >> >> [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. 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 >>> >>> >