| Message ID | 20101224084529.GA23180@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <mst@redhat.com> > --- > > 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 >
On Fri, Dec 24, 2010 at 06:26:42PM +0900, Isaku Yamahata wrote: > 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. Right. I don't have a strong opinion either. We need to make the path used for migration backwards-compatible though, this is the reason we have 2. > 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 <mst@redhat.com> > > --- > > > > 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
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; }
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 <mst@redhat.com> --- 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(-)