[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] hw/pci-assign: split pci-assign.c |
Date: |
Mon, 1 Sep 2014 12:53:14 +0300 |
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
> >
> >