Re: [Qemu-devel] [PATCH 5/8] pci: Replace pci_find_domain() with more general pci_root_bus_path()

[Michael S. Tsirkin]

On Thu, May 09, 2013 at 10:31:09AM +1000, David Gibson wrote:
> pci_find_domain() is used in a number of places where we want an id for a
> whole PCI domain (i.e. the subtree under a PCI root bus).  The trouble is
> that many platforms may support multiple independent host bridges with no
> hardware supplied notion of domain number.
> 
> This patch, therefore, replaces calls to pci_find_domain() with calls to
> a new pci_root_bus_path() returning a string.  The new call is implemented
> in terms of a new callback in the host bridge class, so it can be defined
> in some way that's well defined for the platform.  When no callback is
> available we fall back on the qbus name.
> 
> Most current uses of pci_find_domain() are for error or informational
> messages, so the change in identifiers should be harmless.  The exception
> is pci_get_dev_path(), whose results form part of migration streams.  To
> maintain compatibility with old migration streams, the PIIX PCI host is
> altered to always supply "0000" for this path, which matches the old domain
> number (since the code didn't actually support domains other than 0).
> 
> For the pseries (spapr) PCI bridge we use a different platform-unique
> identifier (pseries machines can routinely have dozens of PCI host
> bridges).  Theoretically that breaks migration streams, but given that we
> don't yet have migration support for pseries, it doesn't matter.
> 
> Any other machines that have working migration support including PCI
> devices will need to be updated to maintain migration stream compatibility.
> 
> Signed-off-by: David Gibson <address@hidden>

AFAIK PC is the only one with working migration, yes, but
we have Q35 as well which can be migrated.

> ---
>  hw/pci-host/piix.c        |    9 +++++++++
>  hw/pci/pci-hotplug-old.c  |    4 ++--
>  hw/pci/pci.c              |   38 ++++++++++++++++++++------------------
>  hw/pci/pci_host.c         |    1 +
>  hw/pci/pcie_aer.c         |    8 ++++----
>  hw/ppc/spapr_pci.c        |   10 ++++++++++
>  include/hw/pci/pci.h      |    2 +-
>  include/hw/pci/pci_host.h |   10 ++++++++++
>  8 files changed, 57 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index f9e68c3..c36e725 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -629,11 +629,20 @@ static const TypeInfo i440fx_info = {
>      .class_init    = i440fx_class_init,
>  };
>  
> +static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> +                                                PCIBus *rootbus)
> +{
> +    /* For backwards compat with old device paths */
> +    return "0000";
> +}
> +
>  static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>  
> +    hc->root_bus_path = i440fx_pcihost_root_bus_path;
>      k->init = i440fx_pcihost_initfn;
>      dc->fw_name = "pci";
>      dc->no_user = 1;
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index 98b4c18..d26674d 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -273,8 +273,8 @@ void pci_device_hot_add(Monitor *mon, const QDict *qdict)
>      }
>  
>      if (dev) {
> -        monitor_printf(mon, "OK domain %d, bus %d, slot %d, function %d\n",
> -                       pci_find_domain(dev),
> +        monitor_printf(mon, "OK root bus %s, bus %d, slot %d, function %d\n",
> +                       pci_root_bus_path(dev),
>                         pci_bus_num(dev->bus), PCI_SLOT(dev->devfn),
>                         PCI_FUNC(dev->devfn));
>      } else
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index f1cee73..a3c192c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -25,6 +25,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/pci/pci_host.h"
>  #include "monitor/monitor.h"
>  #include "net/net.h"
>  #include "sysemu/sysemu.h"
> @@ -270,19 +271,20 @@ PCIBus *pci_device_root_bus(const PCIDevice *d)
>      return bus;
>  }
>  
> -int pci_find_domain(const PCIDevice *dev)
> +const char *pci_root_bus_path(PCIDevice *dev)
>  {
> -    const PCIBus *rootbus = pci_device_root_bus(dev);
> -    struct PCIHostBus *host;
> +    PCIBus *rootbus = pci_device_root_bus(dev);
> +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent);
> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_GET_CLASS(host_bridge);
>  
> -    QLIST_FOREACH(host, &host_buses, next) {
> -        if (host->bus == rootbus) {
> -            return host->domain;
> -        }
> +    assert(!rootbus->parent_dev);
> +    assert(host_bridge->bus == rootbus);
> +
> +    if (hc->root_bus_path) {
> +        return (*hc->root_bus_path)(host_bridge, rootbus);
>      }
>  
> -    abort();    /* should not be reached */
> -    return -1;
> +    return rootbus->qbus.name;
>  }
>  
>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> @@ -2005,10 +2007,10 @@ int pci_add_capability(PCIDevice *pdev, uint8_t 
> cap_id,
>          for (i = offset; i < offset + size; i++) {
>              overlapping_cap = pci_find_capability_at_offset(pdev, i);
>              if (overlapping_cap) {
> -                fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
> +                fprintf(stderr, "ERROR: %s:%02x:%02x.%x "
>                          "Attempt to add PCI capability %x at offset "
>                          "%x overlaps existing capability %x at offset %x\n",
> -                        pci_find_domain(pdev), pci_bus_num(pdev->bus),
> +                        pci_root_bus_path(pdev), pci_bus_num(pdev->bus),
>                          PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
>                          cap_id, offset, overlapping_cap, i);
>                  return -EINVAL;
> @@ -2142,30 +2144,30 @@ static char *pcibus_get_dev_path(DeviceState *dev)
>       * domain:Bus:Slot.Func for systems without nested PCI bridges.
>       * Slot.Function list specifies the slot and function numbers for all
>       * devices on the path from root to the specific device. */
> -    char domain[] = "DDDD:00";
> +    const char *root_bus_path;
> +    int root_bus_len;
>      char slot[] = ":SS.F";
> -    int domain_len = sizeof domain - 1 /* For '\0' */;
>      int slot_len = sizeof slot - 1 /* For '\0' */;
>      int path_len;
>      char *path, *p;
>      int s;
>  
> +    root_bus_path = pci_root_bus_path(d);
> +    root_bus_len = strlen(root_bus_path);
> +
>      /* Calculate # of slots on path between device and root. */;
>      slot_depth = 0;
>      for (t = d; t; t = t->bus->parent_dev) {
>          ++slot_depth;
>      }
>  
> -    path_len = domain_len + slot_len * slot_depth;
> +    path_len = root_bus_len + slot_len * slot_depth;
>  
>      /* Allocate memory, fill in the terminating null byte. */
>      path = g_malloc(path_len + 1 /* For '\0' */);
>      path[path_len] = '\0';
>  
> -    /* First field is the domain. */
> -    s = snprintf(domain, sizeof domain, "%04x:00", pci_find_domain(d));
> -    assert(s == domain_len);
> -    memcpy(path, domain, domain_len);
> +    memcpy(path, root_bus_path, root_bus_len);
>  
>      /* Fill in slot numbers. We walk up from device to root, so need to print
>       * them in the reverse order, last to first. */
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 12254b1..7dd9b25 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -169,6 +169,7 @@ static const TypeInfo pci_host_type_info = {
>      .name = TYPE_PCI_HOST_BRIDGE,
>      .parent = TYPE_SYS_BUS_DEVICE,
>      .abstract = true,
> +    .class_size = sizeof(PCIHostBridgeClass),
>      .instance_size = sizeof(PCIHostState),
>  };
>  
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index 06f77ac..ca762ab 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -817,9 +817,9 @@ void pcie_aer_inject_error_print(Monitor *mon, const 
> QObject *data)
>      qdict = qobject_to_qdict(data);
>  
>      devfn = (int)qdict_get_int(qdict, "devfn");
> -    monitor_printf(mon, "OK id: %s domain: %x, bus: %x devfn: %x.%x\n",
> +    monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n",
>                     qdict_get_str(qdict, "id"),
> -                   (int) qdict_get_int(qdict, "domain"),
> +                   qdict_get_str(qdict, "root_bus"),
>                     (int) qdict_get_int(qdict, "bus"),
>                     PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
> @@ -1020,9 +1020,9 @@ int do_pcie_aer_inject_error(Monitor *mon,
>  
>      ret = pcie_aer_inject_error(dev, &err);
>      *ret_data = qobject_from_jsonf("{'id': %s, "
> -                                   "'domain': %d, 'bus': %d, 'devfn': %d, "
> +                                   "'root_bus': %s, 'bus': %d, 'devfn': %d, "
>                                     "'ret': %d}",
> -                                   id, pci_find_domain(dev),
> +                                   id, pci_root_bus_path(dev),
>                                     pci_bus_num(dev->bus), dev->devfn,
>                                     ret);
>      assert(*ret_data);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 62ff323..2d102f5 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -693,11 +693,21 @@ static Property spapr_phb_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static const char *spapr_phb_root_bus_path(PCIHostState *host_bridge,
> +                                           PCIBus *rootbus)
> +{
> +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(host_bridge);
> +
> +    return sphb->dtbusname;
> +}
> +
>  static void spapr_phb_class_init(ObjectClass *klass, void *data)
>  {
> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>      SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> +    hc->root_bus_path = spapr_phb_root_bus_path;
>      sdc->init = spapr_phb_init;
>      dc->props = spapr_phb_properties;
>      dc->reset = spapr_phb_reset;
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 1383cfe..1b50dc0 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -392,7 +392,7 @@ void pci_for_each_device(PCIBus *bus, int bus_num,
>                           void *opaque);
>  PCIBus *pci_get_primary_bus(void);
>  PCIBus *pci_device_root_bus(const PCIDevice *d);
> -int pci_find_domain(const PCIDevice *dev);
> +const char *pci_root_bus_path(PCIDevice *dev);
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
>  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
>  PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr);
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index 236cd0f..44052f2 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -33,6 +33,10 @@
>  #define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
>  #define PCI_HOST_BRIDGE(obj) \
>      OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST_BRIDGE)
> +#define PCI_HOST_BRIDGE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(PCIHostBridgeClass, (klass), TYPE_PCI_HOST_BRIDGE)
> +#define PCI_HOST_BRIDGE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(PCIHostBridgeClass, (obj), TYPE_PCI_HOST_BRIDGE)
>  
>  struct PCIHostState {
>      SysBusDevice busdev;
> @@ -44,6 +48,12 @@ struct PCIHostState {
>      PCIBus *bus;
>  };
>  
> +typedef struct PCIHostBridgeClass {
> +    SysBusDeviceClass parent_class;
> +
> +    const char *(*root_bus_path)(PCIHostState *, PCIBus *);
> +} PCIHostBridgeClass;
> +
>  /* common internal helpers for PCI/PCIe hosts, cut off overflows */
>  void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
>                                    uint32_t limit, uint32_t val, uint32_t 
> len);
> -- 
> 1.7.10.4