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

[Chen, Tiejun]

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 <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.

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