Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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, back to index

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

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git