From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=47661 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1ORNUA-0000ep-FX for qemu-devel@nongnu.org; Wed, 23 Jun 2010 06:47:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1ORNU9-00077r-0N for qemu-devel@nongnu.org; Wed, 23 Jun 2010 06:47:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15802) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1ORNU8-00077S-Pw for qemu-devel@nongnu.org; Wed, 23 Jun 2010 06:47:12 -0400 Date: Wed, 23 Jun 2010 13:41:57 +0300 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately. Message-ID: <20100623104157.GA19664@redhat.com> References: <99d849f1b4eeb12893447e78a6950c26a32088ac.1277100005.git.yamahata@valinux.co.jp> <20100621123600.GB10511@redhat.com> <20100623072520.GC3471@valinux.co.jp> <20100623095210.GA9802@redhat.com> <20100623101338.GD3471@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100623101338.GD3471@valinux.co.jp> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: blauwirbel@gmail.com, yu.liu@freescale.com, qemu-devel@nongnu.org, aurelien@aurel32.net, paul@codesourcery.com On Wed, Jun 23, 2010 at 07:13:38PM +0900, Isaku Yamahata wrote: > On Wed, Jun 23, 2010 at 12:52:10PM +0300, Michael S. Tsirkin wrote: > > > > > @@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d) > > > > > pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff); > > > > > } > > > > > > > > > > +static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev) > > > > > +{ > > > > > > > > IMO we should just add in pci_register_device: > > > > > > > > if (d->cap_resent & QEMU_PCI_CAP_MULTIFUNCTION) { > > > > dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION; > > > > } else if (PCI_FUNC(dev->devfn)) { > > > > error_report("PCI: single function device can't be populated %x.%x", > > > > PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn)); > > > > return -1; > > > > } > > > > > > > > And be done with it. > > > > > > Unfortunately there are two ways to set the bit. > > > - set the bit of all the function. > > > Example: Intel X58(north bridge.) > > > - set the bit of only function = 0. > > > Example: PIIX3, PIIX4, ... ICH10. > > > > > > lspci -x would help to see what your pc has. > > > > This is correct: > > The order in which configuration software probes devices residing on a > > bus segment is not specified. Typically, configuration software either > > starts with Device Number 0 and works up or starts at Device Number 31 > > and works down. If a single function device is detected (i.e., bit 7 in > > the Header Type register of function 0 is 0), no more functions for that > > Device Number will be checked. If a multi-function device is detected > > (i.e., bit 7 in the Header Type register of function 0 is 1), then all > > remaining Function Numbers will be checked. > > > > So what my proposal would do is set the bit for all functions. > > I don't think it matters - do you? > > If you want to try and match the behaviour you observe > > in actual hardware exactly, we can add > > /* Some devices only set multifunction status bit in function 0. */ > > static void pci_clear_multifunction(...) { > > if (PCI_FUNC(dev->devfn)) > > dev->config[PCI_HEADER_TYPE] &= ~PCI_HEADER_TYPE_MULTI_FUNCTION; > > } > > > > and devices can call this in their init routine. > > Personally I'm okay with either way as long as you accept the patch series. > > In fact the existing qemu PIIX3/4 sets the bit of only function 0 > and doesn't set the bit of function > 0. > - It would be better not to change the existing behavior. > - If all functions in a device are required to set multifunction bit, > pci ide and ochi usb initialization code must be touched > for pc and mips malta. > Said that, which way do you want to go? > - The current patches.(v5 9/9) > My preference. I think that your patchset is correct, I'll take it after a bit of review. I will try to find a bit of time to rearrange the code in pci.c a bit, but this can come afterwards. I think it's unfortunate that we need to scan the bus to check other devices in the same function, but I don't have better ideas. > - require all functions in a device to set multi function bit. > patch pci ide, ochi usb > It will result in qemu behavior change. > > - require all functions in a device to set multi function bit. > patch pci ide, ochi usb. > But try not to chage the existing qemu behavior by using > pci_clear_multifunction() > > -- > yamahata