From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42524 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OPb0Q-0004ZV-12 for qemu-devel@nongnu.org; Fri, 18 Jun 2010 08:49:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OPb0O-00013v-8k for qemu-devel@nongnu.org; Fri, 18 Jun 2010 08:49:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41353) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OPb0O-00013X-29 for qemu-devel@nongnu.org; Fri, 18 Jun 2010 08:49:08 -0400 Date: Fri, 18 Jun 2010 15:44:04 +0300 From: "Michael S. Tsirkin" Message-ID: <20100618124404.GA6397@redhat.com> References: <81d134b49ddbd54e82648618c0ce4e4d11fb77c3.1276755023.git.yamahata@valinux.co.jp> <20100617093721.GB7912@redhat.com> <20100618024057.GC14658@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100618024057.GC14658@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH 09/10] pci: set PCI multi-function bit appropriately. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: stefano.stabellini@eu.citrix.com, jan.kiszka@siemens.com, allen.m.kay@intel.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, kraxel@redhat.com, jean.guyader@gmail.com On Fri, Jun 18, 2010 at 11:40:57AM +0900, Isaku Yamahata wrote: > On Thu, Jun 17, 2010 at 12:37:21PM +0300, Michael S. Tsirkin wrote: > > On Thu, Jun 17, 2010 at 03:15:51PM +0900, Isaku Yamahata wrote: > > > set PCI multi-function bit appropriately. > > > > > > Signed-off-by: Isaku Yamahata > > > > > > --- > > > changes v1 -> v2: > > > don't set header type register in configuration space. > > > --- > > > hw/pci.c | 25 +++++++++++++++++++++++++ > > > 1 files changed, 25 insertions(+), 0 deletions(-) > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > index 5316aa5..ee391dc 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -607,6 +607,30 @@ static void pci_init_wmask_bridge(PCIDevice *d) > > > pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff); > > > } > > > > > > +static void pci_init_multifunction(PCIBus *bus, PCIDevice *dev) > > > +{ > > > + uint8_t slot = PCI_SLOT(dev->devfn); > > > + uint8_t func_max = 8; > > > > enum or define would be better > > Will fix. > > > > > + uint8_t func; > > > > If I understand correctly what this does, it goes over > > other functions of the same device, and sets the MULTI_FUNCTION bit > > for them if there's more than one function. > > Instead, why don't we just set PCI_HEADER_TYPE_MULTI_FUNCTION > > in relevant devices? > > pci address, devfn ,is exported to users as addr property, > so users can populate pci function(PCIDevice in qemu) > at arbitrary devfn. > It means each function(PCIDevice) don't know whether pci device > (PCIDevice[8]) is multi function or not. > So I chose to handle it in pci generic layer. > > It can be argued that it's user operation fault and that > the missing part is validation checks to catch such user errors. Exactly. Another part that is missing is a way to hotplug a multifunction device. OTOH I think that hotplug of separate functions has no chance to work, so users are better off getting an error. > But I prefer more flexible and more user friendly way. I think that most users would only add many functions to a device as a result of an error. If we really want the ability to put unrelated devices as functions in a single one, let's just add a 'multifunction' qdev property, and validate that it is set appropriately. > > > > + > > > + for (func = 0; func < func_max; ++func) { > > > + if (bus->devices[PCI_DEVFN(slot, func)]) { > > > + break; > > > + } > > > + } > > > + if (func == func_max) { > > > + return; > > > + } > > > + > > > > The above only works if the function is called before > > device is added to bus. > > That's right. This function is called before bus->devices[devfn] = dev. This needs a comment. > > > > > + for (func = 0; func < func_max; ++func) { > > > + if (bus->devices[PCI_DEVFN(slot, func)]) { > > > + bus->devices[PCI_DEVFN(slot, func)]->config[PCI_HEADER_TYPE] |= > > > + PCI_HEADER_TYPE_MULTI_FUNCTION; > > > + } > > > + } > > > + dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION; > > > > Isn't the bit set above already? > > > > > +} > > > + > > > static void pci_config_alloc(PCIDevice *pci_dev) > > > { > > > int config_size = pci_config_size(pci_dev); > > > @@ -660,6 +684,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > > if (is_bridge) { > > > pci_init_wmask_bridge(pci_dev); > > > } > > > + pci_init_multifunction(bus, pci_dev); > > > > > > if (!config_read) > > > config_read = pci_default_read_config; > > > -- > > > 1.6.6.1 > > > > -- > yamahata