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

[Alex Williamson]

On Tue, 04 Jun 2019 11:26:42 -0600
Alex Williamson <alex.williamson@xxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
>  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