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 kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
