From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39439 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PW3vi-0003Df-V3 for qemu-devel@nongnu.org; Fri, 24 Dec 2010 04:27:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PW3vD-0005I5-3L for qemu-devel@nongnu.org; Fri, 24 Dec 2010 04:26:48 -0500 Received: from mail.valinux.co.jp ([210.128.90.3]:40840) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PW3vC-0005Gx-Ex for qemu-devel@nongnu.org; Fri, 24 Dec 2010 04:26:47 -0500 Date: Fri, 24 Dec 2010 18:26:42 +0900 From: Isaku Yamahata Message-ID: <20101224092642.GH7603@valinux.co.jp> References: <20101224084529.GA23180@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20101224084529.GA23180@redhat.com> Subject: [Qemu-devel] Re: [PATCH repost] pci: fix migration device path for devices behind nested bridges List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Blue Swirl , Alex Williamson , qemu-devel@nongnu.org, gleb@redhat.com The code looks good. Regarding to the format itself, I don't have strong opinion about it. What cames into my mind while I'm looking at the code is, Does BusInfo have to have two path functions? get_dev_path and get_fw_dev_path. Right now only pci supplies get_dev_path, on the other hand get_fw_dev_path covers more buses. This would be another topic, though. On Fri, Dec 24, 2010 at 10:45:29AM +0200, Michael S. Tsirkin wrote: > 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. > > Replace this by a hierarchical list of slot/function numbers, walking > the path from root down to device. Add :00 after the domain 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, > and the IDs used are not guest or user visible at all, > so it probably doesn't matter what they are exactly. > > Signed-off-by: Michael S. Tsirkin > --- > > OK I think I'll just apply the below, it seems better for the > next release than the alternative of disabling migration > completely, and the IDs used are completely transparent to > the user, so it probably doesn't matter what they are exactly. > > hw/pci.c | 44 ++++++++++++++++++++++++++++++++++++-------- > 1 files changed, 36 insertions(+), 8 deletions(-) > > 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; > } > > -- > 1.7.3.2.91.g446ac > -- yamahata