Re: [Qemu-devel] [PATCH v3 6/6] pci: introduce PCI_DEVFN_AUTO

[Hu Tao]

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 <address@hidden>
> > ---
> >  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