[PATCH] PCI/IOV: Fix VF cfg_size

[PATCH] PCI/IOV: Fix VF cfg_size

[Alex Williamson]

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;
 

Re: [PATCH] PCI/IOV: Fix VF cfg_size

[Alex Williamson]

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

Re: [PATCH] PCI/IOV: Fix VF cfg_size

[Bjorn Helgaas]

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

Re: [PATCH] PCI/IOV: Fix VF cfg_size

[Alex Williamson]

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