| Message ID | 155966918965.10361.16228304474160813310.stgit@gimli.home (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/IOV: Fix VF cfg_size | expand |
On Tue, 04 Jun 2019 11:26:42 -0600 Alex Williamson <alex.williamson@redhat.com> wrote: > Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > other VFs") attempts to cache the config space size of VF0 to re-use > for all other VFs, but the cache is setup before the call to > pci_setup_device(), where we use set_pcie_port_type() to setup the > pcie_cap field on the struct pci_dev. Without pcie_cap configured, > pci_cfg_space_size() returns PCI_CFG_SPACE_SIZE for the size. VF0 > has a bypass through pci_cfg_space_size(), so its size is reported > correctly, but all subsequent VFs incorrectly report 256 bytes of > config space. > > Resolve by delaying pci_read_vf_config_common() until after > pci_setup_device(). > > Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for other VFs") > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1714978 > Cc: KarimAllah Ahmed <karahmed@amazon.de> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > drivers/pci/iov.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index 3aa115ed3a65..34b1f78f4d31 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -161,13 +161,13 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > virtfn->is_virtfn = 1; > virtfn->physfn = pci_dev_get(dev); > > - if (id == 0) > - pci_read_vf_config_common(virtfn); > - > rc = pci_setup_device(virtfn); > if (rc) > goto failed1; > > + if (id == 0) > + pci_read_vf_config_common(virtfn); > + > virtfn->dev.parent = dev->dev.parent; > virtfn->multifunction = 0; > > Would it actually make more sense to revert 975bb8b4dc93 and just assume any is_virtfn device has PCI_CFG_SPACE_EXP_SIZE for cfg_size? Per the SR-IOV spec, VFs are required to implement a PCIe capability, which should imply 4K of config space. The reachability of that extended config space seems unnecessary to test if we assume that it has the same characteristics as the PF, which must be reachable if we're able to enable SR-IOV. Thoughts? Thanks, Alex
On Tue, Jun 04, 2019 at 02:36:17PM -0600, Alex Williamson wrote: > On Tue, 04 Jun 2019 11:26:42 -0600 > Alex Williamson <alex.williamson@redhat.com> wrote: > > > Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > > other VFs") attempts to cache the config space size of VF0 to re-use > > for all other VFs, but the cache is setup before the call to > > pci_setup_device(), where we use set_pcie_port_type() to setup the > > pcie_cap field on the struct pci_dev. Without pcie_cap configured, > > pci_cfg_space_size() returns PCI_CFG_SPACE_SIZE for the size. VF0 > > has a bypass through pci_cfg_space_size(), so its size is reported > > correctly, but all subsequent VFs incorrectly report 256 bytes of > > config space. > > > > Resolve by delaying pci_read_vf_config_common() until after > > pci_setup_device(). > > > > Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for other VFs") > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1714978 > > Cc: KarimAllah Ahmed <karahmed@amazon.de> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > drivers/pci/iov.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > > index 3aa115ed3a65..34b1f78f4d31 100644 > > --- a/drivers/pci/iov.c > > +++ b/drivers/pci/iov.c > > @@ -161,13 +161,13 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > > virtfn->is_virtfn = 1; > > virtfn->physfn = pci_dev_get(dev); > > > > - if (id == 0) > > - pci_read_vf_config_common(virtfn); > > - > > rc = pci_setup_device(virtfn); > > if (rc) > > goto failed1; > > > > + if (id == 0) > > + pci_read_vf_config_common(virtfn); > > + > > virtfn->dev.parent = dev->dev.parent; > > virtfn->multifunction = 0; > > Would it actually make more sense to revert 975bb8b4dc93 and just > assume any is_virtfn device has PCI_CFG_SPACE_EXP_SIZE for cfg_size? > Per the SR-IOV spec, VFs are required to implement a PCIe capability, > which should imply 4K of config space. The reachability of that > extended config space seems unnecessary to test if we assume that it > has the same characteristics as the PF, which must be reachable if > we're able to enable SR-IOV. Thoughts? Thanks, I like this idea. I first thought maybe we'd still be susceptible to the gotchas described in the pci_cfg_space_size_ext() comment, i.e., we might not have a way to generate extended config space accesses, or the device might be behind a reverse Express bridge. But as you say, SR-IOV is an extended capability that must be located at config offset 0x100 or greater, so the fact that we have a VF at all means we must be able to reach it. Bjorn
On Thu, 13 Jun 2019 16:20:39 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Jun 04, 2019 at 02:36:17PM -0600, Alex Williamson wrote: > > On Tue, 04 Jun 2019 11:26:42 -0600 > > Alex Williamson <alex.williamson@redhat.com> wrote: > > > > > Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > > > other VFs") attempts to cache the config space size of VF0 to re-use > > > for all other VFs, but the cache is setup before the call to > > > pci_setup_device(), where we use set_pcie_port_type() to setup the > > > pcie_cap field on the struct pci_dev. Without pcie_cap configured, > > > pci_cfg_space_size() returns PCI_CFG_SPACE_SIZE for the size. VF0 > > > has a bypass through pci_cfg_space_size(), so its size is reported > > > correctly, but all subsequent VFs incorrectly report 256 bytes of > > > config space. > > > > > > Resolve by delaying pci_read_vf_config_common() until after > > > pci_setup_device(). > > > > > > Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for other VFs") > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1714978 > > > Cc: KarimAllah Ahmed <karahmed@amazon.de> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > --- > > > drivers/pci/iov.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > > > index 3aa115ed3a65..34b1f78f4d31 100644 > > > --- a/drivers/pci/iov.c > > > +++ b/drivers/pci/iov.c > > > @@ -161,13 +161,13 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > > > virtfn->is_virtfn = 1; > > > virtfn->physfn = pci_dev_get(dev); > > > > > > - if (id == 0) > > > - pci_read_vf_config_common(virtfn); > > > - > > > rc = pci_setup_device(virtfn); > > > if (rc) > > > goto failed1; > > > > > > + if (id == 0) > > > + pci_read_vf_config_common(virtfn); > > > + > > > virtfn->dev.parent = dev->dev.parent; > > > virtfn->multifunction = 0; > > > > Would it actually make more sense to revert 975bb8b4dc93 and just > > assume any is_virtfn device has PCI_CFG_SPACE_EXP_SIZE for cfg_size? > > Per the SR-IOV spec, VFs are required to implement a PCIe capability, > > which should imply 4K of config space. The reachability of that > > extended config space seems unnecessary to test if we assume that it > > has the same characteristics as the PF, which must be reachable if > > we're able to enable SR-IOV. Thoughts? Thanks, > > I like this idea. > > I first thought maybe we'd still be susceptible to the gotchas > described in the pci_cfg_space_size_ext() comment, i.e., we might not > have a way to generate extended config space accesses, or the device > might be behind a reverse Express bridge. > > But as you say, SR-IOV is an extended capability that must be located > at config offset 0x100 or greater, so the fact that we have a VF at > all means we must be able to reach it. Cool, I'll send a patch shortly. Thanks, Alex
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 3aa115ed3a65..34b1f78f4d31 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -161,13 +161,13 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) virtfn->is_virtfn = 1; virtfn->physfn = pci_dev_get(dev); - if (id == 0) - pci_read_vf_config_common(virtfn); - rc = pci_setup_device(virtfn); if (rc) goto failed1; + if (id == 0) + pci_read_vf_config_common(virtfn); + virtfn->dev.parent = dev->dev.parent; virtfn->multifunction = 0;
Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for other VFs") attempts to cache the config space size of VF0 to re-use for all other VFs, but the cache is setup before the call to pci_setup_device(), where we use set_pcie_port_type() to setup the pcie_cap field on the struct pci_dev. Without pcie_cap configured, pci_cfg_space_size() returns PCI_CFG_SPACE_SIZE for the size. VF0 has a bypass through pci_cfg_space_size(), so its size is reported correctly, but all subsequent VFs incorrectly report 256 bytes of config space. Resolve by delaying pci_read_vf_config_common() until after pci_setup_device(). Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for other VFs") Link: https://bugzilla.redhat.com/show_bug.cgi?id=1714978 Cc: KarimAllah Ahmed <karahmed@amazon.de> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/pci/iov.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)