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

["Michael S. Tsirkin" <mst@redhat.com>]

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 <hutao@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?
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