[Qemu-devel] Re: [PATCH repost] pci: fix migration device path for devices behind nested bridges

[Michael S. Tsirkin]

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 <address@hidden>
> > ---
> > 
> > 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