diff mbox

Re: [PATCH] pci: disable migration of p2p bridge

Message ID 20101222105437.GA10771@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Dec. 22, 2010, 10:54 a.m. UTC 
On Wed, Dec 22, 2010 at 05:00:45PM +0900, Isaku Yamahata wrote:
> On Wed, Dec 22, 2010 at 08:27:17AM +0200, Michael S. Tsirkin wrote:
> > On Wed, Dec 22, 2010 at 12:13:43PM +0900, Isaku Yamahata wrote:
> > > Right now pcibus_get_dev_path() isn't migration save because
> > > bus number/secondary bus number are set by guest OS.
> > > So it can't be used reliably for qemu internal id.
> > > 
> > > For 0.14 release, disable p2p bridge migration at the moment.
> > > Once pcibus_get_dev_path() is fixed, this patch should be reverted.
> > > It will be addressed for 0.15 release.
> > > 
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: Alex Williamson <alex.williamson@redhat.com>
> > > Cc: Blue Swirl <blauwirbel@gmail.com>
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > 
> > Hmm, haven't looked into this deeply - can we do this in one place
> > when the bridge is created?
> 
> Unfortunately it's not easy. It requires revising
> register_device_unmigratable(). I have to admit this patch is ugly. 
> 
> This patch is temporal work around and should be reverted eventually.
> So I think it is better to address the original issue (allowing migration
> of p2p bridge) instead of addressing register_device_unmigratable().
> 
> thanks

Hmm. Let's discuss how we will fix pcibus_get_dev_path?
It does not seem to matter how exactly we build it up.
Any way to describe the topology will do I think,
it is slighly ugly to have multiple routines to
build up the path.

For migration, it does not even matter how we describe the domain
as it can't be hot-polugged, we can just use some kind of number.
However, at the moment we don't really support multi-domain
systems, in that there's no way to create such, correct?
So we could use the domain:00:dev.fn:dev.fn:dev.fn syntax as the
natural and backwards compatible way.

What does matter is the commands we give the users for hotplug, these
really are visible.  However, it's not clear that we must support a full
hierarchical path for this.  Maybe the parent bus qdev id is sufficient
for this?

If so the multiple routines to build up the path issue is
a non-issue and we can easily just fix migration...

Sorry about rambling, to summarise let's just apply the below?

---->

pci: fix migration device path for devices behind nested bridges

We were using bus number in the device path, which is clearly
broken as this number is guest-assigned for all devices
except the root.

Fix by using hierarchical list of slot/function numbers, walking
the path from root down to device, instead. Add :00 as bus number
so that if there are no nested bridges, this is compatible
with what we have now.

Note: as pointed out by Gleb, using openfirmware paths
might be cleaner, doing this would break compatibility though.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.c |   44 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 36 insertions(+), 8 deletions(-)
diff mbox

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 8f6fcf8..aed2d42 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1826,15 +1826,41 @@  static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
 
 static char *pcibus_get_dev_path(DeviceState *dev)
 {
-    PCIDevice *d = (PCIDevice *)dev;
-    char path[16];
-
-    snprintf(path, sizeof(path), "%04x:%02x:%02x.%x",
-             pci_find_domain(d->bus),
-             0 /* TODO: need a persistent path for nested buses.
-                * Note: pci_bus_num(d->bus) is not right as it's guest
-                * assigned. */,
-             PCI_SLOT(d->devfn), PCI_FUNC(d->devfn));
-
-    return strdup(path);
+    PCIDevice *d = container_of(dev, PCIDevice, qdev);
+    PCIDevice *t;
+    int slot_depth;
+    /* Path format: Domain:00:Slot.Function:Slot.Function....:Slot.Function.
+     * 00 is added here to make this format compatible with
+     * 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. */
+    int domain_len = strlen("DDDD:00");
+    int slot_len = strlen(":SS.F");
+    int path_len;
+    char *path, *p;
+
+    /* 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;
+
+    /* Allocate memory, fill in the terminating null byte. */
+    path = malloc(path_len + 1 /* For '\0' */);
+    path[path_len] = '\0';
+
+    /* First field is the domain. */
+    snprintf(path, domain_len, "%04x:00", pci_find_domain(d->bus));
+
+    /* Fill in slot numbers. We walk up from device to root, so need to print
+     * them in the reverse order, last to first. */
+    p = path + path_len;
+    for (t = d; t; t = t->bus->parent_dev) {
+        p -= slot_len;
+        snprintf(p, slot_len, ":%02x.%x", PCI_SLOT(t->devfn), PCI_FUNC(d->devfn));
+    }
+
+    return path;
 }