[PATCH 0/2] PCI/IOV: Resolve regression in SR-IOV VF cfg_size

[PATCH 0/2] PCI/IOV: Resolve regression in SR-IOV VF cfg_size

[Alex Williamson]

The commit reverted in the first patch introduced a regression where
only the first VF reports the correct config space size, subsequent VFs
report 256 bytes of config space.  Replace this in the second patch
with an assumption that all VFs support extended config space by virtue
of the SR-IOV spec requiring a PCIe capability and reachability of the
PF extended config space already being proven by the existence of the
VF.  Thanks,

Alex

---

Alex Williamson (2):
      Revert: PCI/IOV: Use VF0 cached config space size for other VFs
      PCI/IOV: Assume SR-IOV VFs support extended config space.


 drivers/pci/iov.c   |    2 --
 drivers/pci/pci.h   |    1 -
 drivers/pci/probe.c |   26 ++++++++++++--------------
 3 files changed, 12 insertions(+), 17 deletions(-)

[PATCH 1/2] Revert: PCI/IOV: Use VF0 cached config space size for other VFs

[Alex Williamson]

Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for
other VFs") attempts to cache the config space size from the first VF
to re-use for subsequent VFs, but the cached value is determined prior
to discovering the PCIe capability on the VF.  This results in the
first VF reporting the correct config space size (4K), as it has a
special case through pci_cfg_space_size(), while all the other VFs
only report 256 bytes.  As this is only a performance optimization,
we're better off without it.

Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for other VFs")
Cc: KarimAllah Ahmed <karahmed@amazon.de>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Hao Zheng <yinhe@linux.alibaba.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/iov.c   |    2 --
 drivers/pci/pci.h   |    1 -
 drivers/pci/probe.c |   17 -----------------
 3 files changed, 20 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 3aa115ed3a65..525fd3f272b3 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -132,8 +132,6 @@ static void pci_read_vf_config_common(struct pci_dev *virtfn)
 			     &physfn->sriov->subsystem_vendor);
 	pci_read_config_word(virtfn, PCI_SUBSYSTEM_ID,
 			     &physfn->sriov->subsystem_device);
-
-	physfn->sriov->cfg_size = pci_cfg_space_size(virtfn);
 }
 
 int pci_iov_add_virtfn(struct pci_dev *dev, int id)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9cb99380c61e..3fc227ef0815 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -293,7 +293,6 @@ struct pci_sriov {
 	u16		driver_max_VFs;	/* Max num VFs driver supports */
 	struct pci_dev	*dev;		/* Lowest numbered PF */
 	struct pci_dev	*self;		/* This PF */
-	u32		cfg_size;	/* VF config space size */
 	u32		class;		/* VF device */
 	u8		hdr_type;	/* VF header type */
 	u16		subsystem_vendor; /* VF subsystem vendor */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 0e8e2c186f50..a3a3c6b28343 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1555,29 +1555,12 @@ static int pci_cfg_space_size_ext(struct pci_dev *dev)
 	return PCI_CFG_SPACE_EXP_SIZE;
 }
 
-#ifdef CONFIG_PCI_IOV
-static bool is_vf0(struct pci_dev *dev)
-{
-	if (pci_iov_virtfn_devfn(dev->physfn, 0) == dev->devfn &&
-	    pci_iov_virtfn_bus(dev->physfn, 0) == dev->bus->number)
-		return true;
-
-	return false;
-}
-#endif
-
 int pci_cfg_space_size(struct pci_dev *dev)
 {
 	int pos;
 	u32 status;
 	u16 class;
 
-#ifdef CONFIG_PCI_IOV
-	/* Read cached value for all VFs except for VF0 */
-	if (dev->is_virtfn && !is_vf0(dev))
-		return dev->physfn->sriov->cfg_size;
-#endif
-
 	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
 		return PCI_CFG_SPACE_SIZE;
 

[PATCH 2/2] PCI/IOV: Assume SR-IOV VFs support extended config space.

[Alex Williamson]

The SR-IOV specification requires both PFs and VFs to implement a PCIe
capability.  Generally this is sufficient to assume extended config
space is present, but we generally also perform additional tests to
make sure the extended config space is reachable and not simply an
alias of standard config space.  For a VF to exist extended config
space must be accessible on the PF, therefore we can also assume it to
be accessible on the VF.  This enables a micro performance
optimization previously implemented in commit 975bb8b4dc93 ("PCI/IOV:
Use VF0 cached config space size for other VFs") to speed up probing
of VFs.

Cc: KarimAllah Ahmed <karahmed@amazon.de>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Hao Zheng <yinhe@linux.alibaba.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/probe.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a3a3c6b28343..439244ff8f09 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1561,6 +1561,21 @@ int pci_cfg_space_size(struct pci_dev *dev)
 	u32 status;
 	u16 class;
 
+#ifdef CONFIG_PCI_IOV
+	/*
+	 * Per the SR-IOV specification (rev 1.1, sec 3.5), VFs are required to
+	 * implement a PCIe capability and therefore must implement extended
+	 * config space.  We can skip the NO_EXTCFG test below and the
+	 * reachability/aliasing test in pci_cfg_space_size_ext() by virtue of
+	 * the fact that the SR-IOV capability on the PF resides in extended
+	 * config space and must be accessible and non-aliased to have enabled
+	 * support for this VF.  This is a micro performance optimization for
+	 * systems supporting many VFs.
+	 */
+	if (dev->is_virtfn)
+		return PCI_CFG_SPACE_EXP_SIZE;
+#endif
+
 	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
 		return PCI_CFG_SPACE_SIZE;
 

Re: [PATCH 2/2] PCI/IOV: Assume SR-IOV VFs support extended config space.

[sathyanarayanan kuppuswamy]

Hi,

On 6/13/19 3:57 PM, Alex Williamson wrote:
> The SR-IOV specification requires both PFs and VFs to implement a PCIe
> capability.  Generally this is sufficient to assume extended config
> space is present, but we generally also perform additional tests to
> make sure the extended config space is reachable and not simply an
> alias of standard config space.  For a VF to exist extended config
> space must be accessible on the PF, therefore we can also assume it to
> be accessible on the VF.  This enables a micro performance
> optimization previously implemented in commit 975bb8b4dc93 ("PCI/IOV:
> Use VF0 cached config space size for other VFs") to speed up probing
> of VFs.
>
> Cc: KarimAllah Ahmed <karahmed@amazon.de>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Hao Zheng <yinhe@linux.alibaba.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>   drivers/pci/probe.c |   15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index a3a3c6b28343..439244ff8f09 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1561,6 +1561,21 @@ int pci_cfg_space_size(struct pci_dev *dev)
>   	u32 status;
>   	u16 class;
>   
> +#ifdef CONFIG_PCI_IOV
> +	/*
> +	 * Per the SR-IOV specification (rev 1.1, sec 3.5), VFs are required to
> +	 * implement a PCIe capability and therefore must implement extended
> +	 * config space.  We can skip the NO_EXTCFG test below and the
> +	 * reachability/aliasing test in pci_cfg_space_size_ext() by virtue of
> +	 * the fact that the SR-IOV capability on the PF resides in extended
> +	 * config space and must be accessible and non-aliased to have enabled
> +	 * support for this VF.  This is a micro performance optimization for
> +	 * systems supporting many VFs.
> +	 */
> +	if (dev->is_virtfn)
> +		return PCI_CFG_SPACE_EXP_SIZE;
> +#endif

It looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>

> +
>   	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
>   		return PCI_CFG_SPACE_SIZE;
>   
>
>
-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH 0/2] PCI/IOV: Resolve regression in SR-IOV VF cfg_size

[Bjorn Helgaas]

On Thu, Jun 13, 2019 at 04:56:57PM -0600, Alex Williamson wrote:
> The commit reverted in the first patch introduced a regression where
> only the first VF reports the correct config space size, subsequent VFs
> report 256 bytes of config space.  Replace this in the second patch
> with an assumption that all VFs support extended config space by virtue
> of the SR-IOV spec requiring a PCIe capability and reachability of the
> PF extended config space already being proven by the existence of the
> VF.  Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (2):
>       Revert: PCI/IOV: Use VF0 cached config space size for other VFs
>       PCI/IOV: Assume SR-IOV VFs support extended config space.

Applied to pci/virtualization for v5.3 with Kuppuswamy's reviewed-by on
2/2, thanks!

>  drivers/pci/iov.c   |    2 --
>  drivers/pci/pci.h   |    1 -
>  drivers/pci/probe.c |   26 ++++++++++++--------------
>  3 files changed, 12 insertions(+), 17 deletions(-)