* [PATCH] PCI/IOV: Fix VF cfg_size @ 2019-06-04 17:26 Alex Williamson 2019-06-04 20:36 ` Alex Williamson 0 siblings, 1 reply; 4+ messages in thread From: Alex Williamson @ 2019-06-04 17:26 UTC (permalink / raw) To: linux-pci; +Cc: KarimAllah Ahmed, bhelgaas, linux-kernel 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; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI/IOV: Fix VF cfg_size 2019-06-04 17:26 [PATCH] PCI/IOV: Fix VF cfg_size Alex Williamson @ 2019-06-04 20:36 ` Alex Williamson 2019-06-13 21:20 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Alex Williamson @ 2019-06-04 20:36 UTC (permalink / raw) To: linux-pci; +Cc: KarimAllah Ahmed, bhelgaas, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI/IOV: Fix VF cfg_size 2019-06-04 20:36 ` Alex Williamson @ 2019-06-13 21:20 ` Bjorn Helgaas 2019-06-13 22:01 ` Alex Williamson 0 siblings, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2019-06-13 21:20 UTC (permalink / raw) To: Alex Williamson; +Cc: linux-pci, KarimAllah Ahmed, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI/IOV: Fix VF cfg_size 2019-06-13 21:20 ` Bjorn Helgaas @ 2019-06-13 22:01 ` Alex Williamson 0 siblings, 0 replies; 4+ messages in thread From: Alex Williamson @ 2019-06-13 22:01 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci, KarimAllah Ahmed, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-13 22:01 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-04 17:26 [PATCH] PCI/IOV: Fix VF cfg_size Alex Williamson 2019-06-04 20:36 ` Alex Williamson 2019-06-13 21:20 ` Bjorn Helgaas 2019-06-13 22:01 ` Alex Williamson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).