lkml.org 
[lkml]   [2009]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v8 1/7] PCI: initialize and release SR-IOV capability
    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.


    \
     
     \ /
      Last update: 2009-02-14 03:51    [W:0.037 / U:89.200 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site