Re: [Qemu-devel] Re: [PATCH] PCI: Bus number from the bridge, not the device

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2010-12-14 at 06:57 +0200, Michael S. Tsirkin wrote:
> On Mon, Dec 13, 2010 at 09:49:21PM -0700, Alex Williamson wrote:
> > On Tue, 2010-12-14 at 06:46 +0200, Michael S. Tsirkin wrote:
> > > On Mon, Dec 13, 2010 at 01:04:23PM -0700, Alex Williamson wrote:
> > > > On Mon, 2010-11-08 at 13:22 +0200, Michael S. Tsirkin wrote:
> > > > > On Mon, Oct 04, 2010 at 03:53:11PM -0600, Alex Williamson wrote:
> > > > > > pcibus_dev_print() was erroneously retrieving the device bus
> > > > > > number from the secondary bus number offset of the device
> > > > > > instead of the bridge above the device.  This ends of landing
> > > > > > in the 2nd byte of the 3rd BAR for devices, which thankfully
> > > > > > is usually zero.  pcibus_get_dev_path() copied this code,
> > > > > > inheriting the same bug.  pcibus_get_dev_path() is used for
> > > > > > ramblock naming, so changing it can effect migration.  However,
> > > > > > I've only seen this byte be non-zero for an assigned device,
> > > > > > which can't migrate anyway, so hopefully we won't run into
> > > > > > any issues.
> > > > > > 
> > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > 
> > > > > Good catch. Applied.
> > > > 
> > > > Um... submitted vs applied:
> > > > 
> > > >      PCI: Bus number from the bridge, not the device
> > > >  
> > > > @@ -6,20 +8,28 @@
> > > >      number from the secondary bus number offset of the device
> > > >      instead of the bridge above the device.  This ends of landing
> > > >      in the 2nd byte of the 3rd BAR for devices, which thankfully
> > > > -    is usually zero.  pcibus_get_dev_path() copied this code,
> > > > +    is usually zero.
> > > > +    
> > > > +    Note: pcibus_get_dev_path() copied this code,
> > > >      inheriting the same bug.  pcibus_get_dev_path() is used for
> > > >      ramblock naming, so changing it can effect migration.  However,
> > > >      I've only seen this byte be non-zero for an assigned device,
> > > >      which can't migrate anyway, so hopefully we won't run into
> > > >      any issues.
> > > >  
> > > > +    This patch does not touch pcibus_get_dev_path, as
> > > > +    bus number is guest assigned for nested buses,
> > > > +    so using it for migration is broken anyway.
> > > > +    Fix it properly later.
> > > > +    
> > > >      Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > +    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >  
> > > >  diff --git a/hw/pci.c b/hw/pci.c
> > > > -index 6d0934d..15416dd 100644
> > > > +index 962886e..8f6fcf8 100644
> > > >  --- a/hw/pci.c
> > > >  +++ b/hw/pci.c
> > > > -@@ -1940,8 +1940,7 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> > > > +@@ -1806,8 +1806,7 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> > > >   
> > > >       monitor_printf(mon, "%*sclass %s, addr %02x:%02x.%x, "
> > > >                      "pci id %04x:%04x (sub %04x:%04x)\n",
> > > > @@ -29,14 +39,3 @@
> > > >                      PCI_SLOT(d->devfn), PCI_FUNC(d->devfn),
> > > >                      pci_get_word(d->config + PCI_VENDOR_ID),
> > > >                      pci_get_word(d->config + PCI_DEVICE_ID),
> > > > -@@ -1965,7 +1964,7 @@ static char *pcibus_get_dev_path(DeviceState *dev)
> > > > -     char path[16];
> > > > - 
> > > > -     snprintf(path, sizeof(path), "%04x:%02x:%02x.%x",
> > > > --             pci_find_domain(d->bus), d->config[PCI_SECONDARY_BUS],
> > > > -+             pci_find_domain(d->bus), pci_bus_num(d->bus),
> > > > -              PCI_SLOT(d->devfn), PCI_FUNC(d->devfn));
> > > > - 
> > > > -     return strdup(path);
> > > > -
> > > > -
> > > > 
> > > > So the chunk that fixed the part that I was actually interested in got
> > > > dropped even though the existing code is clearly wrong.  Yes, we still
> > > > have issues with nested bridges (not that we have many of those), but
> > > > until the "Fix it properly later" part comes along, can we please
> > > > include the obvious bug fix?  Thanks,
> > > > 
> > > > Alex
> > > 
> > > We can stick 0 in there - would that help?  I would much rather not
> > > create a version where we put the bus number there.
> > 
> > Yep, 0 is good enough until we solve the nested bridge problem.  Thanks,
> > 
> > Alex
> 
> I'm surprised you see that it matters in practice, but ok.
> Like this?

I've only ever seen config[PCI_SECONDARY_BUS] be non-zero for an
assigned device, so I'm pretty sure we're not going to hurt migration,
but the code is clearly wrong and I'd like to make sure we don't trip on
a migration failure for a minor device config space change.

> diff --git a/hw/pci.c b/hw/pci.c
> index 254647b..81231c5 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1952,7 +1952,10 @@ static char *pcibus_get_dev_path(DeviceState *dev)
>      char path[16];
>  
>      snprintf(path, sizeof(path), "%04x:%02x:%02x.%x",
> -             pci_find_domain(d->bus), d->config[PCI_SECONDARY_BUS],
> +             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);

Sure, that's fine.

Acked-by: Alex Williamson <alex.williamson@redhat.com>

Thanks,

Alex