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 <m...@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
