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

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

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