[PATCH v2 0/3] PCI: VF resizable BAR

[PATCH v2 0/3] PCI: VF resizable BAR

[Michał Winiarski]

Hi,

Resurrecting an old series [1], now that we have an in-tree user for it.

For regular BAR, drivers can use pci_resize_resource to resize it to the
desired size provided that it is supported by the hardware, which the
driver can query using pci_rebar_get_possible_sizes.
This series expands the API to work with IOV BAR as well.
It also adds the additional API to allow extending the VF BAR within the
original resource boundary.

Thanks,
-Michał

[1] https://lore.kernel.org/all/20211215141626.3090807-1-michal.winiarski@intel.com/

v1 -> v2:
- Add pci_iov_resource_extend() and usage in Xe driver
- Reduce the number of ifdefs (Christian)
- Drop patch 2/2 from v1 (Christian)
- Add a helper to avoid upsetting static analysis tools (Krzysztof)

Michał Winiarski (3):
  PCI: Add support for VF Resizable Bar extended cap
  PCI: Allow extending VF BAR within original resource boundary
  drm/xe/pf: Extend the VF LMEM BAR

 drivers/gpu/drm/xe/regs/xe_bars.h |   1 +
 drivers/gpu/drm/xe/xe_sriov_pf.c  |   8 ++
 drivers/pci/iov.c                 | 120 +++++++++++++++++++++++++++++-
 drivers/pci/pci.c                 |  40 +++++++++-
 drivers/pci/pci.h                 |  15 +++-
 drivers/pci/setup-res.c           |  44 +++++++++--
 include/linux/pci.h               |   3 +
 include/uapi/linux/pci_regs.h     |   1 +
 8 files changed, 223 insertions(+), 9 deletions(-)

-- 
2.46.0

[PATCH v2 1/3] PCI: Add support for VF Resizable Bar extended cap

[Michał Winiarski]

Similar to regular resizable BAR, VF BAR can also be resized.
The structures are very similar, which means we can reuse most of the
implementation. See PCIe r4.0, sec 9.3.7.4.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/pci/iov.c             | 28 ++++++++++++++++++++++
 drivers/pci/pci.c             | 40 ++++++++++++++++++++++++++++++-
 drivers/pci/pci.h             | 14 ++++++++++-
 drivers/pci/setup-res.c       | 44 ++++++++++++++++++++++++++++++-----
 include/uapi/linux/pci_regs.h |  1 +
 5 files changed, 119 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index aaa33e8dc4c97..e8ccd2ae0f024 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -153,6 +153,34 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
 	return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
 }
 
+bool pci_resource_is_iov(struct pci_dev *dev, int resno)
+{
+	if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
+		return true;
+
+	return false;
+}
+
+void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size)
+{
+	if (!pci_resource_is_iov(dev, resno)) {
+		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
+			 pci_resource_name(dev, resno));
+		return;
+	}
+
+	dev->sriov->barsz[resno - PCI_IOV_RESOURCES] = size;
+}
+
+bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
+{
+	u16 cmd;
+
+	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_CTRL, &cmd);
+
+	return cmd & PCI_SRIOV_CTRL_MSE;
+}
+
 static void pci_read_vf_config_common(struct pci_dev *virtfn)
 {
 	struct pci_dev *physfn = virtfn->physfn;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ffaaca0978cbc..d4522e365e7ba 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1901,6 +1901,35 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
 	}
 }
 
+static void pci_restore_vf_rebar_state(struct pci_dev *pdev)
+{
+	unsigned int pos, nbars, i;
+	u32 ctrl;
+
+	if (!pdev->is_physfn)
+		return;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_VF_REBAR);
+	if (!pos)
+		return;
+
+	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
+	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
+
+	for (i = 0; i < nbars; i++, pos += 8) {
+		struct resource *res;
+		int bar_idx, size;
+
+		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
+		bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
+		res = pdev->resource + bar_idx;
+		size = pci_rebar_bytes_to_size(pdev->sriov->barsz[bar_idx]);
+		ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
+		ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
+		pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
+	}
+}
+
 /**
  * pci_restore_state - Restore the saved state of a PCI device
  * @dev: PCI device that we're dealing with
@@ -1916,6 +1945,7 @@ void pci_restore_state(struct pci_dev *dev)
 	pci_restore_ats_state(dev);
 	pci_restore_vc_state(dev);
 	pci_restore_rebar_state(dev);
+	pci_restore_vf_rebar_state(dev);
 	pci_restore_dpc_state(dev);
 	pci_restore_ptm_state(dev);
 
@@ -3703,10 +3733,18 @@ void pci_acs_init(struct pci_dev *dev)
  */
 static int pci_rebar_find_pos(struct pci_dev *pdev, int bar)
 {
+	int cap = PCI_EXT_CAP_ID_REBAR;
 	unsigned int pos, nbars, i;
 	u32 ctrl;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
+#ifdef CONFIG_PCI_IOV
+	if (pci_resource_is_iov(pdev, bar)) {
+		cap = PCI_EXT_CAP_ID_VF_REBAR;
+		bar -= PCI_IOV_RESOURCES;
+	}
+#endif
+
+	pos = pci_find_ext_capability(pdev, cap);
 	if (!pos)
 		return -ENOTSUPP;
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 79c8398f39384..e763b3fd4c7a2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -556,6 +556,9 @@ void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
 extern const struct attribute_group sriov_pf_dev_attr_group;
 extern const struct attribute_group sriov_vf_dev_attr_group;
+bool pci_resource_is_iov(struct pci_dev *dev, int resno);
+bool pci_iov_memory_decoding_enabled(struct pci_dev *dev);
+void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size);
 #else
 static inline int pci_iov_init(struct pci_dev *dev)
 {
@@ -568,7 +571,16 @@ static inline int pci_iov_bus_range(struct pci_bus *bus)
 {
 	return 0;
 }
-
+static inline bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
+{
+	return false;
+}
+static inline bool pci_resource_is_iov(struct pci_dev *dev, int resno)
+{
+	return false;
+}
+static inline void pci_iov_resource_set_size(struct pci_dev *dev, int resno,
+					     resource_size_t size) { }
 #endif /* CONFIG_PCI_IOV */
 
 #ifdef CONFIG_PCIE_PTM
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index c6d933ddfd464..87a952a114f38 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -427,13 +427,44 @@ void pci_release_resource(struct pci_dev *dev, int resno)
 }
 EXPORT_SYMBOL(pci_release_resource);
 
+static bool pci_memory_decoding_enabled(struct pci_dev *dev)
+{
+	u16 cmd;
+
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+
+	return cmd & PCI_COMMAND_MEMORY;
+}
+
+static int pci_resize_check_memory_decoding(struct pci_dev *dev, int resno)
+{
+	if (!pci_resource_is_iov(dev, resno) && pci_memory_decoding_enabled(dev))
+		return -EBUSY;
+	else if (pci_resource_is_iov(dev, resno) && pci_iov_memory_decoding_enabled(dev))
+		return -EBUSY;
+
+	return 0;
+}
+
+static void pci_resize_resource_set_size(struct pci_dev *dev, int resno, int size)
+{
+	resource_size_t res_size = pci_rebar_size_to_bytes(size);
+	struct resource *res = dev->resource + resno;
+
+	if (!pci_resource_is_iov(dev, resno)) {
+		res->end = res->start + res_size - 1;
+	} else {
+		res->end = res->start + res_size * pci_sriov_get_totalvfs(dev) - 1;
+		pci_iov_resource_set_size(dev, resno, res_size);
+	}
+}
+
 int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 {
 	struct resource *res = dev->resource + resno;
 	struct pci_host_bridge *host;
 	int old, ret;
 	u32 sizes;
-	u16 cmd;
 
 	/* Check if we must preserve the firmware's resource assignment */
 	host = pci_find_host_bridge(dev->bus);
@@ -444,9 +475,9 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 	if (!(res->flags & IORESOURCE_UNSET))
 		return -EBUSY;
 
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	if (cmd & PCI_COMMAND_MEMORY)
-		return -EBUSY;
+	ret = pci_resize_check_memory_decoding(dev, resno);
+	if (ret)
+		return ret;
 
 	sizes = pci_rebar_get_possible_sizes(dev, resno);
 	if (!sizes)
@@ -463,7 +494,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 	if (ret)
 		return ret;
 
-	res->end = res->start + pci_rebar_size_to_bytes(size) - 1;
+	pci_resize_resource_set_size(dev, resno, size);
 
 	/* Check if the new config works by trying to assign everything. */
 	if (dev->bus->self) {
@@ -475,7 +506,8 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 
 error_resize:
 	pci_rebar_set_size(dev, resno, old);
-	res->end = res->start + pci_rebar_size_to_bytes(old) - 1;
+	pci_resize_resource_set_size(dev, resno, old);
+
 	return ret;
 }
 EXPORT_SYMBOL(pci_resize_resource);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 94c00996e633e..cb010008c6bb3 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -738,6 +738,7 @@
 #define PCI_EXT_CAP_ID_L1SS	0x1E	/* L1 PM Substates */
 #define PCI_EXT_CAP_ID_PTM	0x1F	/* Precision Time Measurement */
 #define PCI_EXT_CAP_ID_DVSEC	0x23	/* Designated Vendor-Specific */
+#define PCI_EXT_CAP_ID_VF_REBAR 0x24	/* VF Resizable BAR */
 #define PCI_EXT_CAP_ID_DLF	0x25	/* Data Link Feature */
 #define PCI_EXT_CAP_ID_PL_16GT	0x26	/* Physical Layer 16.0 GT/s */
 #define PCI_EXT_CAP_ID_PL_32GT  0x2A    /* Physical Layer 32.0 GT/s */
-- 
2.46.0

Re: [PATCH v2 1/3] PCI: Add support for VF Resizable Bar extended cap

[kernel test robot]

Hi Michał,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/for-linus]
[also build test WARNING on drm-xe/drm-xe-next drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.11 next-20240919]
[cannot apply to pci/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Micha-Winiarski/PCI-Add-support-for-VF-Resizable-Bar-extended-cap/20240920-064112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git for-linus
patch link:    https://lore.kernel.org/r/20240919223557.1897608-2-michal.winiarski%40intel.com
patch subject: [PATCH v2 1/3] PCI: Add support for VF Resizable Bar extended cap
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240920/202409201629.QlC0MRbn-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240920/202409201629.QlC0MRbn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409201629.QlC0MRbn-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pci/pci.c:1920:20: warning: variable 'res' set but not used [-Wunused-but-set-variable]
    1920 |                 struct resource *res;
         |                                  ^
   1 warning generated.


vim +/res +1920 drivers/pci/pci.c

  1903	
  1904	static void pci_restore_vf_rebar_state(struct pci_dev *pdev)
  1905	{
  1906		unsigned int pos, nbars, i;
  1907		u32 ctrl;
  1908	
  1909		if (!pdev->is_physfn)
  1910			return;
  1911	
  1912		pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_VF_REBAR);
  1913		if (!pos)
  1914			return;
  1915	
  1916		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
  1917		nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
  1918	
  1919		for (i = 0; i < nbars; i++, pos += 8) {
> 1920			struct resource *res;
  1921			int bar_idx, size;
  1922	
  1923			pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
  1924			bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
  1925			res = pdev->resource + bar_idx;
  1926			size = pci_rebar_bytes_to_size(pdev->sriov->barsz[bar_idx]);
  1927			ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
  1928			ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
  1929			pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
  1930		}
  1931	}
  1932	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/3] PCI: Add support for VF Resizable Bar extended cap

[Christian König]

Am 20.09.24 um 00:35 schrieb Michał Winiarski:
> Similar to regular resizable BAR, VF BAR can also be resized.
> The structures are very similar, which means we can reuse most of the
> implementation. See PCIe r4.0, sec 9.3.7.4.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/pci/iov.c             | 28 ++++++++++++++++++++++
>   drivers/pci/pci.c             | 40 ++++++++++++++++++++++++++++++-
>   drivers/pci/pci.h             | 14 ++++++++++-
>   drivers/pci/setup-res.c       | 44 ++++++++++++++++++++++++++++++-----
>   include/uapi/linux/pci_regs.h |  1 +
>   5 files changed, 119 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index aaa33e8dc4c97..e8ccd2ae0f024 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -153,6 +153,34 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
>   	return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
>   }
>   
> +bool pci_resource_is_iov(struct pci_dev *dev, int resno)
> +{
> +	if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
> +		return true;
> +
> +	return false;
> +}

When you want to generalize that check you should probably but it in a 
header and change the existing checks in pci.h and setup-res.c as well. 
Otherwise I don't really see the value in having a separate function.

Additional to that please code that something like "return resno >=...." 
the extra if just increases the number of lines without adding any value.

> +
> +void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size)
> +{
> +	if (!pci_resource_is_iov(dev, resno)) {
> +		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
> +			 pci_resource_name(dev, resno));
> +		return;
> +	}
> +
> +	dev->sriov->barsz[resno - PCI_IOV_RESOURCES] = size;
> +}
> +
> +bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
> +{
> +	u16 cmd;
> +
> +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_CTRL, &cmd);
> +
> +	return cmd & PCI_SRIOV_CTRL_MSE;
> +}
> +
>   static void pci_read_vf_config_common(struct pci_dev *virtfn)
>   {
>   	struct pci_dev *physfn = virtfn->physfn;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ffaaca0978cbc..d4522e365e7ba 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1901,6 +1901,35 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
>   	}
>   }
>   
> +static void pci_restore_vf_rebar_state(struct pci_dev *pdev)
> +{
> +	unsigned int pos, nbars, i;
> +	u32 ctrl;
> +
> +	if (!pdev->is_physfn)
> +		return;
> +
> +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_VF_REBAR);
> +	if (!pos)
> +		return;
> +
> +	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
> +
> +	for (i = 0; i < nbars; i++, pos += 8) {
> +		struct resource *res;
> +		int bar_idx, size;
> +
> +		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +		bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> +		res = pdev->resource + bar_idx;

The variable res seems to be unused.

In general I think you should split up the patch into restoring the VF 
rebar state on resume and implementing the new resize API.

> +		size = pci_rebar_bytes_to_size(pdev->sriov->barsz[bar_idx]);
> +		ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> +		ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
> +		pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> +	}
> +}
> +
>   /**
>    * pci_restore_state - Restore the saved state of a PCI device
>    * @dev: PCI device that we're dealing with
> @@ -1916,6 +1945,7 @@ void pci_restore_state(struct pci_dev *dev)
>   	pci_restore_ats_state(dev);
>   	pci_restore_vc_state(dev);
>   	pci_restore_rebar_state(dev);
> +	pci_restore_vf_rebar_state(dev);
>   	pci_restore_dpc_state(dev);
>   	pci_restore_ptm_state(dev);
>   
> @@ -3703,10 +3733,18 @@ void pci_acs_init(struct pci_dev *dev)
>    */
>   static int pci_rebar_find_pos(struct pci_dev *pdev, int bar)
>   {
> +	int cap = PCI_EXT_CAP_ID_REBAR;
>   	unsigned int pos, nbars, i;
>   	u32 ctrl;
>   
> -	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> +#ifdef CONFIG_PCI_IOV
> +	if (pci_resource_is_iov(pdev, bar)) {
> +		cap = PCI_EXT_CAP_ID_VF_REBAR;
> +		bar -= PCI_IOV_RESOURCES;
> +	}
> +#endif
> +
> +	pos = pci_find_ext_capability(pdev, cap);
>   	if (!pos)
>   		return -ENOTSUPP;
>   
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 79c8398f39384..e763b3fd4c7a2 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -556,6 +556,9 @@ void pci_restore_iov_state(struct pci_dev *dev);
>   int pci_iov_bus_range(struct pci_bus *bus);
>   extern const struct attribute_group sriov_pf_dev_attr_group;
>   extern const struct attribute_group sriov_vf_dev_attr_group;
> +bool pci_resource_is_iov(struct pci_dev *dev, int resno);
> +bool pci_iov_memory_decoding_enabled(struct pci_dev *dev);
> +void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size);
>   #else
>   static inline int pci_iov_init(struct pci_dev *dev)
>   {
> @@ -568,7 +571,16 @@ static inline int pci_iov_bus_range(struct pci_bus *bus)
>   {
>   	return 0;
>   }
> -
> +static inline bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
> +{
> +	return false;
> +}
> +static inline bool pci_resource_is_iov(struct pci_dev *dev, int resno)
> +{
> +	return false;
> +}
> +static inline void pci_iov_resource_set_size(struct pci_dev *dev, int resno,
> +					     resource_size_t size) { }
>   #endif /* CONFIG_PCI_IOV */
>   
>   #ifdef CONFIG_PCIE_PTM
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index c6d933ddfd464..87a952a114f38 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -427,13 +427,44 @@ void pci_release_resource(struct pci_dev *dev, int resno)
>   }
>   EXPORT_SYMBOL(pci_release_resource);
>   
> +static bool pci_memory_decoding_enabled(struct pci_dev *dev)
> +{

I don't really see the value in making it a separate function, just keep 
the check inside the only caller.

> +	u16 cmd;
> +
> +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> +
> +	return cmd & PCI_COMMAND_MEMORY;
> +}
> +
> +static int pci_resize_check_memory_decoding(struct pci_dev *dev, int resno)

Also doesn't look like much value in having that a separate function.

> +{
> +	if (!pci_resource_is_iov(dev, resno) && pci_memory_decoding_enabled(dev))
> +		return -EBUSY;
> +	else if (pci_resource_is_iov(dev, resno) && pci_iov_memory_decoding_enabled(dev))
> +		return -EBUSY;

Well that is coded as ugly as it could be.

I strongly suggest to not call pci_resource_is_iov() twice and to move 
the -EBUSY return code outside of the function (if you really want a 
separate function for that).

E.g. something like "bool pci_resize_is_decoding_enabled(...)" and then 
"if (pci_resize_is_decoding_enabled(...)) return -EBUSY;" in the caller.

Regards,
Christian.

> +
> +	return 0;
> +}
> +
> +static void pci_resize_resource_set_size(struct pci_dev *dev, int resno, int size)
> +{
> +	resource_size_t res_size = pci_rebar_size_to_bytes(size);
> +	struct resource *res = dev->resource + resno;
> +
> +	if (!pci_resource_is_iov(dev, resno)) {
> +		res->end = res->start + res_size - 1;
> +	} else {
> +		res->end = res->start + res_size * pci_sriov_get_totalvfs(dev) - 1;
> +		pci_iov_resource_set_size(dev, resno, res_size);
> +	}
> +}
> +
>   int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>   {
>   	struct resource *res = dev->resource + resno;
>   	struct pci_host_bridge *host;
>   	int old, ret;
>   	u32 sizes;
> -	u16 cmd;
>   
>   	/* Check if we must preserve the firmware's resource assignment */
>   	host = pci_find_host_bridge(dev->bus);
> @@ -444,9 +475,9 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>   	if (!(res->flags & IORESOURCE_UNSET))
>   		return -EBUSY;
>   
> -	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> -	if (cmd & PCI_COMMAND_MEMORY)
> -		return -EBUSY;
> +	ret = pci_resize_check_memory_decoding(dev, resno);
> +	if (ret)
> +		return ret;
>   
>   	sizes = pci_rebar_get_possible_sizes(dev, resno);
>   	if (!sizes)
> @@ -463,7 +494,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>   	if (ret)
>   		return ret;
>   
> -	res->end = res->start + pci_rebar_size_to_bytes(size) - 1;
> +	pci_resize_resource_set_size(dev, resno, size);
>   
>   	/* Check if the new config works by trying to assign everything. */
>   	if (dev->bus->self) {
> @@ -475,7 +506,8 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>   
>   error_resize:
>   	pci_rebar_set_size(dev, resno, old);
> -	res->end = res->start + pci_rebar_size_to_bytes(old) - 1;
> +	pci_resize_resource_set_size(dev, resno, old);
> +
>   	return ret;
>   }
>   EXPORT_SYMBOL(pci_resize_resource);
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 94c00996e633e..cb010008c6bb3 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -738,6 +738,7 @@
>   #define PCI_EXT_CAP_ID_L1SS	0x1E	/* L1 PM Substates */
>   #define PCI_EXT_CAP_ID_PTM	0x1F	/* Precision Time Measurement */
>   #define PCI_EXT_CAP_ID_DVSEC	0x23	/* Designated Vendor-Specific */
> +#define PCI_EXT_CAP_ID_VF_REBAR 0x24	/* VF Resizable BAR */
>   #define PCI_EXT_CAP_ID_DLF	0x25	/* Data Link Feature */
>   #define PCI_EXT_CAP_ID_PL_16GT	0x26	/* Physical Layer 16.0 GT/s */
>   #define PCI_EXT_CAP_ID_PL_32GT  0x2A    /* Physical Layer 32.0 GT/s */

Re: [PATCH v2 1/3] PCI: Add support for VF Resizable Bar extended cap

[Michał Winiarski]

On Fri, Sep 20, 2024 at 11:57:34AM +0200, Christian König wrote:
> Am 20.09.24 um 00:35 schrieb Michał Winiarski:
> > Similar to regular resizable BAR, VF BAR can also be resized.
> > The structures are very similar, which means we can reuse most of the
> > implementation. See PCIe r4.0, sec 9.3.7.4.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > ---
> >   drivers/pci/iov.c             | 28 ++++++++++++++++++++++
> >   drivers/pci/pci.c             | 40 ++++++++++++++++++++++++++++++-
> >   drivers/pci/pci.h             | 14 ++++++++++-
> >   drivers/pci/setup-res.c       | 44 ++++++++++++++++++++++++++++++-----
> >   include/uapi/linux/pci_regs.h |  1 +
> >   5 files changed, 119 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index aaa33e8dc4c97..e8ccd2ae0f024 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -153,6 +153,34 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
> >   	return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
> >   }
> > +bool pci_resource_is_iov(struct pci_dev *dev, int resno)
> > +{
> > +	if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
> > +		return true;
> > +
> > +	return false;
> > +}
> 
> When you want to generalize that check you should probably but it in a
> header and change the existing checks in pci.h and setup-res.c as well.
> Otherwise I don't really see the value in having a separate function.

I'll split it into separate patch that's changing the existing checks.

> 
> Additional to that please code that something like "return resno >=...." the
> extra if just increases the number of lines without adding any value.

Ok.

> 
> > +
> > +void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size)
> > +{
> > +	if (!pci_resource_is_iov(dev, resno)) {
> > +		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
> > +			 pci_resource_name(dev, resno));
> > +		return;
> > +	}
> > +
> > +	dev->sriov->barsz[resno - PCI_IOV_RESOURCES] = size;
> > +}
> > +
> > +bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
> > +{
> > +	u16 cmd;
> > +
> > +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_CTRL, &cmd);
> > +
> > +	return cmd & PCI_SRIOV_CTRL_MSE;
> > +}
> > +
> >   static void pci_read_vf_config_common(struct pci_dev *virtfn)
> >   {
> >   	struct pci_dev *physfn = virtfn->physfn;
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index ffaaca0978cbc..d4522e365e7ba 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1901,6 +1901,35 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> >   	}
> >   }
> > +static void pci_restore_vf_rebar_state(struct pci_dev *pdev)
> > +{
> > +	unsigned int pos, nbars, i;
> > +	u32 ctrl;
> > +
> > +	if (!pdev->is_physfn)
> > +		return;
> > +
> > +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_VF_REBAR);
> > +	if (!pos)
> > +		return;
> > +
> > +	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> > +	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
> > +
> > +	for (i = 0; i < nbars; i++, pos += 8) {
> > +		struct resource *res;
> > +		int bar_idx, size;
> > +
> > +		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> > +		bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> > +		res = pdev->resource + bar_idx;
> 
> The variable res seems to be unused.
> 
> In general I think you should split up the patch into restoring the VF rebar
> state on resume and implementing the new resize API.

I'll split it into separate patch.

> 
> > +		size = pci_rebar_bytes_to_size(pdev->sriov->barsz[bar_idx]);
> > +		ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> > +		ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
> > +		pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> > +	}
> > +}
> > +
> >   /**
> >    * pci_restore_state - Restore the saved state of a PCI device
> >    * @dev: PCI device that we're dealing with
> > @@ -1916,6 +1945,7 @@ void pci_restore_state(struct pci_dev *dev)
> >   	pci_restore_ats_state(dev);
> >   	pci_restore_vc_state(dev);
> >   	pci_restore_rebar_state(dev);
> > +	pci_restore_vf_rebar_state(dev);
> >   	pci_restore_dpc_state(dev);
> >   	pci_restore_ptm_state(dev);
> > @@ -3703,10 +3733,18 @@ void pci_acs_init(struct pci_dev *dev)
> >    */
> >   static int pci_rebar_find_pos(struct pci_dev *pdev, int bar)
> >   {
> > +	int cap = PCI_EXT_CAP_ID_REBAR;
> >   	unsigned int pos, nbars, i;
> >   	u32 ctrl;
> > -	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> > +#ifdef CONFIG_PCI_IOV
> > +	if (pci_resource_is_iov(pdev, bar)) {
> > +		cap = PCI_EXT_CAP_ID_VF_REBAR;
> > +		bar -= PCI_IOV_RESOURCES;
> > +	}
> > +#endif
> > +
> > +	pos = pci_find_ext_capability(pdev, cap);
> >   	if (!pos)
> >   		return -ENOTSUPP;
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 79c8398f39384..e763b3fd4c7a2 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -556,6 +556,9 @@ void pci_restore_iov_state(struct pci_dev *dev);
> >   int pci_iov_bus_range(struct pci_bus *bus);
> >   extern const struct attribute_group sriov_pf_dev_attr_group;
> >   extern const struct attribute_group sriov_vf_dev_attr_group;
> > +bool pci_resource_is_iov(struct pci_dev *dev, int resno);
> > +bool pci_iov_memory_decoding_enabled(struct pci_dev *dev);
> > +void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size);
> >   #else
> >   static inline int pci_iov_init(struct pci_dev *dev)
> >   {
> > @@ -568,7 +571,16 @@ static inline int pci_iov_bus_range(struct pci_bus *bus)
> >   {
> >   	return 0;
> >   }
> > -
> > +static inline bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
> > +{
> > +	return false;
> > +}
> > +static inline bool pci_resource_is_iov(struct pci_dev *dev, int resno)
> > +{
> > +	return false;
> > +}
> > +static inline void pci_iov_resource_set_size(struct pci_dev *dev, int resno,
> > +					     resource_size_t size) { }
> >   #endif /* CONFIG_PCI_IOV */
> >   #ifdef CONFIG_PCIE_PTM
> > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> > index c6d933ddfd464..87a952a114f38 100644
> > --- a/drivers/pci/setup-res.c
> > +++ b/drivers/pci/setup-res.c
> > @@ -427,13 +427,44 @@ void pci_release_resource(struct pci_dev *dev, int resno)
> >   }
> >   EXPORT_SYMBOL(pci_release_resource);
> > +static bool pci_memory_decoding_enabled(struct pci_dev *dev)
> > +{
> 
> I don't really see the value in making it a separate function, just keep the
> check inside the only caller.

Ok.

> 
> > +	u16 cmd;
> > +
> > +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > +
> > +	return cmd & PCI_COMMAND_MEMORY;
> > +}
> > +
> > +static int pci_resize_check_memory_decoding(struct pci_dev *dev, int resno)
> 
> Also doesn't look like much value in having that a separate function.
> 
> > +{
> > +	if (!pci_resource_is_iov(dev, resno) && pci_memory_decoding_enabled(dev))
> > +		return -EBUSY;
> > +	else if (pci_resource_is_iov(dev, resno) && pci_iov_memory_decoding_enabled(dev))
> > +		return -EBUSY;
> 
> Well that is coded as ugly as it could be.
> 
> I strongly suggest to not call pci_resource_is_iov() twice and to move the
> -EBUSY return code outside of the function (if you really want a separate
> function for that).
> 
> E.g. something like "bool pci_resize_is_decoding_enabled(...)" and then "if
> (pci_resize_is_decoding_enabled(...)) return -EBUSY;" in the caller.

Ok.

Thanks,
-Michał

> 
> Regards,
> Christian.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void pci_resize_resource_set_size(struct pci_dev *dev, int resno, int size)
> > +{
> > +	resource_size_t res_size = pci_rebar_size_to_bytes(size);
> > +	struct resource *res = dev->resource + resno;
> > +
> > +	if (!pci_resource_is_iov(dev, resno)) {
> > +		res->end = res->start + res_size - 1;
> > +	} else {
> > +		res->end = res->start + res_size * pci_sriov_get_totalvfs(dev) - 1;
> > +		pci_iov_resource_set_size(dev, resno, res_size);
> > +	}
> > +}
> > +
> >   int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> >   {
> >   	struct resource *res = dev->resource + resno;
> >   	struct pci_host_bridge *host;
> >   	int old, ret;
> >   	u32 sizes;
> > -	u16 cmd;
> >   	/* Check if we must preserve the firmware's resource assignment */
> >   	host = pci_find_host_bridge(dev->bus);
> > @@ -444,9 +475,9 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> >   	if (!(res->flags & IORESOURCE_UNSET))
> >   		return -EBUSY;
> > -	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > -	if (cmd & PCI_COMMAND_MEMORY)
> > -		return -EBUSY;
> > +	ret = pci_resize_check_memory_decoding(dev, resno);
> > +	if (ret)
> > +		return ret;
> >   	sizes = pci_rebar_get_possible_sizes(dev, resno);
> >   	if (!sizes)
> > @@ -463,7 +494,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> >   	if (ret)
> >   		return ret;
> > -	res->end = res->start + pci_rebar_size_to_bytes(size) - 1;
> > +	pci_resize_resource_set_size(dev, resno, size);
> >   	/* Check if the new config works by trying to assign everything. */
> >   	if (dev->bus->self) {
> > @@ -475,7 +506,8 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> >   error_resize:
> >   	pci_rebar_set_size(dev, resno, old);
> > -	res->end = res->start + pci_rebar_size_to_bytes(old) - 1;
> > +	pci_resize_resource_set_size(dev, resno, old);
> > +
> >   	return ret;
> >   }
> >   EXPORT_SYMBOL(pci_resize_resource);
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index 94c00996e633e..cb010008c6bb3 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -738,6 +738,7 @@
> >   #define PCI_EXT_CAP_ID_L1SS	0x1E	/* L1 PM Substates */
> >   #define PCI_EXT_CAP_ID_PTM	0x1F	/* Precision Time Measurement */
> >   #define PCI_EXT_CAP_ID_DVSEC	0x23	/* Designated Vendor-Specific */
> > +#define PCI_EXT_CAP_ID_VF_REBAR 0x24	/* VF Resizable BAR */
> >   #define PCI_EXT_CAP_ID_DLF	0x25	/* Data Link Feature */
> >   #define PCI_EXT_CAP_ID_PL_16GT	0x26	/* Physical Layer 16.0 GT/s */
> >   #define PCI_EXT_CAP_ID_PL_32GT  0x2A    /* Physical Layer 32.0 GT/s */
> 

[PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[Michał Winiarski]

VF MMIO resource reservation, either created by system firmware and
inherited by Linux PCI subsystem or created by the subsystem itself,
contains enough space to fit the BAR of all SR-IOV Virtual Functions
that can potentially be created (total VFs supported by the device).
This can be leveraged when the device is exposing lower than optimal BAR
size as a default, allowing access to the entire resource when lower
number of VFs are created.
It is achieved by dynamically resizing the BAR to largest possible value
that allows to fit all newly created VFs within the original resource
boundary.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/pci/iov.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci.h   |  1 +
 include/linux/pci.h |  3 ++
 3 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index e8ccd2ae0f024..d88efbfa70e42 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
 	return cmd & PCI_SRIOV_CTRL_MSE;
 }
 
+static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs)
+{
+	resource_size_t size;
+	int ret, old, i;
+	u32 sizes;
+
+	pci_config_pm_runtime_get(dev);
+
+	if (pci_iov_memory_decoding_enabled(dev)) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	sizes = pci_rebar_get_possible_sizes(dev, resno);
+	if (!sizes) {
+		ret = -ENOTSUPP;
+		goto err;
+	}
+
+	old = pci_rebar_get_current_size(dev, resno);
+	if (old < 0) {
+		ret = old;
+		goto err;
+	}
+
+	while (sizes > 0) {
+		i = __fls(sizes);
+		size = pci_rebar_size_to_bytes(i);
+		if (size * num_vfs <= pci_resource_len(dev, resno)) {
+			if (i != old) {
+				ret = pci_rebar_set_size(dev, resno, size);
+				if (ret)
+					goto err;
+
+				pci_iov_resource_set_size(dev, resno, size);
+				pci_iov_update_resource(dev, resno);
+			}
+			break;
+		}
+		sizes &= ~BIT(i);
+	}
+
+	pci_config_pm_runtime_put(dev);
+
+	return;
+
+err:
+	dev_WARN(&dev->dev, "Failed to extend %s: %d\n",
+		 pci_resource_name(dev, resno), ret);
+
+	pci_config_pm_runtime_put(dev);
+}
+
+static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno)
+{
+	if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES])
+		pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs);
+}
+
+int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
+{
+	if (!pci_resource_is_iov(dev, resno)) {
+		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
+			 pci_resource_name(dev, resno));
+
+		return -ENODEV;
+	}
+
+	if (!pci_rebar_get_possible_sizes(dev, resno))
+		return -ENOTSUPP;
+
+	if (!enable)
+		pci_iov_resource_do_restore(dev, resno);
+
+	dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES] = enable;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_iov_resource_extend);
+
 static void pci_read_vf_config_common(struct pci_dev *virtfn)
 {
 	struct pci_dev *physfn = virtfn->physfn;
@@ -445,7 +525,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 				  const char *buf, size_t count)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	int ret = 0;
+	int i, ret = 0;
 	u16 num_vfs;
 
 	if (kstrtou16(buf, 0, &num_vfs) < 0)
@@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 		goto exit;
 	}
 
+	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
+		if (pdev->sriov->rebar_extend[i])
+			pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs);
+	}
+
 	ret = pdev->driver->sriov_configure(pdev, num_vfs);
 	if (ret < 0)
 		goto exit;
@@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos)
 
 static void sriov_release(struct pci_dev *dev)
 {
+	int i;
+
 	BUG_ON(dev->sriov->num_VFs);
 
+	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
+		pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES);
+
 	if (dev != dev->sriov->dev)
 		pci_dev_put(dev->sriov->dev);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e763b3fd4c7a2..47ed2633232aa 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -385,6 +385,7 @@ struct pci_sriov {
 	u16		subsystem_vendor; /* VF subsystem vendor */
 	u16		subsystem_device; /* VF subsystem device */
 	resource_size_t	barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
+	bool		rebar_extend[PCI_SRIOV_NUM_BARS];	/* Resize VF BAR */
 	bool		drivers_autoprobe; /* Auto probing of VFs by driver */
 };
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4cf89a4b4cbcf..c007119da7b3d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
 int pci_sriov_get_totalvfs(struct pci_dev *dev);
 int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
 resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
+int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable);
 void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
 
 /* Arch may override these (weak) */
@@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
 #define pci_sriov_configure_simple	NULL
 static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
 { return 0; }
+static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
+{ return -ENODEV; }
 static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
 #endif
 
-- 
2.46.0

Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[Christian König]

Am 20.09.24 um 00:35 schrieb Michał Winiarski:
> VF MMIO resource reservation, either created by system firmware and
> inherited by Linux PCI subsystem or created by the subsystem itself,
> contains enough space to fit the BAR of all SR-IOV Virtual Functions
> that can potentially be created (total VFs supported by the device).
> This can be leveraged when the device is exposing lower than optimal BAR
> size as a default, allowing access to the entire resource when lower
> number of VFs are created.
> It is achieved by dynamically resizing the BAR to largest possible value
> that allows to fit all newly created VFs within the original resource
> boundary.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/pci/iov.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++-
>   drivers/pci/pci.h   |  1 +
>   include/linux/pci.h |  3 ++
>   3 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index e8ccd2ae0f024..d88efbfa70e42 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
>   	return cmd & PCI_SRIOV_CTRL_MSE;
>   }
>   
> +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs)
> +{
> +	resource_size_t size;
> +	int ret, old, i;
> +	u32 sizes;
> +
> +	pci_config_pm_runtime_get(dev);
> +
> +	if (pci_iov_memory_decoding_enabled(dev)) {
> +		ret = -EBUSY;
> +		goto err;
> +	}
> +
> +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> +	if (!sizes) {
> +		ret = -ENOTSUPP;
> +		goto err;
> +	}
> +
> +	old = pci_rebar_get_current_size(dev, resno);
> +	if (old < 0) {
> +		ret = old;
> +		goto err;
> +	}
> +
> +	while (sizes > 0) {
> +		i = __fls(sizes);
> +		size = pci_rebar_size_to_bytes(i);
> +		if (size * num_vfs <= pci_resource_len(dev, resno)) {
> +			if (i != old) {
> +				ret = pci_rebar_set_size(dev, resno, size);
> +				if (ret)
> +					goto err;
> +
> +				pci_iov_resource_set_size(dev, resno, size);
> +				pci_iov_update_resource(dev, resno);
> +			}
> +			break;
> +		}
> +		sizes &= ~BIT(i);
> +	}
> +
> +	pci_config_pm_runtime_put(dev);
> +
> +	return;
> +
> +err:
> +	dev_WARN(&dev->dev, "Failed to extend %s: %d\n",
> +		 pci_resource_name(dev, resno), ret);
> +
> +	pci_config_pm_runtime_put(dev);
> +}
> +
> +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno)
> +{
> +	if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES])
> +		pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs);
> +}
> +
> +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> +{
> +	if (!pci_resource_is_iov(dev, resno)) {
> +		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
> +			 pci_resource_name(dev, resno));
> +
> +		return -ENODEV;
> +	}
> +
> +	if (!pci_rebar_get_possible_sizes(dev, resno))
> +		return -ENOTSUPP;
> +
> +	if (!enable)
> +		pci_iov_resource_do_restore(dev, resno);
> +
> +	dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES] = enable;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_iov_resource_extend);
> +
>   static void pci_read_vf_config_common(struct pci_dev *virtfn)
>   {
>   	struct pci_dev *physfn = virtfn->physfn;
> @@ -445,7 +525,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>   				  const char *buf, size_t count)
>   {
>   	struct pci_dev *pdev = to_pci_dev(dev);
> -	int ret = 0;
> +	int i, ret = 0;
>   	u16 num_vfs;
>   
>   	if (kstrtou16(buf, 0, &num_vfs) < 0)
> @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>   		goto exit;
>   	}
>   
> +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> +		if (pdev->sriov->rebar_extend[i])
> +			pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs);
> +	}
> +

That sounds like a really bad idea to me.

Basically the suggestion is here that the PCI subsystem should silently 
extend and shrink the VF BARs when the number of VFs change?

Bjorn has the last word on that but I would say that instead the driver 
owning the PCIe device as hypervisor should resize the VF BARs to a 
desired size and that in turn restricts the number of VFs you can enable.

Regards,
Christian.

>   	ret = pdev->driver->sriov_configure(pdev, num_vfs);
>   	if (ret < 0)
>   		goto exit;
> @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos)
>   
>   static void sriov_release(struct pci_dev *dev)
>   {
> +	int i;
> +
>   	BUG_ON(dev->sriov->num_VFs);
>   
> +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> +		pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES);
> +
>   	if (dev != dev->sriov->dev)
>   		pci_dev_put(dev->sriov->dev);
>   
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e763b3fd4c7a2..47ed2633232aa 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -385,6 +385,7 @@ struct pci_sriov {
>   	u16		subsystem_vendor; /* VF subsystem vendor */
>   	u16		subsystem_device; /* VF subsystem device */
>   	resource_size_t	barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
> +	bool		rebar_extend[PCI_SRIOV_NUM_BARS];	/* Resize VF BAR */
>   	bool		drivers_autoprobe; /* Auto probing of VFs by driver */
>   };
>   
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4cf89a4b4cbcf..c007119da7b3d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>   int pci_sriov_get_totalvfs(struct pci_dev *dev);
>   int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
>   resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
> +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable);
>   void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
>   
>   /* Arch may override these (weak) */
> @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>   #define pci_sriov_configure_simple	NULL
>   static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
>   { return 0; }
> +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> +{ return -ENODEV; }
>   static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
>   #endif
>   

Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[Michał Winiarski]

On Fri, Sep 20, 2024 at 12:07:34PM +0200, Christian König wrote:
> Am 20.09.24 um 00:35 schrieb Michał Winiarski:
> > VF MMIO resource reservation, either created by system firmware and
> > inherited by Linux PCI subsystem or created by the subsystem itself,
> > contains enough space to fit the BAR of all SR-IOV Virtual Functions
> > that can potentially be created (total VFs supported by the device).
> > This can be leveraged when the device is exposing lower than optimal BAR
> > size as a default, allowing access to the entire resource when lower
> > number of VFs are created.
> > It is achieved by dynamically resizing the BAR to largest possible value
> > that allows to fit all newly created VFs within the original resource
> > boundary.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > ---
> >   drivers/pci/iov.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++-
> >   drivers/pci/pci.h   |  1 +
> >   include/linux/pci.h |  3 ++
> >   3 files changed, 95 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index e8ccd2ae0f024..d88efbfa70e42 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
> >   	return cmd & PCI_SRIOV_CTRL_MSE;
> >   }
> > +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs)
> > +{
> > +	resource_size_t size;
> > +	int ret, old, i;
> > +	u32 sizes;
> > +
> > +	pci_config_pm_runtime_get(dev);
> > +
> > +	if (pci_iov_memory_decoding_enabled(dev)) {
> > +		ret = -EBUSY;
> > +		goto err;
> > +	}
> > +
> > +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> > +	if (!sizes) {
> > +		ret = -ENOTSUPP;
> > +		goto err;
> > +	}
> > +
> > +	old = pci_rebar_get_current_size(dev, resno);
> > +	if (old < 0) {
> > +		ret = old;
> > +		goto err;
> > +	}
> > +
> > +	while (sizes > 0) {
> > +		i = __fls(sizes);
> > +		size = pci_rebar_size_to_bytes(i);
> > +		if (size * num_vfs <= pci_resource_len(dev, resno)) {
> > +			if (i != old) {
> > +				ret = pci_rebar_set_size(dev, resno, size);
> > +				if (ret)
> > +					goto err;
> > +
> > +				pci_iov_resource_set_size(dev, resno, size);
> > +				pci_iov_update_resource(dev, resno);
> > +			}
> > +			break;
> > +		}
> > +		sizes &= ~BIT(i);
> > +	}
> > +
> > +	pci_config_pm_runtime_put(dev);
> > +
> > +	return;
> > +
> > +err:
> > +	dev_WARN(&dev->dev, "Failed to extend %s: %d\n",
> > +		 pci_resource_name(dev, resno), ret);
> > +
> > +	pci_config_pm_runtime_put(dev);
> > +}
> > +
> > +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno)
> > +{
> > +	if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES])
> > +		pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs);
> > +}
> > +
> > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> > +{
> > +	if (!pci_resource_is_iov(dev, resno)) {
> > +		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
> > +			 pci_resource_name(dev, resno));
> > +
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (!pci_rebar_get_possible_sizes(dev, resno))
> > +		return -ENOTSUPP;
> > +
> > +	if (!enable)
> > +		pci_iov_resource_do_restore(dev, resno);
> > +
> > +	dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES] = enable;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_iov_resource_extend);
> > +
> >   static void pci_read_vf_config_common(struct pci_dev *virtfn)
> >   {
> >   	struct pci_dev *physfn = virtfn->physfn;
> > @@ -445,7 +525,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
> >   				  const char *buf, size_t count)
> >   {
> >   	struct pci_dev *pdev = to_pci_dev(dev);
> > -	int ret = 0;
> > +	int i, ret = 0;
> >   	u16 num_vfs;
> >   	if (kstrtou16(buf, 0, &num_vfs) < 0)
> > @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
> >   		goto exit;
> >   	}
> > +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > +		if (pdev->sriov->rebar_extend[i])
> > +			pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs);
> > +	}
> > +
> 
> That sounds like a really bad idea to me.
> 
> Basically the suggestion is here that the PCI subsystem should silently
> extend and shrink the VF BARs when the number of VFs change?

Why do you think it's a bad idea? Everything is under PCI subsystem
control and the driver in charge has to explicitly opt-in to this
behavior on a per-BAR basis.

> Bjorn has the last word on that but I would say that instead the driver
> owning the PCIe device as hypervisor should resize the VF BARs to a desired
> size and that in turn restricts the number of VFs you can enable.

Then the PCI subsystem would silently change the driver_max_VFs (or new
variable, as driver_max_VFs is under PF control, so it's either new var
or checking VF BAR size in pci_sriov_set_totalvfs).
It also means that we have to do the maths to calculate the new VF limit
in both PCI subsystem and the caller.

We can go this route as well - I just think it's cleaner to keep this
all under PCI subsystem control.

I'll keep the current behavior in v3, but I'm open to changing it.

Thanks,
-Michał

> 
> Regards,
> Christian.
> 
> >   	ret = pdev->driver->sriov_configure(pdev, num_vfs);
> >   	if (ret < 0)
> >   		goto exit;
> > @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos)
> >   static void sriov_release(struct pci_dev *dev)
> >   {
> > +	int i;
> > +
> >   	BUG_ON(dev->sriov->num_VFs);
> > +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> > +		pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES);
> > +
> >   	if (dev != dev->sriov->dev)
> >   		pci_dev_put(dev->sriov->dev);
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index e763b3fd4c7a2..47ed2633232aa 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -385,6 +385,7 @@ struct pci_sriov {
> >   	u16		subsystem_vendor; /* VF subsystem vendor */
> >   	u16		subsystem_device; /* VF subsystem device */
> >   	resource_size_t	barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
> > +	bool		rebar_extend[PCI_SRIOV_NUM_BARS];	/* Resize VF BAR */
> >   	bool		drivers_autoprobe; /* Auto probing of VFs by driver */
> >   };
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 4cf89a4b4cbcf..c007119da7b3d 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >   int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >   int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
> >   resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
> > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable);
> >   void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
> >   /* Arch may override these (weak) */
> > @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >   #define pci_sriov_configure_simple	NULL
> >   static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
> >   { return 0; }
> > +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> > +{ return -ENODEV; }
> >   static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
> >   #endif
> 

Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[Christian König]

Re-sending this as text from my private mail account since the AMD 
servers now seem to convert everything to HTML ^^.

Christian.

Am 11.10.24 um 10:57 schrieb Christian König:
> Am 10.10.24 um 10:59 schrieb Michał Winiarski:
>> On Fri, Sep 20, 2024 at 12:07:34PM +0200, Christian König wrote:
>>> Am 20.09.24 um 00:35 schrieb Michał Winiarski:
>>>> [SNIP]
>>>> @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>>>>    		goto exit;
>>>>    	}
>>>> +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>>>> +		if (pdev->sriov->rebar_extend[i])
>>>> +			pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs);
>>>> +	}
>>>> +
>>> That sounds like a really bad idea to me.
>>>
>>> Basically the suggestion is here that the PCI subsystem should silently
>>> extend and shrink the VF BARs when the number of VFs change?
>> Why do you think it's a bad idea? Everything is under PCI subsystem
>> control and the driver in charge has to explicitly opt-in to this
>> behavior on a per-BAR basis.
>
> And exactly that's a bad idea. The PCI subsystem shouldn't control 
> this, the driver should.
>
> At least for some devices we have tons of interactions with ACPI and 
> EFI. Only the driver does know for example when platform drivers which 
> might be in the way for a resize have been unloaded.
>
> From the past experience BAR resize should only be triggered by the 
> driver and never from the PCI subsystem while scanning the bus or 
> probing devices.
>
>>> Bjorn has the last word on that but I would say that instead the driver
>>> owning the PCIe device as hypervisor should resize the VF BARs to a desired
>>> size and that in turn restricts the number of VFs you can enable.
>> Then the PCI subsystem would silently change the driver_max_VFs (or new
>> variable, as driver_max_VFs is under PF control, so it's either new var
>> or checking VF BAR size in pci_sriov_set_totalvfs).
>
> Nope, the PCI subsystem should not magically adjust anything.
>
> What should happen instead is that the driver would call 
> pci_enable_sriov() with the number of virtual functions to enable and 
> the PCI subsystem then validates that number and return -EINVAL or 
> -ENOSPC if it won't work.
>
>> It also means that we have to do the maths to calculate the new VF limit
>> in both PCI subsystem and the caller.
>
> Well the point is that those calculations are different.
>
> What the subsystem does is to validate if with the number of requested 
> virtual functions the necessary resources will fit into the allocate 
> space.
>
> What the driver does previously is to either change the allocate space 
> or calculate the other way around and determine the maximum virtual 
> functions from the space available.
>
>> We can go this route as well - I just think it's cleaner to keep this
>> all under PCI subsystem control.
>
> I think that would be much cleaner, especially the PCI subsystem 
> shouldn't adjust any values given from the driver or even more general 
> overrule decisions the driver made.
>
> Instead proper error codes should be returned if some values don't 
> make sense or the subsystem isn't able to move around BARs currently 
> in use etc...
>
> Regards,
> Christian.
>
>> I'll keep the current behavior in v3, but I'm open to changing it.
>>
>> Thanks,
>> -Michał
>>
>>> Regards,
>>> Christian.
>>>
>>>>    	ret = pdev->driver->sriov_configure(pdev, num_vfs);
>>>>    	if (ret < 0)
>>>>    		goto exit;
>>>> @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>>>    static void sriov_release(struct pci_dev *dev)
>>>>    {
>>>> +	int i;
>>>> +
>>>>    	BUG_ON(dev->sriov->num_VFs);
>>>> +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
>>>> +		pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES);
>>>> +
>>>>    	if (dev != dev->sriov->dev)
>>>>    		pci_dev_put(dev->sriov->dev);
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index e763b3fd4c7a2..47ed2633232aa 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -385,6 +385,7 @@ struct pci_sriov {
>>>>    	u16		subsystem_vendor; /* VF subsystem vendor */
>>>>    	u16		subsystem_device; /* VF subsystem device */
>>>>    	resource_size_t	barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
>>>> +	bool		rebar_extend[PCI_SRIOV_NUM_BARS];	/* Resize VF BAR */
>>>>    	bool		drivers_autoprobe; /* Auto probing of VFs by driver */
>>>>    };
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 4cf89a4b4cbcf..c007119da7b3d 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>>>    int pci_sriov_get_totalvfs(struct pci_dev *dev);
>>>>    int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
>>>>    resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
>>>> +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable);
>>>>    void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
>>>>    /* Arch may override these (weak) */
>>>> @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>>>    #define pci_sriov_configure_simple	NULL
>>>>    static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
>>>>    { return 0; }
>>>> +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
>>>> +{ return -ENODEV; }
>>>>    static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
>>>>    #endif
>

Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[kernel test robot]

Hi Michał,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/for-linus]
[also build test WARNING on drm-xe/drm-xe-next drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.11 next-20240920]
[cannot apply to pci/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Micha-Winiarski/PCI-Add-support-for-VF-Resizable-Bar-extended-cap/20240920-064112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git for-linus
patch link:    https://lore.kernel.org/r/20240919223557.1897608-3-michal.winiarski%40intel.com
patch subject: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary
config: arc-allnoconfig (https://download.01.org/0day-ci/archive/20240920/202409201854.z0daqyYE-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240920/202409201854.z0daqyYE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409201854.z0daqyYE-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/pci/of.c:12:
   include/linux/pci.h: In function 'pci_iov_resource_extend':
>> include/linux/pci.h:2421:10: warning: 'return' with a value, in function returning void [-Wreturn-type]
    2421 | { return -ENODEV; }
         |          ^
   include/linux/pci.h:2420:20: note: declared here
    2420 | static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
         |                    ^~~~~~~~~~~~~~~~~~~~~~~


vim +/return +2421 include/linux/pci.h

  2397	
  2398	static inline int pci_iov_sysfs_link(struct pci_dev *dev,
  2399					     struct pci_dev *virtfn, int id)
  2400	{
  2401		return -ENODEV;
  2402	}
  2403	static inline int pci_iov_add_virtfn(struct pci_dev *dev, int id)
  2404	{
  2405		return -ENOSYS;
  2406	}
  2407	static inline void pci_iov_remove_virtfn(struct pci_dev *dev,
  2408						 int id) { }
  2409	static inline void pci_disable_sriov(struct pci_dev *dev) { }
  2410	static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
  2411	static inline int pci_vfs_assigned(struct pci_dev *dev)
  2412	{ return 0; }
  2413	static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
  2414	{ return 0; }
  2415	static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
  2416	{ return 0; }
  2417	#define pci_sriov_configure_simple	NULL
  2418	static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
  2419	{ return 0; }
  2420	static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> 2421	{ return -ENODEV; }
  2422	static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
  2423	#endif
  2424	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[kernel test robot]

Hi Michał,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/for-linus]
[also build test WARNING on drm-xe/drm-xe-next drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.11 next-20240920]
[cannot apply to pci/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Micha-Winiarski/PCI-Add-support-for-VF-Resizable-Bar-extended-cap/20240920-064112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git for-linus
patch link:    https://lore.kernel.org/r/20240919223557.1897608-3-michal.winiarski%40intel.com
patch subject: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary
config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20240920/202409201934.yM9hVUai-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240920/202409201934.yM9hVUai-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409201934.yM9hVUai-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/arm/mm/iomap.c:9:
>> include/linux/pci.h:2421:3: warning: void function 'pci_iov_resource_extend' should not return a value [-Wreturn-mismatch]
    2421 | { return -ENODEV; }
         |   ^      ~~~~~~~
   In file included from arch/arm/mm/iomap.c:9:
   In file included from include/linux/pci.h:2672:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   2 warnings generated.


vim +/pci_iov_resource_extend +2421 include/linux/pci.h

  2397	
  2398	static inline int pci_iov_sysfs_link(struct pci_dev *dev,
  2399					     struct pci_dev *virtfn, int id)
  2400	{
  2401		return -ENODEV;
  2402	}
  2403	static inline int pci_iov_add_virtfn(struct pci_dev *dev, int id)
  2404	{
  2405		return -ENOSYS;
  2406	}
  2407	static inline void pci_iov_remove_virtfn(struct pci_dev *dev,
  2408						 int id) { }
  2409	static inline void pci_disable_sriov(struct pci_dev *dev) { }
  2410	static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
  2411	static inline int pci_vfs_assigned(struct pci_dev *dev)
  2412	{ return 0; }
  2413	static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
  2414	{ return 0; }
  2415	static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
  2416	{ return 0; }
  2417	#define pci_sriov_configure_simple	NULL
  2418	static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
  2419	{ return 0; }
  2420	static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> 2421	{ return -ENODEV; }
  2422	static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
  2423	#endif
  2424	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 6108 bytes --]

On Fri, 20 Sep 2024, Michał Winiarski wrote:

> VF MMIO resource reservation, either created by system firmware and
> inherited by Linux PCI subsystem or created by the subsystem itself,
> contains enough space to fit the BAR of all SR-IOV Virtual Functions
> that can potentially be created (total VFs supported by the device).
> This can be leveraged when the device is exposing lower than optimal BAR
> size as a default, allowing access to the entire resource when lower
> number of VFs are created.
> It is achieved by dynamically resizing the BAR to largest possible value
> that allows to fit all newly created VFs within the original resource
> boundary.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/pci/iov.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/pci/pci.h   |  1 +
>  include/linux/pci.h |  3 ++
>  3 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index e8ccd2ae0f024..d88efbfa70e42 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
>  	return cmd & PCI_SRIOV_CTRL_MSE;
>  }
>  
> +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs)
> +{
> +	resource_size_t size;
> +	int ret, old, i;
> +	u32 sizes;
> +
> +	pci_config_pm_runtime_get(dev);
> +
> +	if (pci_iov_memory_decoding_enabled(dev)) {
> +		ret = -EBUSY;
> +		goto err;
> +	}
> +
> +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> +	if (!sizes) {
> +		ret = -ENOTSUPP;
> +		goto err;
> +	}
> +
> +	old = pci_rebar_get_current_size(dev, resno);
> +	if (old < 0) {
> +		ret = old;
> +		goto err;
> +	}
> +
> +	while (sizes > 0) {
> +		i = __fls(sizes);
> +		size = pci_rebar_size_to_bytes(i);
> +		if (size * num_vfs <= pci_resource_len(dev, resno)) {
> +			if (i != old) {
> +				ret = pci_rebar_set_size(dev, resno, size);
> +				if (ret)
> +					goto err;
> +
> +				pci_iov_resource_set_size(dev, resno, size);
> +				pci_iov_update_resource(dev, resno);
> +			}
> +			break;
> +		}
> +		sizes &= ~BIT(i);
> +	}
> +
> +	pci_config_pm_runtime_put(dev);
> +
> +	return;
> +
> +err:
> +	dev_WARN(&dev->dev, "Failed to extend %s: %d\n",
> +		 pci_resource_name(dev, resno), ret);

Why do you use dev_WARN()? (analoguous to WARN_ON() / friends).

I suppose you'd want to use pci_warn() instead.

> +	pci_config_pm_runtime_put(dev);
> +}
> +
> +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno)
> +{
> +	if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES])
> +		pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs);
> +}
> +
> +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> +{
> +	if (!pci_resource_is_iov(dev, resno)) {
> +		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
> +			 pci_resource_name(dev, resno));

pci_warn() ?

-- 
 i.

> +
> +		return -ENODEV;
> +	}
> +
> +	if (!pci_rebar_get_possible_sizes(dev, resno))
> +		return -ENOTSUPP;
> +
> +	if (!enable)
> +		pci_iov_resource_do_restore(dev, resno);
> +
> +	dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES] = enable;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_iov_resource_extend);
> +
>  static void pci_read_vf_config_common(struct pci_dev *virtfn)
>  {
>  	struct pci_dev *physfn = virtfn->physfn;
> @@ -445,7 +525,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>  				  const char *buf, size_t count)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	int ret = 0;
> +	int i, ret = 0;
>  	u16 num_vfs;
>  
>  	if (kstrtou16(buf, 0, &num_vfs) < 0)
> @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>  		goto exit;
>  	}
>  
> +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> +		if (pdev->sriov->rebar_extend[i])
> +			pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs);
> +	}
> +
>  	ret = pdev->driver->sriov_configure(pdev, num_vfs);
>  	if (ret < 0)
>  		goto exit;
> @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos)
>  
>  static void sriov_release(struct pci_dev *dev)
>  {
> +	int i;
> +
>  	BUG_ON(dev->sriov->num_VFs);
>  
> +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> +		pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES);
> +
>  	if (dev != dev->sriov->dev)
>  		pci_dev_put(dev->sriov->dev);
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e763b3fd4c7a2..47ed2633232aa 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -385,6 +385,7 @@ struct pci_sriov {
>  	u16		subsystem_vendor; /* VF subsystem vendor */
>  	u16		subsystem_device; /* VF subsystem device */
>  	resource_size_t	barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
> +	bool		rebar_extend[PCI_SRIOV_NUM_BARS];	/* Resize VF BAR */
>  	bool		drivers_autoprobe; /* Auto probing of VFs by driver */
>  };
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4cf89a4b4cbcf..c007119da7b3d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
>  int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
>  resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
> +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable);
>  void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
>  
>  /* Arch may override these (weak) */
> @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
>  #define pci_sriov_configure_simple	NULL
>  static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
>  { return 0; }
> +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> +{ return -ENODEV; }
>  static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
>  #endif
>  
> 

Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[Michał Winiarski]

On Fri, Sep 20, 2024 at 02:30:00PM +0300, Ilpo Järvinen wrote:
> On Fri, 20 Sep 2024, Michał Winiarski wrote:
> 
> > VF MMIO resource reservation, either created by system firmware and
> > inherited by Linux PCI subsystem or created by the subsystem itself,
> > contains enough space to fit the BAR of all SR-IOV Virtual Functions
> > that can potentially be created (total VFs supported by the device).
> > This can be leveraged when the device is exposing lower than optimal BAR
> > size as a default, allowing access to the entire resource when lower
> > number of VFs are created.
> > It is achieved by dynamically resizing the BAR to largest possible value
> > that allows to fit all newly created VFs within the original resource
> > boundary.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > ---
> >  drivers/pci/iov.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/pci/pci.h   |  1 +
> >  include/linux/pci.h |  3 ++
> >  3 files changed, 95 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index e8ccd2ae0f024..d88efbfa70e42 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
> >  	return cmd & PCI_SRIOV_CTRL_MSE;
> >  }
> >  
> > +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs)
> > +{
> > +	resource_size_t size;
> > +	int ret, old, i;
> > +	u32 sizes;
> > +
> > +	pci_config_pm_runtime_get(dev);
> > +
> > +	if (pci_iov_memory_decoding_enabled(dev)) {
> > +		ret = -EBUSY;
> > +		goto err;
> > +	}
> > +
> > +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> > +	if (!sizes) {
> > +		ret = -ENOTSUPP;
> > +		goto err;
> > +	}
> > +
> > +	old = pci_rebar_get_current_size(dev, resno);
> > +	if (old < 0) {
> > +		ret = old;
> > +		goto err;
> > +	}
> > +
> > +	while (sizes > 0) {
> > +		i = __fls(sizes);
> > +		size = pci_rebar_size_to_bytes(i);
> > +		if (size * num_vfs <= pci_resource_len(dev, resno)) {
> > +			if (i != old) {
> > +				ret = pci_rebar_set_size(dev, resno, size);
> > +				if (ret)
> > +					goto err;
> > +
> > +				pci_iov_resource_set_size(dev, resno, size);
> > +				pci_iov_update_resource(dev, resno);
> > +			}
> > +			break;
> > +		}
> > +		sizes &= ~BIT(i);
> > +	}
> > +
> > +	pci_config_pm_runtime_put(dev);
> > +
> > +	return;
> > +
> > +err:
> > +	dev_WARN(&dev->dev, "Failed to extend %s: %d\n",
> > +		 pci_resource_name(dev, resno), ret);
> 
> Why do you use dev_WARN()? (analoguous to WARN_ON() / friends).
> 
> I suppose you'd want to use pci_warn() instead.

This should never happen - but fair point, with something like
panic_on_warn it would have more negative consequences. Let's downgrade
it to pci_warn().

> 
> > +	pci_config_pm_runtime_put(dev);
> > +}
> > +
> > +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno)
> > +{
> > +	if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES])
> > +		pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs);
> > +}
> > +
> > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> > +{
> > +	if (!pci_resource_is_iov(dev, resno)) {
> > +		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
> > +			 pci_resource_name(dev, resno));
> 
> pci_warn() ?

And this one is programmer error. But same as above, let's use
pci_warn().

Thanks
-Michał

[PATCH v2 3/3] drm/xe/pf: Extend the VF LMEM BAR

[Michał Winiarski]

Opt into extending the VF BAR.
LMEM is partitioned between multiple VFs, and we expect that the more
VFs we have, the less LMEM is assigned to each VF.
This means that we can achieve full LMEM BAR access without the need to
attempt full VF LMEM BAR resize via pci_resize_resource().

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/xe/regs/xe_bars.h | 1 +
 drivers/gpu/drm/xe/xe_sriov_pf.c  | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/xe/regs/xe_bars.h b/drivers/gpu/drm/xe/regs/xe_bars.h
index ce05b6ae832f1..880140d6ccdca 100644
--- a/drivers/gpu/drm/xe/regs/xe_bars.h
+++ b/drivers/gpu/drm/xe/regs/xe_bars.h
@@ -7,5 +7,6 @@
 
 #define GTTMMADR_BAR			0 /* MMIO + GTT */
 #define LMEM_BAR			2 /* VRAM */
+#define VF_LMEM_BAR			9 /* VF VRAM */
 
 #endif
diff --git a/drivers/gpu/drm/xe/xe_sriov_pf.c b/drivers/gpu/drm/xe/xe_sriov_pf.c
index 0f721ae17b266..a26719b87ac1e 100644
--- a/drivers/gpu/drm/xe/xe_sriov_pf.c
+++ b/drivers/gpu/drm/xe/xe_sriov_pf.c
@@ -4,7 +4,9 @@
  */
 
 #include <drm/drm_managed.h>
+#include <linux/pci.h>
 
+#include "regs/xe_bars.h"
 #include "xe_assert.h"
 #include "xe_device.h"
 #include "xe_module.h"
@@ -80,8 +82,14 @@ bool xe_sriov_pf_readiness(struct xe_device *xe)
  */
 int xe_sriov_pf_init_early(struct xe_device *xe)
 {
+	int err;
+
 	xe_assert(xe, IS_SRIOV_PF(xe));
 
+	err = pci_iov_resource_extend(to_pci_dev(xe->drm.dev), VF_LMEM_BAR, true);
+	if (err)
+		xe_sriov_info(xe, "Failed to extend VF LMEM BAR: %d", err);
+
 	return drmm_mutex_init(&xe->drm, &xe->sriov.pf.master_lock);
 }
 
-- 
2.46.0