Re: [Qemu-devel] [PATCH 3/8] pci: Abolish pci_find_root_bus()

[Michael S. Tsirkin]

On Thu, May 09, 2013 at 10:31:07AM +1000, David Gibson wrote:
> pci_find_root_bus() takes a domain parameter.  Currently PCI root buses
> with domain other than 0 can't be created, so this is more or less a long
> winded way of retrieving the main PCI root bus.  Numbered domains don't
> actually properly cover the (non x86) possibilities for multiple PCI root
> buses, so this patch for now enforces the domain == 0 restriction in other
> places to replace pci_find_root_bus() with an explicit
> pci_get_primary_bus().
> 
> Signed-off-by: David Gibson <address@hidden>

Can we instead just drop the domain parameter
from pci_find_root_bus and add a comment
saying that it fails if there is more than one root?


> ---
>  hw/pci/pci-hotplug-old.c |   34 +++++++++++++++++++++++++---------
>  hw/pci/pci.c             |   19 +++++++++++++++----
>  include/hw/pci/pci.h     |    2 +-
>  3 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index 1aa0ab8..55441c6 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -34,17 +34,23 @@
>  #include "sysemu/blockdev.h"
>  #include "qapi/error.h"
>  
> -static int pci_read_devaddr(Monitor *mon, const char *addr, int *domp,
> +static int pci_read_devaddr(Monitor *mon, const char *addr,
>                              int *busp, unsigned *slotp)
>  {
> +    int dom;
> +
>      /* strip legacy tag */
>      if (!strncmp(addr, "pci_addr=", 9)) {
>          addr += 9;
>      }
> -    if (pci_parse_devaddr(addr, domp, busp, slotp, NULL)) {
> +    if (pci_parse_devaddr(addr, &dom, busp, slotp, NULL)) {
>          monitor_printf(mon, "Invalid pci address\n");
>          return -1;
>      }
> +    if (dom != 0) {
> +        monitor_printf(mon, "Multiple PCI domains not supported, use 
> device_add\n");
> +        return -1;
> +    }
>      return 0;
>  }
>  
> @@ -126,18 +132,22 @@ static int scsi_hot_add(Monitor *mon, DeviceState 
> *adapter,
>  
>  int pci_drive_hot_add(Monitor *mon, const QDict *qdict, DriveInfo *dinfo)
>  {
> -    int dom, pci_bus;
> +    int pci_bus;
>      unsigned slot;
> +    PCIBus *root = pci_get_primary_bus();
>      PCIDevice *dev;
>      const char *pci_addr = qdict_get_str(qdict, "pci_addr");
>  
>      switch (dinfo->type) {
>      case IF_SCSI:
> -        if (pci_read_devaddr(mon, pci_addr, &dom, &pci_bus, &slot)) {
> +        if (!root) {
> +            monitor_printf(mon, "no primary PCI bus\n");
> +            goto err;
> +        }
> +        if (pci_read_devaddr(mon, pci_addr, &pci_bus, &slot)) {
>              goto err;
>          }
> -        dev = pci_find_device(pci_find_root_bus(dom), pci_bus,
> -                              PCI_DEVFN(slot, 0));
> +        dev = pci_find_device(root, pci_bus, PCI_DEVFN(slot, 0));
>          if (!dev) {
>              monitor_printf(mon, "no pci device with address %s\n", pci_addr);
>              goto err;
> @@ -273,16 +283,22 @@ void pci_device_hot_add(Monitor *mon, const QDict 
> *qdict)
>  
>  static int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
>  {
> +    PCIBus *root = pci_get_primary_bus();
>      PCIDevice *d;
> -    int dom, bus;
> +    int bus;
>      unsigned slot;
>      Error *local_err = NULL;
>  
> -    if (pci_read_devaddr(mon, pci_addr, &dom, &bus, &slot)) {
> +    if (!root) {
> +        monitor_printf(mon, "no primary PCI bus\n");
> +        return -1;
> +    }
> +
> +    if (pci_read_devaddr(mon, pci_addr, &bus, &slot)) {
>          return -1;
>      }
>  
> -    d = pci_find_device(pci_find_root_bus(dom), bus, PCI_DEVFN(slot, 0));
> +    d = pci_find_device(root, bus, PCI_DEVFN(slot, 0));
>      if (!d) {
>          monitor_printf(mon, "slot %d empty\n", slot);
>          return -1;
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 9906e84..9503d56 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -246,12 +246,12 @@ static void pci_host_bus_register(int domain, PCIBus 
> *bus)
>      QLIST_INSERT_HEAD(&host_buses, host, next);
>  }
>  
> -PCIBus *pci_find_root_bus(int domain)
> +PCIBus *pci_get_primary_bus(void)
>  {
>      struct PCIHostBus *host;
>  
>      QLIST_FOREACH(host, &host_buses, next) {
> -        if (host->domain == domain) {
> +        if (host->domain == 0) {
>              return host->bus;
>          }
>      }
> @@ -583,20 +583,31 @@ int pci_parse_devaddr(const char *addr, int *domp, int 
> *busp,
>  
>  PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr)
>  {
> +    PCIBus *root = pci_get_primary_bus();
>      int dom, bus;
>      unsigned slot;
>  
> +    if (!root) {
> +        fprintf(stderr, "No primary PCI bus\n");
> +        return NULL;
> +    }
> +
>      if (!devaddr) {
>          *devfnp = -1;
> -        return pci_find_bus_nr(pci_find_root_bus(0), 0);
> +        return pci_find_bus_nr(root, 0);
>      }
>  
>      if (pci_parse_devaddr(devaddr, &dom, &bus, &slot, NULL) < 0) {
>          return NULL;
>      }
>  
> +    if (dom != 0) {
> +        fprintf(stderr, "No support for non-zero PCI domains\n");
> +        return NULL;
> +    }
> +
>      *devfnp = PCI_DEVFN(slot, 0);
> -    return pci_find_bus_nr(pci_find_root_bus(dom), bus);
> +    return pci_find_bus_nr(root, bus);
>  }
>  
>  static void pci_init_cmask(PCIDevice *dev)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 3ef2ee1..38682e8 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -390,7 +390,7 @@ int pci_bus_num(PCIBus *s);
>  void pci_for_each_device(PCIBus *bus, int bus_num,
>                           void (*fn)(PCIBus *bus, PCIDevice *d, void *opaque),
>                           void *opaque);
> -PCIBus *pci_find_root_bus(int domain);
> +PCIBus *pci_get_primary_bus(void);
>  int pci_find_domain(const PCIBus *bus);
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
>  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
> -- 
> 1.7.10.4