From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60554 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PFT53-0004uy-Lo for qemu-devel@nongnu.org; Mon, 08 Nov 2010 09:52:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PFT52-0004du-Bs for qemu-devel@nongnu.org; Mon, 08 Nov 2010 09:52:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:14509) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PFT52-0004dq-3o for qemu-devel@nongnu.org; Mon, 08 Nov 2010 09:52:20 -0500 From: Alex Williamson In-Reply-To: <20101108112227.GA1075@redhat.com> References: <20101004215311.17070.54862.stgit@s20.home> <20101108112227.GA1075@redhat.com> Content-Type: text/plain; charset="UTF-8" Date: Mon, 08 Nov 2010 07:52:12 -0700 Message-ID: <1289227932.19902.11.camel@x201> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] PCI: Bus number from the bridge, not the device List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: yamahata@valinux.co.jp, qemu-devel@nongnu.org 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 > > Good catch. Applied. > I don't really see why do we put the dev path > in the bus object: why not let device supply its name? Because the device name is not unique. This came about from the discussion about how to create a canonical device path that Gleb and Markus are again trying to hash out. If we go up to the bus and get the bus address, we have a VM unique name. Unfortunately, it's difficult to define what the bus should print in all cases (ISA), but since they don't do hotplug and typically don't allocate ramblocks, we can mostly ignore it for this use case. > And I think this will affect nested bridges. However they are currently > broken anyway: we really must convert to topological names as bus number > is guest-assigned - they don't have to be unique, even. Yes, nested bridges are a problem. How can the seg/bus/devfn not be unique? > What does fixing this involve? Just changing pcibus_get_dev_path? How do you plan to fix it? Don't forget that migration depends on these names, so some kind of compatibility layer would be required. Thanks, Alex > > --- > > > > hw/pci.c | 5 ++--- > > 1 files changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index 6d0934d..15416dd 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -1940,8 +1940,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", > > - indent, "", ctxt, > > - d->config[PCI_SECONDARY_BUS], > > + indent, "", ctxt, pci_bus_num(d->bus), > > 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); > >