[PATCH v4 0/7] PCI: VF resizable BAR

[PATCH v4 0/7] PCI: VF resizable BAR

[Michał Winiarski]

Hi,

In v4, the approach to extending the BAR was changed to follow the
suggestion from Christian.
The control is now entirely on the driver side, with PCI subsystem
checking whether the resource fits during VF enabling.
I also added helpers to move between IOV / BAR # (suggested by Ilpo),
since the ifdefs started to look mighty ugly after addressing some of
the review feedback.

v3 can be found here:
https://lore.kernel.org/dri-devel/20241010103203.382898-1-michal.winiarski@intel.com/

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 for drivers to change the VF BAR size
without resizing the entire underlying reservation (within the original
resource boundary).

Thanks,
-Michał

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)

v2 -> v3:
- Extract introducing pci_resource_is_iov to separate commit and
  use it elsewhere in PCI subsystem (Christian)
- Extract restoring VF rebar state to separate commit (Christian)
- Reorganize memory decoding check (Christian)
- Don't use dev_WARN (Ilpo)
- Fix build without CONFIG_PCI_IOV (CI)

v3 -> v4:
- Change the approach to extending the BAR (Christian)
- Tidy the commit messages, use 80 line limit where necessary (Bjorn)
- Add kerneldocs to exported functions (Bjorn)
- Add pci_resource_to_iov() / pci_resource_from_iov() helpers (Ilpo)
- Use FIELD_GET(), tidy whitespace (Ilpo)

Michał Winiarski (7):
  PCI/IOV: Restore VF resizable BAR state after reset
  PCI: Add a helper to identify IOV resources
  PCI: Add a helper to convert between standard and IOV resources
  PCI: Allow IOV resources to be resized in pci_resize_resource()
  PCI/IOV: Check that VF BAR fits within the reservation
  PCI: Allow drivers to control VF BAR size
  drm/xe/pf: Set VF LMEM BAR size

 drivers/gpu/drm/xe/regs/xe_bars.h |   1 +
 drivers/gpu/drm/xe/xe_pci_sriov.c |  22 +++++
 drivers/pci/iov.c                 | 158 +++++++++++++++++++++++++++---
 drivers/pci/pci.c                 |  10 +-
 drivers/pci/pci.h                 |  45 ++++++++-
 drivers/pci/setup-bus.c           |   9 +-
 drivers/pci/setup-res.c           |  40 ++++++--
 include/linux/pci.h               |   6 ++
 include/uapi/linux/pci_regs.h     |   1 +
 9 files changed, 261 insertions(+), 31 deletions(-)

-- 
2.47.0

[PATCH v4 1/7] PCI/IOV: Restore VF resizable BAR state after reset

[Michał Winiarski]

Similar to regular resizable BAR, VF BAR can also be resized, e.g. by
the system firmware or the PCI subsystem itself.

Add the capability ID and restore it as a part of IOV state.

See PCIe r4.0, sec 9.3.7.4.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/iov.c             | 29 ++++++++++++++++++++++++++++-
 include/uapi/linux/pci_regs.h |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index aaa33e8dc4c97..6bdc9950b9787 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -7,6 +7,7 @@
  * Copyright (C) 2009 Intel Corporation, Yu Zhao <yu.zhao@intel.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/export.h>
@@ -862,6 +863,30 @@ static void sriov_release(struct pci_dev *dev)
 	dev->sriov = NULL;
 }
 
+static void sriov_restore_vf_rebar_state(struct pci_dev *dev)
+{
+	unsigned int pos, nbars, i;
+	u32 ctrl;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VF_REBAR);
+	if (!pos)
+		return;
+
+	pci_read_config_dword(dev, pos + PCI_REBAR_CTRL, &ctrl);
+	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
+
+	for (i = 0; i < nbars; i++, pos += 8) {
+		int bar_idx, size;
+
+		pci_read_config_dword(dev, pos + PCI_REBAR_CTRL, &ctrl);
+		bar_idx = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, ctrl);
+		size = pci_rebar_bytes_to_size(dev->sriov->barsz[bar_idx]);
+		ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
+		ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
+		pci_write_config_dword(dev, pos + PCI_REBAR_CTRL, ctrl);
+	}
+}
+
 static void sriov_restore_state(struct pci_dev *dev)
 {
 	int i;
@@ -1021,8 +1046,10 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
  */
 void pci_restore_iov_state(struct pci_dev *dev)
 {
-	if (dev->is_physfn)
+	if (dev->is_physfn) {
+		sriov_restore_vf_rebar_state(dev);
 		sriov_restore_state(dev);
+	}
 }
 
 /**
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 12323b3334a9c..a0cf701c4c3af 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -740,6 +740,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_NPEM	0x29	/* Native PCIe Enclosure Management */
-- 
2.47.0

[PATCH v4 2/7] PCI: Add a helper to identify IOV resources

[Michał Winiarski]

There are multiple places where special handling is required for IOV
resources.

Extract it to pci_resource_is_iov() helper and drop a few ifdefs.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.h       | 19 +++++++++++++++----
 drivers/pci/setup-bus.c |  7 +++----
 drivers/pci/setup-res.c |  4 +---
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 14d00ce45bfa9..48d345607e57e 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -580,6 +580,10 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno);
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
+static inline bool pci_resource_is_iov(int resno)
+{
+	return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
+}
 extern const struct attribute_group sriov_pf_dev_attr_group;
 extern const struct attribute_group sriov_vf_dev_attr_group;
 #else
@@ -589,12 +593,21 @@ static inline int pci_iov_init(struct pci_dev *dev)
 }
 static inline void pci_iov_release(struct pci_dev *dev) { }
 static inline void pci_iov_remove(struct pci_dev *dev) { }
+static inline void pci_iov_update_resource(struct pci_dev *dev, int resno) { }
+static inline resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev,
+							   int resno)
+{
+	return 0;
+}
 static inline void pci_restore_iov_state(struct pci_dev *dev) { }
 static inline int pci_iov_bus_range(struct pci_bus *bus)
 {
 	return 0;
 }
-
+static inline bool pci_resource_is_iov(int resno)
+{
+	return false;
+}
 #endif /* CONFIG_PCI_IOV */
 
 #ifdef CONFIG_PCIE_PTM
@@ -616,12 +629,10 @@ unsigned long pci_cardbus_resource_alignment(struct resource *);
 static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
 						     struct resource *res)
 {
-#ifdef CONFIG_PCI_IOV
 	int resno = res - dev->resource;
 
-	if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
+	if (pci_resource_is_iov(resno))
 		return pci_sriov_resource_alignment(dev, resno);
-#endif
 	if (dev->class >> 8 == PCI_CLASS_BRIDGE_CARDBUS)
 		return pci_cardbus_resource_alignment(res);
 	return resource_alignment(res);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 23082bc0ca37a..ba293df10c050 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1093,17 +1093,16 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			     (r->flags & mask) != type3))
 				continue;
 			r_size = resource_size(r);
-#ifdef CONFIG_PCI_IOV
+
 			/* Put SRIOV requested res to the optional list */
-			if (realloc_head && i >= PCI_IOV_RESOURCES &&
-					i <= PCI_IOV_RESOURCE_END) {
+			if (realloc_head && pci_resource_is_iov(i)) {
 				add_align = max(pci_resource_alignment(dev, r), add_align);
 				r->end = r->start - 1;
 				add_to_list(realloc_head, dev, r, r_size, 0 /* Don't care */);
 				children_add_size += r_size;
 				continue;
 			}
-#endif
+
 			/*
 			 * aligns[0] is for 1MB (since bridge memory
 			 * windows are always at least 1MB aligned), so
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index c6d933ddfd464..e2cf79253ebda 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -127,10 +127,8 @@ void pci_update_resource(struct pci_dev *dev, int resno)
 {
 	if (resno <= PCI_ROM_RESOURCE)
 		pci_std_update_resource(dev, resno);
-#ifdef CONFIG_PCI_IOV
-	else if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
+	else if (pci_resource_is_iov(resno))
 		pci_iov_update_resource(dev, resno);
-#endif
 }
 
 int pci_claim_resource(struct pci_dev *dev, int resource)
-- 
2.47.0

[PATCH v4 3/7] PCI: Add a helper to convert between standard and IOV resources

[Michał Winiarski]

There are multiple places where conversions between IOV resources and
standard resources are done.

Extract the logic to pci_resource_to_iov() and pci_resource_from_iov()
helpers.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/pci/iov.c       | 20 ++++++++++----------
 drivers/pci/pci.h       | 18 ++++++++++++++++++
 drivers/pci/setup-bus.c |  2 +-
 3 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 6bdc9950b9787..eedc1df56c49e 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -151,7 +151,7 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
 	if (!dev->is_physfn)
 		return 0;
 
-	return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
+	return dev->sriov->barsz[pci_resource_from_iov(resno)];
 }
 
 static void pci_read_vf_config_common(struct pci_dev *virtfn)
@@ -322,12 +322,12 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
 	virtfn->multifunction = 0;
 
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		res = &dev->resource[i + PCI_IOV_RESOURCES];
+		res = &dev->resource[pci_resource_to_iov(i)];
 		if (!res->parent)
 			continue;
 		virtfn->resource[i].name = pci_name(virtfn);
 		virtfn->resource[i].flags = res->flags;
-		size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
+		size = pci_iov_resource_size(dev, pci_resource_to_iov(i));
 		virtfn->resource[i].start = res->start + size * id;
 		virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
 		rc = request_resource(res, &virtfn->resource[i]);
@@ -624,8 +624,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 
 	nres = 0;
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		bars |= (1 << (i + PCI_IOV_RESOURCES));
-		res = &dev->resource[i + PCI_IOV_RESOURCES];
+		bars |= (1 << pci_resource_to_iov(i));
+		res = &dev->resource[pci_resource_to_iov(i)];
 		if (res->parent)
 			nres++;
 	}
@@ -786,8 +786,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
 
 	nres = 0;
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		res = &dev->resource[i + PCI_IOV_RESOURCES];
-		res_name = pci_resource_name(dev, i + PCI_IOV_RESOURCES);
+		res = &dev->resource[pci_resource_to_iov(i)];
+		res_name = pci_resource_name(dev, pci_resource_to_iov(i));
 
 		/*
 		 * If it is already FIXED, don't change it, something
@@ -844,7 +844,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
 	dev->is_physfn = 0;
 failed:
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		res = &dev->resource[i + PCI_IOV_RESOURCES];
+		res = &dev->resource[pci_resource_to_iov(i)];
 		res->flags = 0;
 	}
 
@@ -906,7 +906,7 @@ static void sriov_restore_state(struct pci_dev *dev)
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, ctrl);
 
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
-		pci_update_resource(dev, i + PCI_IOV_RESOURCES);
+		pci_update_resource(dev, pci_resource_to_iov(i));
 
 	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
 	pci_iov_set_numvfs(dev, iov->num_VFs);
@@ -972,7 +972,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
 {
 	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
 	struct resource *res = dev->resource + resno;
-	int vf_bar = resno - PCI_IOV_RESOURCES;
+	int vf_bar = pci_resource_from_iov(resno);
 	struct pci_bus_region region;
 	u16 cmd;
 	u32 new;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 48d345607e57e..1f8d88f0243b7 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -584,6 +584,15 @@ static inline bool pci_resource_is_iov(int resno)
 {
 	return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
 }
+static inline int pci_resource_to_iov(int resno)
+{
+	return resno + PCI_IOV_RESOURCES;
+}
+
+static inline int pci_resource_from_iov(int resno)
+{
+	return resno - PCI_IOV_RESOURCES;
+}
 extern const struct attribute_group sriov_pf_dev_attr_group;
 extern const struct attribute_group sriov_vf_dev_attr_group;
 #else
@@ -608,6 +617,15 @@ static inline bool pci_resource_is_iov(int resno)
 {
 	return false;
 }
+static inline int pci_resource_to_iov(int resno)
+{
+	return -ENODEV;
+}
+
+static inline int pci_resource_from_iov(int resno)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_PCI_IOV */
 
 #ifdef CONFIG_PCIE_PTM
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ba293df10c050..c5ad7c4ad6eb1 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1778,7 +1778,7 @@ static int iov_resources_unassigned(struct pci_dev *dev, void *data)
 	bool *unassigned = data;
 
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
-		struct resource *r = &dev->resource[i + PCI_IOV_RESOURCES];
+		struct resource *r = &dev->resource[pci_resource_to_iov(i)];
 		struct pci_bus_region region;
 
 		/* Not assigned or rejected by kernel? */
-- 
2.47.0

[PATCH v4 4/7] PCI: Allow IOV resources to be resized in pci_resize_resource()

[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.

Extend the pci_resize_resource() function to accept IOV resources.

See PCIe r4.0, sec 9.3.7.4.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/pci/iov.c       | 21 +++++++++++++++++++++
 drivers/pci/pci.c       | 10 +++++++++-
 drivers/pci/pci.h       | 10 +++++++++-
 drivers/pci/setup-res.c | 36 +++++++++++++++++++++++++++++++-----
 4 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index eedc1df56c49e..79143c1bc7bb4 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -154,6 +154,27 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
 	return dev->sriov->barsz[pci_resource_from_iov(resno)];
 }
 
+void pci_iov_resource_set_size(struct pci_dev *dev, int resno,
+			       resource_size_t size)
+{
+	if (!pci_resource_is_iov(resno)) {
+		pci_warn(dev, "%s is not an IOV resource\n",
+			 pci_resource_name(dev, resno));
+		return;
+	}
+
+	dev->sriov->barsz[pci_resource_from_iov(resno)] = size;
+}
+
+bool pci_iov_is_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 7d85c04fbba2a..905a533807b32 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3720,8 +3720,16 @@ static int pci_rebar_find_pos(struct pci_dev *pdev, int bar)
 {
 	unsigned int pos, nbars, i;
 	u32 ctrl;
+	int cap;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
+	if (pci_resource_is_iov(bar)) {
+		cap = PCI_EXT_CAP_ID_VF_REBAR;
+		bar = pci_resource_from_iov(bar);
+	} else {
+		cap = PCI_EXT_CAP_ID_REBAR;
+	}
+
+	pos = pci_find_ext_capability(pdev, cap);
 	if (!pos)
 		return -ENOTSUPP;
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 1f8d88f0243b7..4347858007145 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -580,6 +580,9 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno);
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
+void pci_iov_resource_set_size(struct pci_dev *dev, int resno,
+			       resource_size_t size);
+bool pci_iov_is_memory_decoding_enabled(struct pci_dev *dev);
 static inline bool pci_resource_is_iov(int resno)
 {
 	return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
@@ -613,6 +616,12 @@ static inline int pci_iov_bus_range(struct pci_bus *bus)
 {
 	return 0;
 }
+static inline void pci_iov_resource_set_size(struct pci_dev *dev, int resno,
+					     resource_size_t size) { }
+static inline bool pci_iov_is_memory_decoding_enabled(struct pci_dev *dev)
+{
+	return false;
+}
 static inline bool pci_resource_is_iov(int resno)
 {
 	return false;
@@ -621,7 +630,6 @@ static inline int pci_resource_to_iov(int resno)
 {
 	return -ENODEV;
 }
-
 static inline int pci_resource_from_iov(int resno)
 {
 	return -ENODEV;
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index e2cf79253ebda..cec8f684b7c70 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -425,13 +425,40 @@ void pci_release_resource(struct pci_dev *dev, int resno)
 }
 EXPORT_SYMBOL(pci_release_resource);
 
+static bool pci_resize_is_memory_decoding_enabled(struct pci_dev *dev,
+						  int resno)
+{
+	u16 cmd;
+
+	if (pci_resource_is_iov(resno))
+		return pci_iov_is_memory_decoding_enabled(dev);
+
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+
+	return cmd & PCI_COMMAND_MEMORY;
+}
+
+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(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);
@@ -442,8 +469,7 @@ 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)
+	if (pci_resize_is_memory_decoding_enabled(dev, resno))
 		return -EBUSY;
 
 	sizes = pci_rebar_get_possible_sizes(dev, resno);
@@ -461,7 +487,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) {
@@ -473,7 +499,7 @@ 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);
-- 
2.47.0

[PATCH v4 5/7] PCI/IOV: Check that VF BAR fits within the reservation

[Michał Winiarski]

VF MMIO resource reservation, either created by system firmware and
inherited by Linux PCI subsystem or created by the subsystem itself,
should contain enough space to fit the BAR of all SR-IOV Virtual
Functions that can potentially be created (total VFs supported by the
device).

However, that assumption only holds in an environment where VF BAR size
can't be modified.

Add an additional check that verifies that VF BAR for all enabled VFs
fits within the underlying reservation resource.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/pci/iov.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 79143c1bc7bb4..5de828e5a26ea 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -645,10 +645,14 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 
 	nres = 0;
 	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
+		int vf_bar_sz = pci_iov_resource_size(dev,
+						      pci_resource_to_iov(i));
 		bars |= (1 << pci_resource_to_iov(i));
 		res = &dev->resource[pci_resource_to_iov(i)];
-		if (res->parent)
-			nres++;
+		if (!res->parent || vf_bar_sz * nr_virtfn > resource_size(res))
+			continue;
+
+		nres++;
 	}
 	if (nres != iov->nres) {
 		pci_err(dev, "not enough MMIO resources for SR-IOV\n");
-- 
2.47.0

[PATCH v4 6/7] PCI: Allow drivers to control VF BAR size

[Michał Winiarski]

Drivers could leverage the fact that the VF BAR MMIO reservation is
created for total number of VFs supported by the device by resizing the
BAR to larger size when smaller number of VFs is enabled.

Add a pci_iov_vf_bar_set_size() function to control the size and a
pci_iov_vf_bar_get_sizes() helper to get the VF BAR sizes that will
allow up to num_vfs to be successfully enabled with the current
underlying reservation size.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/pci/iov.c   | 80 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  6 ++++
 2 files changed, 86 insertions(+)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 5de828e5a26ea..de8d473459440 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -1281,3 +1281,83 @@ int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
 	return nr_virtfn;
 }
 EXPORT_SYMBOL_GPL(pci_sriov_configure_simple);
+
+/**
+ * pci_iov_vf_bar_set_size - set a new size for a VF BAR
+ * @dev: the PCI device
+ * @resno: the resource number
+ * @size: new size as defined in the spec (0=1MB, 19=512GB)
+ *
+ * Set the new size of a VF BAR that supports VF resizable BAR capability.
+ * Unlike pci_resize_resource(), this does not cause the resource that
+ * reserves the MMIO space (originally up to total_VFs) to be resized, which
+ * means that following calls to pci_enable_sriov() can fail if the resources
+ * no longer fit.
+ *
+ * Returns 0 on success, or negative on failure.
+ *
+ */
+int pci_iov_vf_bar_set_size(struct pci_dev *dev, int resno, int size)
+{
+	int ret;
+	u32 sizes;
+
+	if (!pci_resource_is_iov(resno))
+		return -EINVAL;
+
+	if (pci_iov_is_memory_decoding_enabled(dev))
+		return -EBUSY;
+
+	sizes = pci_rebar_get_possible_sizes(dev, resno);
+	if (!sizes)
+		return -ENOTSUPP;
+
+	if (!(sizes & BIT(size)))
+		return -EINVAL;
+
+	ret = pci_rebar_set_size(dev, resno, size);
+	if (ret)
+		return ret;
+
+	pci_iov_resource_set_size(dev, resno, pci_rebar_size_to_bytes(size));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_iov_vf_bar_set_size);
+
+/**
+ * pci_iov_vf_bar_get_sizes - get VF BAR sizes that allow to create up to num_vfs
+ * @dev: the PCI device
+ * @resno: the resource number
+ * @num_vfs: number of VFs
+ *
+ * Get the sizes of a VF resizable BAR that can fit up to num_vfs within the
+ * resource that reserves the MMIO space (originally up to total_VFs) the as
+ * bitmask defined in the spec (bit 0=1MB, bit 19=512GB).
+ *
+ * Returns 0 if BAR isn't resizable.
+ *
+ */
+u32 pci_iov_vf_bar_get_sizes(struct pci_dev *dev, int resno, int num_vfs)
+{
+	resource_size_t size;
+	u32 sizes;
+	int i;
+
+	sizes = pci_rebar_get_possible_sizes(dev, resno);
+	if (!sizes)
+		return 0;
+
+	while (sizes > 0) {
+		i = __fls(sizes);
+		size = pci_rebar_size_to_bytes(i);
+
+		if (size * num_vfs <= pci_resource_len(dev, resno))
+			break;
+
+		sizes &= ~BIT(i);
+	}
+
+	return sizes;
+}
+EXPORT_SYMBOL_GPL(pci_iov_vf_bar_get_sizes);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 573b4c4c2be61..1b9e7e3cab0ce 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2371,6 +2371,8 @@ 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_vf_bar_set_size(struct pci_dev *dev, int resno, int size);
+u32 pci_iov_vf_bar_get_sizes(struct pci_dev *dev, int resno, int num_vfs);
 void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
 
 /* Arch may override these (weak) */
@@ -2423,6 +2425,10 @@ 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 int pci_iov_vf_bar_set_size(struct pci_dev *dev, int resno, int size)
+{ return -ENODEV; }
+static inline u32 pci_iov_vf_bar_get_sizes(struct pci_dev *dev, int resno, int num_vfs)
+{ return 0; }
 static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
 #endif
 
-- 
2.47.0

[PATCH v4 7/7] drm/xe/pf: Set VF LMEM BAR size

[Michał Winiarski]

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().

Always set the largest possible BAR size that allows to fit the number
of enabled VFs.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/xe/regs/xe_bars.h |  1 +
 drivers/gpu/drm/xe/xe_pci_sriov.c | 22 ++++++++++++++++++++++
 2 files changed, 23 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_pci_sriov.c b/drivers/gpu/drm/xe/xe_pci_sriov.c
index aaceee748287e..57cdeb41ef1d9 100644
--- a/drivers/gpu/drm/xe/xe_pci_sriov.c
+++ b/drivers/gpu/drm/xe/xe_pci_sriov.c
@@ -3,6 +3,10 @@
  * Copyright © 2023-2024 Intel Corporation
  */
 
+#include <linux/bitops.h>
+#include <linux/pci.h>
+
+#include "regs/xe_bars.h"
 #include "xe_assert.h"
 #include "xe_device.h"
 #include "xe_gt_sriov_pf_config.h"
@@ -62,6 +66,18 @@ static void pf_reset_vfs(struct xe_device *xe, unsigned int num_vfs)
 			xe_gt_sriov_pf_control_trigger_flr(gt, n);
 }
 
+static int resize_vf_vram_bar(struct xe_device *xe, int num_vfs)
+{
+	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
+	u32 sizes;
+
+	sizes = pci_iov_vf_bar_get_sizes(pdev, VF_LMEM_BAR, num_vfs);
+	if (!sizes)
+		return 0;
+
+	return pci_iov_vf_bar_set_size(pdev, VF_LMEM_BAR, __fls(sizes));
+}
+
 static int pf_enable_vfs(struct xe_device *xe, int num_vfs)
 {
 	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
@@ -88,6 +104,12 @@ static int pf_enable_vfs(struct xe_device *xe, int num_vfs)
 	if (err < 0)
 		goto failed;
 
+	if (IS_DGFX(xe)) {
+		err = resize_vf_vram_bar(xe, num_vfs);
+		if (err)
+			xe_sriov_info(xe, "Failed to set VF LMEM BAR size: %d\n", err);
+	}
+
 	err = pci_enable_sriov(pdev, num_vfs);
 	if (err < 0)
 		goto failed;
-- 
2.47.0

Re: [PATCH v4 5/7] PCI/IOV: Check that VF BAR fits within the reservation

[Michał Winiarski]

On Fri, Oct 25, 2024 at 11:50:36PM +0200, Michał Winiarski wrote:
> VF MMIO resource reservation, either created by system firmware and
> inherited by Linux PCI subsystem or created by the subsystem itself,
> should contain enough space to fit the BAR of all SR-IOV Virtual
> Functions that can potentially be created (total VFs supported by the
> device).
> 
> However, that assumption only holds in an environment where VF BAR size
> can't be modified.
> 
> Add an additional check that verifies that VF BAR for all enabled VFs
> fits within the underlying reservation resource.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/pci/iov.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 79143c1bc7bb4..5de828e5a26ea 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -645,10 +645,14 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  
>  	nres = 0;
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> +		int vf_bar_sz = pci_iov_resource_size(dev,
> +						      pci_resource_to_iov(i));

And that should be resource_size_t.

Fortunately, CI caught it:
https://lore.kernel.org/intel-xe/172990062085.1334319.8298861567163770193@2413ebb6fbb6/

I'll address that in the next rev.

-Michał

>  		bars |= (1 << pci_resource_to_iov(i));
>  		res = &dev->resource[pci_resource_to_iov(i)];
> -		if (res->parent)
> -			nres++;
> +		if (!res->parent || vf_bar_sz * nr_virtfn > resource_size(res))
> +			continue;
> +
> +		nres++;
>  	}
>  	if (nres != iov->nres) {
>  		pci_err(dev, "not enough MMIO resources for SR-IOV\n");
> -- 
> 2.47.0
> 

Re: [PATCH v4 6/7] PCI: Allow drivers to control VF BAR size

[Bjorn Helgaas]

On Fri, Oct 25, 2024 at 11:50:37PM +0200, Michał Winiarski wrote:
> Drivers could leverage the fact that the VF BAR MMIO reservation is
> created for total number of VFs supported by the device by resizing the
> BAR to larger size when smaller number of VFs is enabled.
> 
> Add a pci_iov_vf_bar_set_size() function to control the size and a
> pci_iov_vf_bar_get_sizes() helper to get the VF BAR sizes that will
> allow up to num_vfs to be successfully enabled with the current
> underlying reservation size.
> ...

> + * pci_iov_vf_bar_get_sizes - get VF BAR sizes that allow to create up to num_vfs
> + * @dev: the PCI device
> + * @resno: the resource number
> + * @num_vfs: number of VFs
> + *
> + * Get the sizes of a VF resizable BAR that can fit up to num_vfs within the
> + * resource that reserves the MMIO space (originally up to total_VFs) the as
> + * bitmask defined in the spec (bit 0=1MB, bit 19=512GB).

This sentence doesn't quite parse; something is missing around "the as".

I'm guessing you mean to say something about the return value being a
bitmask of VF BAR sizes that can be accommodated if num_vfs are
enabled?  If so, maybe combine it with the following paragraph:

> + * Returns 0 if BAR isn't resizable.
> + *
> + */
> +u32 pci_iov_vf_bar_get_sizes(struct pci_dev *dev, int resno, int num_vfs)
> +{
> +	resource_size_t size;
> +	u32 sizes;
> +	int i;
> +
> +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> +	if (!sizes)
> +		return 0;
> +
> +	while (sizes > 0) {
> +		i = __fls(sizes);
> +		size = pci_rebar_size_to_bytes(i);
> +
> +		if (size * num_vfs <= pci_resource_len(dev, resno))
> +			break;
> +
> +		sizes &= ~BIT(i);
> +	}
> +
> +	return sizes;
> +}
> +EXPORT_SYMBOL_GPL(pci_iov_vf_bar_get_sizes);

Re: [PATCH v4 5/7] PCI/IOV: Check that VF BAR fits within the reservation

[Bjorn Helgaas]

On Fri, Oct 25, 2024 at 11:50:36PM +0200, Michał Winiarski wrote:
> VF MMIO resource reservation, either created by system firmware and
> inherited by Linux PCI subsystem or created by the subsystem itself,
> should contain enough space to fit the BAR of all SR-IOV Virtual
> Functions that can potentially be created (total VFs supported by the
> device).

I don't think "VF resource reservation ... should contain enough
space" is really accurate or actionable.  It would be *nice* if the PF
BAR is large enough to accommodate the largest supported VF BARs for
all possible VFs, but if it doesn't, it's not really an error.  It's
just a reflection of the fact that resource space is limited.

> However, that assumption only holds in an environment where VF BAR size
> can't be modified.

There's no reason to assume anything about how many VF BARs fit.  The
existing code should avoid enabling the requested nr_virtfn VFs if the
PF doesn't have enough space -- I think that's what the "if
(res->parent)" is supposed to be checking.

The fact that you need a change here makes me suspect that we're
missing some resource claim (and corresponding res->parent update)
elsewhere when resizing the VF BAR.

> Add an additional check that verifies that VF BAR for all enabled VFs
> fits within the underlying reservation resource.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/pci/iov.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 79143c1bc7bb4..5de828e5a26ea 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -645,10 +645,14 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  
>  	nres = 0;
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> +		int vf_bar_sz = pci_iov_resource_size(dev,
> +						      pci_resource_to_iov(i));
>  		bars |= (1 << pci_resource_to_iov(i));
>  		res = &dev->resource[pci_resource_to_iov(i)];
> -		if (res->parent)
> -			nres++;
> +		if (!res->parent || vf_bar_sz * nr_virtfn > resource_size(res))
> +			continue;
> +
> +		nres++;
>  	}
>  	if (nres != iov->nres) {
>  		pci_err(dev, "not enough MMIO resources for SR-IOV\n");
> -- 
> 2.47.0
> 

Re: [PATCH v4 1/7] PCI/IOV: Restore VF resizable BAR state after reset

[Christian König]

Am 25.10.24 um 23:50 schrieb Michał Winiarski:
> Similar to regular resizable BAR, VF BAR can also be resized, e.g. by
> the system firmware or the PCI subsystem itself.
>
> Add the capability ID and restore it as a part of IOV state.
>
> See PCIe r4.0, sec 9.3.7.4.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/pci/iov.c             | 29 ++++++++++++++++++++++++++++-
>   include/uapi/linux/pci_regs.h |  1 +
>   2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index aaa33e8dc4c97..6bdc9950b9787 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -7,6 +7,7 @@
>    * Copyright (C) 2009 Intel Corporation, Yu Zhao <yu.zhao@intel.com>
>    */
>   
> +#include <linux/bitfield.h>
>   #include <linux/pci.h>
>   #include <linux/slab.h>
>   #include <linux/export.h>
> @@ -862,6 +863,30 @@ static void sriov_release(struct pci_dev *dev)
>   	dev->sriov = NULL;
>   }
>   
> +static void sriov_restore_vf_rebar_state(struct pci_dev *dev)
> +{
> +	unsigned int pos, nbars, i;
> +	u32 ctrl;
> +
> +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VF_REBAR);
> +	if (!pos)
> +		return;
> +
> +	pci_read_config_dword(dev, pos + PCI_REBAR_CTRL, &ctrl);
> +	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
> +
> +	for (i = 0; i < nbars; i++, pos += 8) {
> +		int bar_idx, size;
> +
> +		pci_read_config_dword(dev, pos + PCI_REBAR_CTRL, &ctrl);
> +		bar_idx = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, ctrl);
> +		size = pci_rebar_bytes_to_size(dev->sriov->barsz[bar_idx]);
> +		ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> +		ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
> +		pci_write_config_dword(dev, pos + PCI_REBAR_CTRL, ctrl);
> +	}
> +}
> +
>   static void sriov_restore_state(struct pci_dev *dev)
>   {
>   	int i;
> @@ -1021,8 +1046,10 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
>    */
>   void pci_restore_iov_state(struct pci_dev *dev)
>   {
> -	if (dev->is_physfn)
> +	if (dev->is_physfn) {
> +		sriov_restore_vf_rebar_state(dev);
>   		sriov_restore_state(dev);
> +	}
>   }
>   
>   /**
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 12323b3334a9c..a0cf701c4c3af 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -740,6 +740,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_NPEM	0x29	/* Native PCIe Enclosure Management */

Re: [PATCH v4 2/7] PCI: Add a helper to identify IOV resources

[Christian König]

Am 25.10.24 um 23:50 schrieb Michał Winiarski:
> There are multiple places where special handling is required for IOV
> resources.
>
> Extract it to pci_resource_is_iov() helper and drop a few ifdefs.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/pci/pci.h       | 19 +++++++++++++++----
>   drivers/pci/setup-bus.c |  7 +++----
>   drivers/pci/setup-res.c |  4 +---
>   3 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 14d00ce45bfa9..48d345607e57e 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -580,6 +580,10 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno);
>   resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
>   void pci_restore_iov_state(struct pci_dev *dev);
>   int pci_iov_bus_range(struct pci_bus *bus);
> +static inline bool pci_resource_is_iov(int resno)
> +{
> +	return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
> +}
>   extern const struct attribute_group sriov_pf_dev_attr_group;
>   extern const struct attribute_group sriov_vf_dev_attr_group;
>   #else
> @@ -589,12 +593,21 @@ static inline int pci_iov_init(struct pci_dev *dev)
>   }
>   static inline void pci_iov_release(struct pci_dev *dev) { }
>   static inline void pci_iov_remove(struct pci_dev *dev) { }
> +static inline void pci_iov_update_resource(struct pci_dev *dev, int resno) { }
> +static inline resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev,
> +							   int resno)
> +{
> +	return 0;
> +}
>   static inline void pci_restore_iov_state(struct pci_dev *dev) { }
>   static inline int pci_iov_bus_range(struct pci_bus *bus)
>   {
>   	return 0;
>   }
> -
> +static inline bool pci_resource_is_iov(int resno)
> +{
> +	return false;
> +}
>   #endif /* CONFIG_PCI_IOV */
>   
>   #ifdef CONFIG_PCIE_PTM
> @@ -616,12 +629,10 @@ unsigned long pci_cardbus_resource_alignment(struct resource *);
>   static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
>   						     struct resource *res)
>   {
> -#ifdef CONFIG_PCI_IOV
>   	int resno = res - dev->resource;
>   
> -	if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
> +	if (pci_resource_is_iov(resno))
>   		return pci_sriov_resource_alignment(dev, resno);
> -#endif
>   	if (dev->class >> 8 == PCI_CLASS_BRIDGE_CARDBUS)
>   		return pci_cardbus_resource_alignment(res);
>   	return resource_alignment(res);
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 23082bc0ca37a..ba293df10c050 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1093,17 +1093,16 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>   			     (r->flags & mask) != type3))
>   				continue;
>   			r_size = resource_size(r);
> -#ifdef CONFIG_PCI_IOV
> +
>   			/* Put SRIOV requested res to the optional list */
> -			if (realloc_head && i >= PCI_IOV_RESOURCES &&
> -					i <= PCI_IOV_RESOURCE_END) {
> +			if (realloc_head && pci_resource_is_iov(i)) {
>   				add_align = max(pci_resource_alignment(dev, r), add_align);
>   				r->end = r->start - 1;
>   				add_to_list(realloc_head, dev, r, r_size, 0 /* Don't care */);
>   				children_add_size += r_size;
>   				continue;
>   			}
> -#endif
> +
>   			/*
>   			 * aligns[0] is for 1MB (since bridge memory
>   			 * windows are always at least 1MB aligned), so
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index c6d933ddfd464..e2cf79253ebda 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -127,10 +127,8 @@ void pci_update_resource(struct pci_dev *dev, int resno)
>   {
>   	if (resno <= PCI_ROM_RESOURCE)
>   		pci_std_update_resource(dev, resno);
> -#ifdef CONFIG_PCI_IOV
> -	else if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
> +	else if (pci_resource_is_iov(resno))
>   		pci_iov_update_resource(dev, resno);
> -#endif
>   }
>   
>   int pci_claim_resource(struct pci_dev *dev, int resource)

Re: [PATCH v4 3/7] PCI: Add a helper to convert between standard and IOV resources

[Christian König]

Am 25.10.24 um 23:50 schrieb Michał Winiarski:
> There are multiple places where conversions between IOV resources and
> standard resources are done.
>
> Extract the logic to pci_resource_to_iov() and pci_resource_from_iov()
> helpers.
>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

I don't work enough with that code to fully judge if that is useful or not.

But feel free to add Acked-by: Christian König 
<christian.koenig@amd.com> since style etc.. looks good to me.

Regards,
Christian.

> ---
>   drivers/pci/iov.c       | 20 ++++++++++----------
>   drivers/pci/pci.h       | 18 ++++++++++++++++++
>   drivers/pci/setup-bus.c |  2 +-
>   3 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 6bdc9950b9787..eedc1df56c49e 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -151,7 +151,7 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
>   	if (!dev->is_physfn)
>   		return 0;
>   
> -	return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
> +	return dev->sriov->barsz[pci_resource_from_iov(resno)];
>   }
>   
>   static void pci_read_vf_config_common(struct pci_dev *virtfn)
> @@ -322,12 +322,12 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
>   	virtfn->multifunction = 0;
>   
>   	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> +		res = &dev->resource[pci_resource_to_iov(i)];
>   		if (!res->parent)
>   			continue;
>   		virtfn->resource[i].name = pci_name(virtfn);
>   		virtfn->resource[i].flags = res->flags;
> -		size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> +		size = pci_iov_resource_size(dev, pci_resource_to_iov(i));
>   		virtfn->resource[i].start = res->start + size * id;
>   		virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
>   		rc = request_resource(res, &virtfn->resource[i]);
> @@ -624,8 +624,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>   
>   	nres = 0;
>   	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		bars |= (1 << (i + PCI_IOV_RESOURCES));
> -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> +		bars |= (1 << pci_resource_to_iov(i));
> +		res = &dev->resource[pci_resource_to_iov(i)];
>   		if (res->parent)
>   			nres++;
>   	}
> @@ -786,8 +786,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
>   
>   	nres = 0;
>   	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> -		res_name = pci_resource_name(dev, i + PCI_IOV_RESOURCES);
> +		res = &dev->resource[pci_resource_to_iov(i)];
> +		res_name = pci_resource_name(dev, pci_resource_to_iov(i));
>   
>   		/*
>   		 * If it is already FIXED, don't change it, something
> @@ -844,7 +844,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
>   	dev->is_physfn = 0;
>   failed:
>   	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> +		res = &dev->resource[pci_resource_to_iov(i)];
>   		res->flags = 0;
>   	}
>   
> @@ -906,7 +906,7 @@ static void sriov_restore_state(struct pci_dev *dev)
>   	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, ctrl);
>   
>   	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> -		pci_update_resource(dev, i + PCI_IOV_RESOURCES);
> +		pci_update_resource(dev, pci_resource_to_iov(i));
>   
>   	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
>   	pci_iov_set_numvfs(dev, iov->num_VFs);
> @@ -972,7 +972,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
>   {
>   	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
>   	struct resource *res = dev->resource + resno;
> -	int vf_bar = resno - PCI_IOV_RESOURCES;
> +	int vf_bar = pci_resource_from_iov(resno);
>   	struct pci_bus_region region;
>   	u16 cmd;
>   	u32 new;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 48d345607e57e..1f8d88f0243b7 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -584,6 +584,15 @@ static inline bool pci_resource_is_iov(int resno)
>   {
>   	return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
>   }
> +static inline int pci_resource_to_iov(int resno)
> +{
> +	return resno + PCI_IOV_RESOURCES;
> +}
> +
> +static inline int pci_resource_from_iov(int resno)
> +{
> +	return resno - PCI_IOV_RESOURCES;
> +}
>   extern const struct attribute_group sriov_pf_dev_attr_group;
>   extern const struct attribute_group sriov_vf_dev_attr_group;
>   #else
> @@ -608,6 +617,15 @@ static inline bool pci_resource_is_iov(int resno)
>   {
>   	return false;
>   }
> +static inline int pci_resource_to_iov(int resno)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int pci_resource_from_iov(int resno)
> +{
> +	return -ENODEV;
> +}
>   #endif /* CONFIG_PCI_IOV */
>   
>   #ifdef CONFIG_PCIE_PTM
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index ba293df10c050..c5ad7c4ad6eb1 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1778,7 +1778,7 @@ static int iov_resources_unassigned(struct pci_dev *dev, void *data)
>   	bool *unassigned = data;
>   
>   	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		struct resource *r = &dev->resource[i + PCI_IOV_RESOURCES];
> +		struct resource *r = &dev->resource[pci_resource_to_iov(i)];
>   		struct pci_bus_region region;
>   
>   		/* Not assigned or rejected by kernel? */

Re: [PATCH v4 5/7] PCI/IOV: Check that VF BAR fits within the reservation

[Michał Winiarski]

On Mon, Oct 28, 2024 at 11:56:04AM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 25, 2024 at 11:50:36PM +0200, Michał Winiarski wrote:
> > VF MMIO resource reservation, either created by system firmware and
> > inherited by Linux PCI subsystem or created by the subsystem itself,
> > should contain enough space to fit the BAR of all SR-IOV Virtual
> > Functions that can potentially be created (total VFs supported by the
> > device).
> 
> I don't think "VF resource reservation ... should contain enough
> space" is really accurate or actionable.  It would be *nice* if the PF
> BAR is large enough to accommodate the largest supported VF BARs for
> all possible VFs, but if it doesn't, it's not really an error.  It's
> just a reflection of the fact that resource space is limited.

From PCI perspective, you're right, IOV resources are optional, and it's
not really an error for PF device itself.
From IOV perspective - we do need those resources to be able to create
VFs.

All I'm trying to say here, is that the context of the change is the
"success" case, where the VF BAR reservation was successfully assigned,
and the PF will be able to create VFs.
The case where there were not enough resources for VF BAR (and PF won't
be able to create VFs) remains unchanged.

> > However, that assumption only holds in an environment where VF BAR size
> > can't be modified.
> 
> There's no reason to assume anything about how many VF BARs fit.  The
> existing code should avoid enabling the requested nr_virtfn VFs if the
> PF doesn't have enough space -- I think that's what the "if
> (res->parent)" is supposed to be checking.
> 
> The fact that you need a change here makes me suspect that we're
> missing some resource claim (and corresponding res->parent update)
> elsewhere when resizing the VF BAR.

My understanding is that res->parent is only expressing that the
resource is assigned.
We don't really want to change that, the resource is still there and is
assigned - we just want to make sure that VF enabling fails if the
caller wants to enable more VFs than possible for current resource size.

Let's use an example. A device with a single BAR.
initial_vf_bar_size = X
total_vfs = 4
supported_vf_resizable_bar_sizes = X, 2X, 4X

With that - the initial underlying resource looks like this:
            +----------------------+
            |+--------------------+|
            ||                    ||
            |+--------------------+|
            |+--------------------+|
            ||                    ||
            |+--------------------+|
            |+--------------------+|
            ||                    ||
            |+--------------------+|
            |+--------------------+|
            ||                    ||
            |+--------------------+|
            +----------------------+
Its size is 4X, and it contains BAR for 4 VFs.
"resource_size >= vf_bar_size * num_vfs" is true for any num_vfs
Let's assume that there are enough resources to assign it.

Patch 4/7 allows to resize the entire resource (in addition to changing
the VF BAR size), which means that after calling:
pci_resize_resource() with size = 2X, the underlying resource will look
like this:
            +----------------------+ 
            |+--------------------+| 
            ||                    || 
            ||                    || 
            ||                    || 
            ||                    || 
            |+--------------------+| 
            |+--------------------+| 
            ||                    || 
            ||                    || 
            ||                    || 
            ||                    || 
            |+--------------------+| 
            |+--------------------+| 
            ||                    || 
            ||                    || 
            ||                    || 
            ||                    || 
            |+--------------------+| 
            |+--------------------+| 
            ||                    || 
            ||                    || 
            ||                    || 
            ||                    || 
            |+--------------------+| 
            +----------------------+ 
Its size is 8X, and it contains BAR for 4 VFs.
"resource_size >= vf_bar_size * num_vfs" is true for any num_vfs
It does require an extra 4X of MMIO resources, so this can fail in
resource constrained environment, even though the original 4X resource
was able to be assigned.

The following patch 6/7 allows to change VF BAR size without touching
the underlying reservation size.
After calling pci_iov_vf_bar_set_size() to 4X and enabling a single VF,
the underlying resource will look like this:
            +----------------------+ 
            |+--------------------+| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            ||░░░░░░░░░░░░░░░░░░░░|| 
            |+--------------------+| 
            +----------------------+ 
Its size is 4X, but since pci_iov_vf_bar_set_size() was called, it is no
longer able to accomodate 4 VFs.
"resource_size >= vf_bar_size * num_vfs" is only true for num_vfs = 1
and any attempts to create more than 1 VF should fail.
We don't need to worry about being MMIO resource constrained, no extra
MMIO resources are needed.

-Michał

> > Add an additional check that verifies that VF BAR for all enabled VFs
> > fits within the underlying reservation resource.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > ---
> >  drivers/pci/iov.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index 79143c1bc7bb4..5de828e5a26ea 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -645,10 +645,14 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> >  
> >  	nres = 0;
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > +		int vf_bar_sz = pci_iov_resource_size(dev,
> > +						      pci_resource_to_iov(i));
> >  		bars |= (1 << pci_resource_to_iov(i));
> >  		res = &dev->resource[pci_resource_to_iov(i)];
> > -		if (res->parent)
> > -			nres++;
> > +		if (!res->parent || vf_bar_sz * nr_virtfn > resource_size(res))
> > +			continue;
> > +
> > +		nres++;
> >  	}
> >  	if (nres != iov->nres) {
> >  		pci_err(dev, "not enough MMIO resources for SR-IOV\n");
> > -- 
> > 2.47.0
> > 

Re: [PATCH v4 5/7] PCI/IOV: Check that VF BAR fits within the reservation

[Bjorn Helgaas]

On Wed, Oct 30, 2024 at 12:43:19PM +0100, Michał Winiarski wrote:
> On Mon, Oct 28, 2024 at 11:56:04AM -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 25, 2024 at 11:50:36PM +0200, Michał Winiarski wrote:
> > > VF MMIO resource reservation, either created by system firmware and
> > > inherited by Linux PCI subsystem or created by the subsystem itself,
> > > should contain enough space to fit the BAR of all SR-IOV Virtual
> > > Functions that can potentially be created (total VFs supported by the
> > > device).
> > 
> > I don't think "VF resource reservation ... should contain enough
> > space" is really accurate or actionable.  It would be *nice* if the PF
> > BAR is large enough to accommodate the largest supported VF BARs for
> > all possible VFs, but if it doesn't, it's not really an error.  It's
> > just a reflection of the fact that resource space is limited.
> 
> From PCI perspective, you're right, IOV resources are optional, and it's
> not really an error for PF device itself.
> From IOV perspective - we do need those resources to be able to create
> VFs.
> 
> All I'm trying to say here, is that the context of the change is the
> "success" case, where the VF BAR reservation was successfully assigned,
> and the PF will be able to create VFs.
> The case where there were not enough resources for VF BAR (and PF won't
> be able to create VFs) remains unchanged.
> 
> > > However, that assumption only holds in an environment where VF BAR size
> > > can't be modified.
> > 
> > There's no reason to assume anything about how many VF BARs fit.  The
> > existing code should avoid enabling the requested nr_virtfn VFs if the
> > PF doesn't have enough space -- I think that's what the "if
> > (res->parent)" is supposed to be checking.
> > 
> > The fact that you need a change here makes me suspect that we're
> > missing some resource claim (and corresponding res->parent update)
> > elsewhere when resizing the VF BAR.
> 
> My understanding is that res->parent is only expressing that the
> resource is assigned.
> We don't really want to change that, the resource is still there and is
> assigned - we just want to make sure that VF enabling fails if the
> caller wants to enable more VFs than possible for current resource size.
> 
> Let's use an example. A device with a single BAR.
> initial_vf_bar_size = X
> total_vfs = 4
> supported_vf_resizable_bar_sizes = X, 2X, 4X

In addition, IIUC we're assuming the PF BAR size is 4X, since the
conclusion is that 4 VF BARs of size X fill it completely.

> With that - the initial underlying resource looks like this:
>             +----------------------+
>             |+--------------------+|
>             ||                    ||
>             |+--------------------+|
>             |+--------------------+|
>             ||                    ||
>             |+--------------------+|
>             |+--------------------+|
>             ||                    ||
>             |+--------------------+|
>             |+--------------------+|
>             ||                    ||
>             |+--------------------+|
>             +----------------------+
> Its size is 4X, and it contains BAR for 4 VFs.
> "resource_size >= vf_bar_size * num_vfs" is true for any num_vfs
> Let's assume that there are enough resources to assign it.
> 
> Patch 4/7 allows to resize the entire resource (in addition to changing
> the VF BAR size), which means that after calling:
> pci_resize_resource() with size = 2X, the underlying resource will look
> like this:
>             +----------------------+ 
>             |+--------------------+| 
>             ||                    || 
>             ||                    || 
>             ||                    || 
>             ||                    || 
>             |+--------------------+| 
>             |+--------------------+| 
>             ||                    || 
>             ||                    || 
>             ||                    || 
>             ||                    || 
>             |+--------------------+| 
>             |+--------------------+| 
>             ||                    || 
>             ||                    || 
>             ||                    || 
>             ||                    || 
>             |+--------------------+| 
>             |+--------------------+| 
>             ||                    || 
>             ||                    || 
>             ||                    || 
>             ||                    || 
>             |+--------------------+| 
>             +----------------------+ 
> Its size is 8X, and it contains BAR for 4 VFs.
> "resource_size >= vf_bar_size * num_vfs" is true for any num_vfs

With the assumption that the PF BAR size is 4X, these VFs would no
longer fit.  I guess that's basically what you say here:

> It does require an extra 4X of MMIO resources, so this can fail in
> resource constrained environment, even though the original 4X resource
> was able to be assigned.
> 
> The following patch 6/7 allows to change VF BAR size without touching
> the underlying reservation size.
> After calling pci_iov_vf_bar_set_size() to 4X and enabling a single VF,
> the underlying resource will look like this:
>             +----------------------+ 
>             |+--------------------+| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             ||░░░░░░░░░░░░░░░░░░░░|| 
>             |+--------------------+| 
>             +----------------------+ 
> Its size is 4X, but since pci_iov_vf_bar_set_size() was called, it is no
> longer able to accomodate 4 VFs.
> "resource_size >= vf_bar_size * num_vfs" is only true for num_vfs = 1
> and any attempts to create more than 1 VF should fail.
> We don't need to worry about being MMIO resource constrained, no extra
> MMIO resources are needed.

IIUC this series only resizes VF BARs.  Those VF BARs are carved out
of a PF BAR, and this series doesn't touch the PF BAR resizing path.
I guess the driver might be able to increase the PF BAR size if
necessary, and then increase the VF BAR size.

It sounds like this patch is really a bug fix independent of VF BAR
resizing.  If we currently allow enabling more VFs than will fit in a
PF BAR, that sounds like a bug.

So if we try to enable too many VFs, sriov_enable() should fail.  I
still don't see why this check should change the res->parent test,
though.

> > > Add an additional check that verifies that VF BAR for all enabled VFs
> > > fits within the underlying reservation resource.
> > > 
> > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > > ---
> > >  drivers/pci/iov.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > index 79143c1bc7bb4..5de828e5a26ea 100644
> > > --- a/drivers/pci/iov.c
> > > +++ b/drivers/pci/iov.c
> > > @@ -645,10 +645,14 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> > >  
> > >  	nres = 0;
> > >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > > +		int vf_bar_sz = pci_iov_resource_size(dev,
> > > +						      pci_resource_to_iov(i));
> > >  		bars |= (1 << pci_resource_to_iov(i));
> > >  		res = &dev->resource[pci_resource_to_iov(i)];
> > > -		if (res->parent)
> > > -			nres++;
> > > +		if (!res->parent || vf_bar_sz * nr_virtfn > resource_size(res))
> > > +			continue;
> > > +
> > > +		nres++;
> > >  	}
> > >  	if (nres != iov->nres) {
> > >  		pci_err(dev, "not enough MMIO resources for SR-IOV\n");
> > > -- 
> > > 2.47.0
> > > 

Re: [PATCH v4 3/7] PCI: Add a helper to convert between standard and IOV resources

[Ilpo Järvinen]

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

On Fri, 25 Oct 2024, Michał Winiarski wrote:

> There are multiple places where conversions between IOV resources and
> standard resources are done.
> 
> Extract the logic to pci_resource_to_iov() and pci_resource_from_iov()
> helpers.
> 
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/pci/iov.c       | 20 ++++++++++----------
>  drivers/pci/pci.h       | 18 ++++++++++++++++++
>  drivers/pci/setup-bus.c |  2 +-
>  3 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 6bdc9950b9787..eedc1df56c49e 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -151,7 +151,7 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
>  	if (!dev->is_physfn)
>  		return 0;
>  
> -	return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
> +	return dev->sriov->barsz[pci_resource_from_iov(resno)];
>  }
>  
>  static void pci_read_vf_config_common(struct pci_dev *virtfn)
> @@ -322,12 +322,12 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
>  	virtfn->multifunction = 0;
>  
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> +		res = &dev->resource[pci_resource_to_iov(i)];
>  		if (!res->parent)
>  			continue;
>  		virtfn->resource[i].name = pci_name(virtfn);
>  		virtfn->resource[i].flags = res->flags;
> -		size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> +		size = pci_iov_resource_size(dev, pci_resource_to_iov(i));
>  		virtfn->resource[i].start = res->start + size * id;
>  		virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
>  		rc = request_resource(res, &virtfn->resource[i]);
> @@ -624,8 +624,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  
>  	nres = 0;
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		bars |= (1 << (i + PCI_IOV_RESOURCES));
> -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> +		bars |= (1 << pci_resource_to_iov(i));
> +		res = &dev->resource[pci_resource_to_iov(i)];
>  		if (res->parent)
>  			nres++;
>  	}
> @@ -786,8 +786,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
>  
>  	nres = 0;
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> -		res_name = pci_resource_name(dev, i + PCI_IOV_RESOURCES);
> +		res = &dev->resource[pci_resource_to_iov(i)];
> +		res_name = pci_resource_name(dev, pci_resource_to_iov(i));
>  
>  		/*
>  		 * If it is already FIXED, don't change it, something
> @@ -844,7 +844,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
>  	dev->is_physfn = 0;
>  failed:
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> +		res = &dev->resource[pci_resource_to_iov(i)];
>  		res->flags = 0;
>  	}
>  
> @@ -906,7 +906,7 @@ static void sriov_restore_state(struct pci_dev *dev)
>  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, ctrl);
>  
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> -		pci_update_resource(dev, i + PCI_IOV_RESOURCES);
> +		pci_update_resource(dev, pci_resource_to_iov(i));
>  
>  	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
>  	pci_iov_set_numvfs(dev, iov->num_VFs);
> @@ -972,7 +972,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
>  {
>  	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
>  	struct resource *res = dev->resource + resno;
> -	int vf_bar = resno - PCI_IOV_RESOURCES;
> +	int vf_bar = pci_resource_from_iov(resno);
>  	struct pci_bus_region region;
>  	u16 cmd;
>  	u32 new;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 48d345607e57e..1f8d88f0243b7 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -584,6 +584,15 @@ static inline bool pci_resource_is_iov(int resno)
>  {
>  	return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
>  }
> +static inline int pci_resource_to_iov(int resno)
> +{
> +	return resno + PCI_IOV_RESOURCES;
> +}
> +
> +static inline int pci_resource_from_iov(int resno)
> +{
> +	return resno - PCI_IOV_RESOURCES;
> +}

to/from feels wrong way around for me. What is named as "PCI resource from 
IOV" converts from PCI resource indexing to IOV compatible indexing, and 
vice versa.

>  extern const struct attribute_group sriov_pf_dev_attr_group;
>  extern const struct attribute_group sriov_vf_dev_attr_group;
>  #else
> @@ -608,6 +617,15 @@ static inline bool pci_resource_is_iov(int resno)
>  {
>  	return false;
>  }
> +static inline int pci_resource_to_iov(int resno)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int pci_resource_from_iov(int resno)
> +{
> +	return -ENODEV;
> +}

These seem dangerous as the errors are not checked by the callers. Perhaps 
put something like BUG_ON(1) there instead as it really is something that 
should never be called for real if CONFIG_PCI_IOV is not enabled, they are 
just to make compiler happy without #ifdefs in C code.

-- 
 i.

>  #endif /* CONFIG_PCI_IOV */
>  
>  #ifdef CONFIG_PCIE_PTM
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index ba293df10c050..c5ad7c4ad6eb1 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1778,7 +1778,7 @@ static int iov_resources_unassigned(struct pci_dev *dev, void *data)
>  	bool *unassigned = data;
>  
>  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> -		struct resource *r = &dev->resource[i + PCI_IOV_RESOURCES];
> +		struct resource *r = &dev->resource[pci_resource_to_iov(i)];
>  		struct pci_bus_region region;
>  
>  		/* Not assigned or rejected by kernel? */
> 

Re: [PATCH v4 5/7] PCI/IOV: Check that VF BAR fits within the reservation

[Michał Winiarski]

On Wed, Oct 30, 2024 at 11:55:01AM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 30, 2024 at 12:43:19PM +0100, Michał Winiarski wrote:
> > On Mon, Oct 28, 2024 at 11:56:04AM -0500, Bjorn Helgaas wrote:
> > > On Fri, Oct 25, 2024 at 11:50:36PM +0200, Michał Winiarski wrote:
> > > > VF MMIO resource reservation, either created by system firmware and
> > > > inherited by Linux PCI subsystem or created by the subsystem itself,
> > > > should contain enough space to fit the BAR of all SR-IOV Virtual
> > > > Functions that can potentially be created (total VFs supported by the
> > > > device).
> > > 
> > > I don't think "VF resource reservation ... should contain enough
> > > space" is really accurate or actionable.  It would be *nice* if the PF
> > > BAR is large enough to accommodate the largest supported VF BARs for
> > > all possible VFs, but if it doesn't, it's not really an error.  It's
> > > just a reflection of the fact that resource space is limited.
> > 
> > From PCI perspective, you're right, IOV resources are optional, and it's
> > not really an error for PF device itself.
> > From IOV perspective - we do need those resources to be able to create
> > VFs.
> > 
> > All I'm trying to say here, is that the context of the change is the
> > "success" case, where the VF BAR reservation was successfully assigned,
> > and the PF will be able to create VFs.
> > The case where there were not enough resources for VF BAR (and PF won't
> > be able to create VFs) remains unchanged.
> > 
> > > > However, that assumption only holds in an environment where VF BAR size
> > > > can't be modified.
> > > 
> > > There's no reason to assume anything about how many VF BARs fit.  The
> > > existing code should avoid enabling the requested nr_virtfn VFs if the
> > > PF doesn't have enough space -- I think that's what the "if
> > > (res->parent)" is supposed to be checking.
> > > 
> > > The fact that you need a change here makes me suspect that we're
> > > missing some resource claim (and corresponding res->parent update)
> > > elsewhere when resizing the VF BAR.
> > 
> > My understanding is that res->parent is only expressing that the
> > resource is assigned.
> > We don't really want to change that, the resource is still there and is
> > assigned - we just want to make sure that VF enabling fails if the
> > caller wants to enable more VFs than possible for current resource size.
> > 
> > Let's use an example. A device with a single BAR.
> > initial_vf_bar_size = X
> > total_vfs = 4
> > supported_vf_resizable_bar_sizes = X, 2X, 4X
> 
> In addition, IIUC we're assuming the PF BAR size is 4X, since the
> conclusion is that 4 VF BARs of size X fill it completely.

The PF BAR is initially sized based on VF BAR size [1]. So yeah - that's
the assumption for the initial state (prior to doing any resizing).
For VF PCI device BAR 0, it would be PF PCI device resource 7 (and it's
programmed using PCI SR-IOV extended capability - so slightly different
path then regular PCI BARs).
Note that this resource is completely independent from actual PF BAR 0
(resource 0), which is why I'm calling it "underlying resource" or
"reservation".

[1] https://elixir.bootlin.com/linux/v6.11/source/drivers/pci/iov.c#L807


> > With that - the initial underlying resource looks like this:
> >             +----------------------+
> >             |+--------------------+|
> >             ||                    ||
> >             |+--------------------+|
> >             |+--------------------+|
> >             ||                    ||
> >             |+--------------------+|
> >             |+--------------------+|
> >             ||                    ||
> >             |+--------------------+|
> >             |+--------------------+|
> >             ||                    ||
> >             |+--------------------+|
> >             +----------------------+
> > Its size is 4X, and it contains BAR for 4 VFs.
> > "resource_size >= vf_bar_size * num_vfs" is true for any num_vfs
> > Let's assume that there are enough resources to assign it.
> > 
> > Patch 4/7 allows to resize the entire resource (in addition to changing
> > the VF BAR size), which means that after calling:
> > pci_resize_resource() with size = 2X, the underlying resource will look
> > like this:
> >             +----------------------+ 
> >             |+--------------------+| 
> >             ||                    || 
> >             ||                    || 
> >             ||                    || 
> >             ||                    || 
> >             |+--------------------+| 
> >             |+--------------------+| 
> >             ||                    || 
> >             ||                    || 
> >             ||                    || 
> >             ||                    || 
> >             |+--------------------+| 
> >             |+--------------------+| 
> >             ||                    || 
> >             ||                    || 
> >             ||                    || 
> >             ||                    || 
> >             |+--------------------+| 
> >             |+--------------------+| 
> >             ||                    || 
> >             ||                    || 
> >             ||                    || 
> >             ||                    || 
> >             |+--------------------+| 
> >             +----------------------+ 
> > Its size is 8X, and it contains BAR for 4 VFs.
> > "resource_size >= vf_bar_size * num_vfs" is true for any num_vfs
> 
> With the assumption that the PF BAR size is 4X, these VFs would no
> longer fit.  I guess that's basically what you say here:

Exactly - it wouldn't fit, unless we resize the underlying resource as
well.
Which is what the successfull call to pci_resize_resource() with size =
2X, will do.

> > It does require an extra 4X of MMIO resources, so this can fail in
> > resource constrained environment, even though the original 4X resource
> > was able to be assigned.
> > The following patch 6/7 allows to change VF BAR size without touching
> > the underlying reservation size.
> > After calling pci_iov_vf_bar_set_size() to 4X and enabling a single VF,
> > the underlying resource will look like this:
> >             +----------------------+ 
> >             |+--------------------+| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             ||░░░░░░░░░░░░░░░░░░░░|| 
> >             |+--------------------+| 
> >             +----------------------+ 
> > Its size is 4X, but since pci_iov_vf_bar_set_size() was called, it is no
> > longer able to accomodate 4 VFs.
> > "resource_size >= vf_bar_size * num_vfs" is only true for num_vfs = 1
> > and any attempts to create more than 1 VF should fail.
> > We don't need to worry about being MMIO resource constrained, no extra
> > MMIO resources are needed.
> 
> IIUC this series only resizes VF BARs.  Those VF BARs are carved out
> of a PF BAR, and this series doesn't touch the PF BAR resizing path.
> I guess the driver might be able to increase the PF BAR size if
> necessary, and then increase the VF BAR size.

No - it is possible to resize the PF BAR as well.

In addition to adding pci_iov_vf_bar_set_size() /
pci_iov_vf_bar_get_size(), the series expands the pci_resize_resource()
function, so that it can accept IOV resources (PF BARs 7+, or
"underlying resource" for VF BAR).
The usage of pci_resize_resource() for IOV resources is the same as for
the regular PCI BARs, with the caller expected to release all the
resource prior to resizing it (as the bridge windows may need to be
reprogrammed).

> It sounds like this patch is really a bug fix independent of VF BAR
> resizing.  If we currently allow enabling more VFs than will fit in a
> PF BAR, that sounds like a bug.

It's not really a bug fix. Without the ability to control VF BAR size
independently of PF BAR size (via pci_iov_vf_bar_set_size()
/ pci_iov_vf_bar_get_size()), PF BAR size is always tied to the VF BAR
size and the total (max) number of VFs supported by the device, so
there's no need to check if the "nr_virtfn" will fit - as "nr_virtfn" is
guaranteed to be smaller than the total number of VFs supported by the
device.

> So if we try to enable too many VFs, sriov_enable() should fail.  I
> still don't see why this check should change the res->parent test,
> though.

The logic for res->parent isn't really changed - we are still failing VF
enabling if the resource is not assigned.

Previously we were incrementing resource counter if the resource is
assigned:

if (res->parent)
	nres++;

Now, we're not incrementing the resource counter if the resource is not
assigned or if it is too small to fit all enabled VFs:

if (!res->parent || vf_bar_sz * nr_virtfn > resource_size(res))
	continue;

I can split it into two conditions, if you think it would be clearer:

if (vf_bar_sz * nr_virtfn > resource_size(res))
	continue;

if (res->parent)
	nres++;

-Michał

> 
> > > > Add an additional check that verifies that VF BAR for all enabled VFs
> > > > fits within the underlying reservation resource.
> > > > 
> > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > > > ---
> > > >  drivers/pci/iov.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > > index 79143c1bc7bb4..5de828e5a26ea 100644
> > > > --- a/drivers/pci/iov.c
> > > > +++ b/drivers/pci/iov.c
> > > > @@ -645,10 +645,14 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> > > >  
> > > >  	nres = 0;
> > > >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > > > +		int vf_bar_sz = pci_iov_resource_size(dev,
> > > > +						      pci_resource_to_iov(i));
> > > >  		bars |= (1 << pci_resource_to_iov(i));
> > > >  		res = &dev->resource[pci_resource_to_iov(i)];
> > > > -		if (res->parent)
> > > > -			nres++;
> > > > +		if (!res->parent || vf_bar_sz * nr_virtfn > resource_size(res))
> > > > +			continue;
> > > > +
> > > > +		nres++;
> > > >  	}
> > > >  	if (nres != iov->nres) {
> > > >  		pci_err(dev, "not enough MMIO resources for SR-IOV\n");
> > > > -- 
> > > > 2.47.0
> > > > 

Re: [PATCH v4 3/7] PCI: Add a helper to convert between standard and IOV resources

[Michał Winiarski]

On Wed, Nov 06, 2024 at 04:22:11PM +0200, Ilpo Järvinen wrote:
> On Fri, 25 Oct 2024, Michał Winiarski wrote:
> 
> > There are multiple places where conversions between IOV resources and
> > standard resources are done.
> > 
> > Extract the logic to pci_resource_to_iov() and pci_resource_from_iov()
> > helpers.
> > 
> > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > ---
> >  drivers/pci/iov.c       | 20 ++++++++++----------
> >  drivers/pci/pci.h       | 18 ++++++++++++++++++
> >  drivers/pci/setup-bus.c |  2 +-
> >  3 files changed, 29 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index 6bdc9950b9787..eedc1df56c49e 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -151,7 +151,7 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
> >  	if (!dev->is_physfn)
> >  		return 0;
> >  
> > -	return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
> > +	return dev->sriov->barsz[pci_resource_from_iov(resno)];
> >  }
> >  
> >  static void pci_read_vf_config_common(struct pci_dev *virtfn)
> > @@ -322,12 +322,12 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> >  	virtfn->multifunction = 0;
> >  
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> > +		res = &dev->resource[pci_resource_to_iov(i)];
> >  		if (!res->parent)
> >  			continue;
> >  		virtfn->resource[i].name = pci_name(virtfn);
> >  		virtfn->resource[i].flags = res->flags;
> > -		size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> > +		size = pci_iov_resource_size(dev, pci_resource_to_iov(i));
> >  		virtfn->resource[i].start = res->start + size * id;
> >  		virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
> >  		rc = request_resource(res, &virtfn->resource[i]);
> > @@ -624,8 +624,8 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
> >  
> >  	nres = 0;
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		bars |= (1 << (i + PCI_IOV_RESOURCES));
> > -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> > +		bars |= (1 << pci_resource_to_iov(i));
> > +		res = &dev->resource[pci_resource_to_iov(i)];
> >  		if (res->parent)
> >  			nres++;
> >  	}
> > @@ -786,8 +786,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
> >  
> >  	nres = 0;
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> > -		res_name = pci_resource_name(dev, i + PCI_IOV_RESOURCES);
> > +		res = &dev->resource[pci_resource_to_iov(i)];
> > +		res_name = pci_resource_name(dev, pci_resource_to_iov(i));
> >  
> >  		/*
> >  		 * If it is already FIXED, don't change it, something
> > @@ -844,7 +844,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> >  	dev->is_physfn = 0;
> >  failed:
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		res = &dev->resource[i + PCI_IOV_RESOURCES];
> > +		res = &dev->resource[pci_resource_to_iov(i)];
> >  		res->flags = 0;
> >  	}
> >  
> > @@ -906,7 +906,7 @@ static void sriov_restore_state(struct pci_dev *dev)
> >  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, ctrl);
> >  
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> > -		pci_update_resource(dev, i + PCI_IOV_RESOURCES);
> > +		pci_update_resource(dev, pci_resource_to_iov(i));
> >  
> >  	pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
> >  	pci_iov_set_numvfs(dev, iov->num_VFs);
> > @@ -972,7 +972,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
> >  {
> >  	struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
> >  	struct resource *res = dev->resource + resno;
> > -	int vf_bar = resno - PCI_IOV_RESOURCES;
> > +	int vf_bar = pci_resource_from_iov(resno);
> >  	struct pci_bus_region region;
> >  	u16 cmd;
> >  	u32 new;
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 48d345607e57e..1f8d88f0243b7 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -584,6 +584,15 @@ static inline bool pci_resource_is_iov(int resno)
> >  {
> >  	return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
> >  }
> > +static inline int pci_resource_to_iov(int resno)
> > +{
> > +	return resno + PCI_IOV_RESOURCES;
> > +}
> > +
> > +static inline int pci_resource_from_iov(int resno)
> > +{
> > +	return resno - PCI_IOV_RESOURCES;
> > +}
> 
> to/from feels wrong way around for me. What is named as "PCI resource from 
> IOV" converts from PCI resource indexing to IOV compatible indexing, and 
> vice versa.

It converts "from" IOV BAR (resource 7+) to VF BAR (resource 0+).
I don't think swapping the to/from would make it clearer.

I had it as pci_iov_resource_to_vf_bar() / pci_vf_bar_to_iov_resource()
before, but went with a shorter naming instead.
Would that be clearer?
pci_iov_resource_to_vf_bar() / pci_iov_resource_from_vf_bar()?
Or perhaps you have a better suggestion?

> 
> >  extern const struct attribute_group sriov_pf_dev_attr_group;
> >  extern const struct attribute_group sriov_vf_dev_attr_group;
> >  #else
> > @@ -608,6 +617,15 @@ static inline bool pci_resource_is_iov(int resno)
> >  {
> >  	return false;
> >  }
> > +static inline int pci_resource_to_iov(int resno)
> > +{
> > +	return -ENODEV;
> > +}
> > +
> > +static inline int pci_resource_from_iov(int resno)
> > +{
> > +	return -ENODEV;
> > +}
> 
> These seem dangerous as the errors are not checked by the callers. Perhaps 
> put something like BUG_ON(1) there instead as it really is something that 
> should never be called for real if CONFIG_PCI_IOV is not enabled, they are 
> just to make compiler happy without #ifdefs in C code.

I'll add a WARN_ON_ONCE(1) to make it more visible.

-Michał

> 
> -- 
>  i.
> 
> >  #endif /* CONFIG_PCI_IOV */
> >  
> >  #ifdef CONFIG_PCIE_PTM
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index ba293df10c050..c5ad7c4ad6eb1 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1778,7 +1778,7 @@ static int iov_resources_unassigned(struct pci_dev *dev, void *data)
> >  	bool *unassigned = data;
> >  
> >  	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > -		struct resource *r = &dev->resource[i + PCI_IOV_RESOURCES];
> > +		struct resource *r = &dev->resource[pci_resource_to_iov(i)];
> >  		struct pci_bus_region region;
> >  
> >  		/* Not assigned or rejected by kernel? */
> > 

Re: [PATCH v4 6/7] PCI: Allow drivers to control VF BAR size

[Michał Winiarski]

On Mon, Oct 28, 2024 at 11:50:31AM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 25, 2024 at 11:50:37PM +0200, Michał Winiarski wrote:
> > Drivers could leverage the fact that the VF BAR MMIO reservation is
> > created for total number of VFs supported by the device by resizing the
> > BAR to larger size when smaller number of VFs is enabled.
> > 
> > Add a pci_iov_vf_bar_set_size() function to control the size and a
> > pci_iov_vf_bar_get_sizes() helper to get the VF BAR sizes that will
> > allow up to num_vfs to be successfully enabled with the current
> > underlying reservation size.
> > ...
> 
> > + * pci_iov_vf_bar_get_sizes - get VF BAR sizes that allow to create up to num_vfs
> > + * @dev: the PCI device
> > + * @resno: the resource number
> > + * @num_vfs: number of VFs
> > + *
> > + * Get the sizes of a VF resizable BAR that can fit up to num_vfs within the
> > + * resource that reserves the MMIO space (originally up to total_VFs) the as
> > + * bitmask defined in the spec (bit 0=1MB, bit 19=512GB).
> 
> This sentence doesn't quite parse; something is missing around "the as".

Yeah, typo, "the" should be removed.

> I'm guessing you mean to say something about the return value being a
> bitmask of VF BAR sizes that can be accommodated if num_vfs are
> enabled?  If so, maybe combine it with the following paragraph:

I'll change it to:

"Get the sizes of a VF resizable BAR that can be accomodated within the
resource that reserves the MMIO space if num_vfs are enabled.

Returns 0 if BAR isn't resizable, otherwise returns a bitmask in format
defined in the spec (bit 0=1MB, bit 19=512GB)."

-Michał

> 
> > + * Returns 0 if BAR isn't resizable.
> > + *
> > + */
> > +u32 pci_iov_vf_bar_get_sizes(struct pci_dev *dev, int resno, int num_vfs)
> > +{
> > +	resource_size_t size;
> > +	u32 sizes;
> > +	int i;
> > +
> > +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> > +	if (!sizes)
> > +		return 0;
> > +
> > +	while (sizes > 0) {
> > +		i = __fls(sizes);
> > +		size = pci_rebar_size_to_bytes(i);
> > +
> > +		if (size * num_vfs <= pci_resource_len(dev, resno))
> > +			break;
> > +
> > +		sizes &= ~BIT(i);
> > +	}
> > +
> > +	return sizes;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_iov_vf_bar_get_sizes);