On Sat, Feb 14, 2009 at 01:49:59AM +0800, Matthew Wilcox wrote: > On Fri, Feb 13, 2009 at 05:56:44PM +0100, Andi Kleen wrote: > > > + pci_read_config_word(dev, pos + PCI_SRIOV_CTRL, &ctrl); > > > + if (ctrl & PCI_SRIOV_CTRL_VFE) { > > > + pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, 0); > > > + msleep(100); > > > > That's really long. Hopefully that's really needed. > > Yes and no. The spec says: > > To allow components to perform internal initialization, system software > must wait for at least 100 ms after changing the VF Enable bit from > a 0 to a 1, before it is permitted to issue Configuration Requests to > the VFs which are enabled by that VF Enable bit. > > So we don't have to wait here, but we do have to wait before exposing > all these virtual functions to the rest of the system. Should we add > more complexity, perhaps spawn a thread to do it asynchronously, or add > 0.1 seconds to device initialisation? A question without an easy > answer, iMO. This clears the VF Enable bit only if the BIOS has set it, so it doesn't always happen. Actually the `msleep(100)' should be `ssleep(1)' here, according to the spec you showed us below. I remembered the waiting time incorrectly as 100ms which is the requirment for setting the VF Enable bit rather than clearing it. > > > + > > > + pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl); > > > + pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, total); > > > + pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset); > > > + pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride); > > > + if (!offset || (total > 1 && !stride)) > > > + return -EIO; > > > + > > > + pci_read_config_dword(dev, pos + PCI_SRIOV_SUP_PGSIZE, &pgsz); > > > + i = PAGE_SHIFT > 12 ? PAGE_SHIFT - 12 : 0; > > > + pgsz &= ~((1 << i) - 1); > > > + if (!pgsz) > > > + return -EIO; > > > > All the error paths don't seem to undo the config space writes. > > How will the devices behave with half initialized context? > > I think we should clear the VF_ENABLE bit. That action is also fraught > with danger: The VF Eanble bit hasn't been set yet :-) Actually the spec forbids the s/w to write those registers (NumVFs, Supported Page Size, etc.) when the enabling bit is set. > > If software Clears VF Enable, software must allow 1 second after VF > Enable is Cleared before reading any field in the SR-IOV Extended > Capability or the VF Migration State Array (see Section 3.3.15.1). > > Another msleep(1000) here? Not pretty, but what else can we do? > > Not to mention the danger of something else innocently using lspci -xxxx > to read a field in the extended capability -- I suspect we also need to > block user config accesses before clearing this bit. Yes, we should block user config access. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html