On Wed, Jan 21, 2015 at 02:00:02PM +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 11, 2014 at 10:20:28AM +0800, Hu Tao wrote:
> > Introduce PCI_DEVFN_AUTO rather than using -1 in code.
> >
> > Signed-off-by: Hu Tao <hu...@cn.fujitsu.com>
> > ---
> > hw/core/qdev-properties.c | 1 +
> > hw/pci/pci.c | 5 ++---
> > include/hw/pci/pci.h | 2 ++
> > 3 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 2e47f70..df4ad14 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -6,6 +6,7 @@
> > #include "net/hub.h"
> > #include "qapi/visitor.h"
> > #include "sysemu/char.h"
> > +#include "hw/pci/pci.h"
> >
> > void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
> > Error **errp)
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 371699c..73c7dec 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -50,7 +50,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev);
> > static void pcibus_reset(BusState *qbus);
> >
> > static Property pci_props[] = {
> > - DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > + DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, PCI_DEVFN_AUTO),
> > DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> > DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> > DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> > @@ -801,7 +801,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
> > address_space_destroy(&pci_dev->bus_master_as);
> > }
> >
> > -/* -1 for devfn means auto assign */
> > static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> > const char *name, int devfn)
> > {
> > @@ -810,7 +809,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> > *pci_dev, PCIBus *bus,
> > PCIConfigWriteFunc *config_write = pc->config_write;
> > AddressSpace *dma_as;
> >
> > - if (devfn < 0) {
> > + if (devfn == PCI_DEVFN_AUTO) {
>
> How do we know we found all places that do this?
Good question! Actually I found other places creating devfn=-1 pci
devices, by calling pci_create_simple_multifunction() and
pci_create_simple(). But I don't have a solid way to discover all these
places. So we should keep the original code here.
Please drop this patch.
Thanks,
Hu
> How do we know parameter was already validate for
> other values < 0?
> Can you pls mention in the commit log why you think
> it's safe?
>
>
> > for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> > devfn += PCI_FUNC_MAX) {
> > if (!bus->devices[devfn])
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index b18759a..9206b12 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -164,6 +164,8 @@ enum {
> > QEMU_PCIE_SLTCAP_PCP = (1 << QEMU_PCIE_SLTCAP_PCP_BITNR),
> > };
> >
> > +#define PCI_DEVFN_AUTO -1
> > +
> > #define TYPE_PCI_DEVICE "pci-device"
> > #define PCI_DEVICE(obj) \
> > OBJECT_CHECK(PCIDevice, (obj), TYPE_PCI_DEVICE)
> > --
> > 1.9.3
