[PATCH v2 0/8] PCI: Align small (<4k) BARs

[PATCH v2 0/8] PCI: Align small (<4k) BARs

[Stewart Hildebrand]

This series sets the default minimum resource alignment to 4k for memory
BARs. In preparation, it makes an optimization and addresses some corner
cases observed when reallocating BARs. I consider the prepapatory
patches to be prerequisites to changing the default BAR alignment.

I considered introducing checks for the specific scenarios described,
but chose not to pursue this. A check such as "if (xen_domain())" may be
pretty simple, but that doesn't account for other hypervisors. If other
hypervisors are to be considered, or if we try to dynamically reallocate
BARs for devices being marked for passthrough, such a check may quickly
grow unwieldy. Further, checking for the MSI-X tables residing in a
small (<4k) BAR is unlikely to be a one-liner. Making 4k alignment the
default seems more robust. Lastly, when using IORESOURCE_STARTALIGN, all
resources in the system need to be aligned.

I considered alternatively adding new functionality to the
pci=resource_alignment= option, but that approach was already attempted
and decided against [1].

[1] https://lore.kernel.org/linux-pci/1473757234-5284-4-git-send-email-xyjxie@linux.vnet.ibm.com/

v1->v2:
* rename ("PCI: don't clear already cleared bit") to
  ("PCI: Don't unnecessarily disable memory decoding")
* new patch: ("x86/PCI: Move some logic to new function")
* new patch: ("powerpc/pci: Preserve IORESOURCE_STARTALIGN alignment")

Stewart Hildebrand (8):
  x86/PCI: Move some logic to new function
  PCI: Don't unnecessarily disable memory decoding
  PCI: Restore resource alignment
  PCI: Restore memory decoding after reallocation
  x86/PCI: Preserve IORESOURCE_STARTALIGN alignment
  powerpc/pci: Preserve IORESOURCE_STARTALIGN alignment
  PCI: Don't reassign resources that are already aligned
  PCI: Align small (<4k) BARs

 arch/powerpc/kernel/pci-common.c |  6 +++--
 arch/x86/pci/i386.c              | 38 +++++++++++++++------------
 drivers/pci/pci.c                | 43 +++++++++++++++++++++++--------
 drivers/pci/setup-bus.c          | 44 ++++++++++++++++++++++++++++++++
 include/linux/pci.h              |  2 ++
 5 files changed, 103 insertions(+), 30 deletions(-)

-- 
2.45.2

[PATCH v2 1/8] x86/PCI: Move some logic to new function

[Stewart Hildebrand]

... to reduce indentation level. Take the opportunity to remove
redundant info from debug print string.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v2:
* new patch
---
 arch/x86/pci/i386.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index f2f4a5d50b27..3abd55902dbc 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -246,6 +246,25 @@ struct pci_check_idx_range {
 	int end;
 };
 
+static void alloc_resource(struct pci_dev *dev, int idx, int pass)
+{
+	struct resource *r = &dev->resource[idx];
+
+	dev_dbg(&dev->dev, "BAR %d: reserving %pr (p=%d)\n", idx, r, pass);
+
+	if (pci_claim_resource(dev, idx) < 0) {
+		if (r->flags & IORESOURCE_PCI_FIXED) {
+			dev_info(&dev->dev, "BAR %d %pR is immovable\n",
+				 idx, r);
+		} else {
+			/* We'll assign a new address later */
+			pcibios_save_fw_addr(dev, idx, r->start);
+			r->end -= r->start;
+			r->start = 0;
+		}
+	}
+}
+
 static void pcibios_allocate_dev_resources(struct pci_dev *dev, int pass)
 {
 	int idx, disabled, i;
@@ -271,23 +290,8 @@ static void pcibios_allocate_dev_resources(struct pci_dev *dev, int pass)
 				disabled = !(command & PCI_COMMAND_IO);
 			else
 				disabled = !(command & PCI_COMMAND_MEMORY);
-			if (pass == disabled) {
-				dev_dbg(&dev->dev,
-					"BAR %d: reserving %pr (d=%d, p=%d)\n",
-					idx, r, disabled, pass);
-				if (pci_claim_resource(dev, idx) < 0) {
-					if (r->flags & IORESOURCE_PCI_FIXED) {
-						dev_info(&dev->dev, "BAR %d %pR is immovable\n",
-							 idx, r);
-					} else {
-						/* We'll assign a new address later */
-						pcibios_save_fw_addr(dev,
-								idx, r->start);
-						r->end -= r->start;
-						r->start = 0;
-					}
-				}
-			}
+			if (pass == disabled)
+				alloc_resource(dev, idx, pass);
 		}
 	if (!pass) {
 		r = &dev->resource[PCI_ROM_RESOURCE];
-- 
2.45.2

[PATCH v2 2/8] PCI: Don't unnecessarily disable memory decoding

[Stewart Hildebrand]

If alignment is requested for a device and all the BARs are already
sufficiently aligned, there is no need to disable memory decoding for
the device. Add a check for this scenario to save a PCI config space
access.

Also, there is no need to disable memory decoding if already disabled,
since the bit in question (PCI_COMMAND_MEMORY) is a RW bit. Make the
write conditional to save a PCI config space access.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v2:
* new subject (was: "PCI: don't clear already cleared bit")
* don't disable memory decoding if alignment is not needed
---
 drivers/pci/pci.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 02b1d81b1419..53345dc596b5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6565,7 +6565,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 	return align;
 }
 
-static void pci_request_resource_alignment(struct pci_dev *dev, int bar,
+static bool pci_request_resource_alignment(struct pci_dev *dev, int bar,
 					   resource_size_t align, bool resize)
 {
 	struct resource *r = &dev->resource[bar];
@@ -6573,17 +6573,17 @@ static void pci_request_resource_alignment(struct pci_dev *dev, int bar,
 	resource_size_t size;
 
 	if (!(r->flags & IORESOURCE_MEM))
-		return;
+		return false;
 
 	if (r->flags & IORESOURCE_PCI_FIXED) {
 		pci_info(dev, "%s %pR: ignoring requested alignment %#llx\n",
 			 r_name, r, (unsigned long long)align);
-		return;
+		return false;
 	}
 
 	size = resource_size(r);
 	if (size >= align)
-		return;
+		return false;
 
 	/*
 	 * Increase the alignment of the resource.  There are two ways we
@@ -6626,6 +6626,8 @@ static void pci_request_resource_alignment(struct pci_dev *dev, int bar,
 		r->end = r->start + size - 1;
 	}
 	r->flags |= IORESOURCE_UNSET;
+
+	return true;
 }
 
 /*
@@ -6641,7 +6643,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	struct resource *r;
 	resource_size_t align;
 	u16 command;
-	bool resize = false;
+	bool resize = false, align_needed = false;
 
 	/*
 	 * VF BARs are read-only zero according to SR-IOV spec r1.1, sec
@@ -6663,12 +6665,21 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		return;
 	}
 
-	pci_read_config_word(dev, PCI_COMMAND, &command);
-	command &= ~PCI_COMMAND_MEMORY;
-	pci_write_config_word(dev, PCI_COMMAND, command);
+	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
+		bool ret;
 
-	for (i = 0; i <= PCI_ROM_RESOURCE; i++)
-		pci_request_resource_alignment(dev, i, align, resize);
+		ret = pci_request_resource_alignment(dev, i, align, resize);
+		align_needed = align_needed || ret;
+	}
+
+	if (!align_needed)
+		return;
+
+	pci_read_config_word(dev, PCI_COMMAND, &command);
+	if (command & PCI_COMMAND_MEMORY) {
+		command &= ~PCI_COMMAND_MEMORY;
+		pci_write_config_word(dev, PCI_COMMAND, command);
+	}
 
 	/*
 	 * Need to disable bridge's resource window,
-- 
2.45.2

[PATCH v2 3/8] PCI: Restore resource alignment

[Stewart Hildebrand]

Devices with alignment specified will lose their alignment in cases when
the bridge resources have been released, e.g. due to insufficient bridge
window size. Restore the alignment.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v2:
* capitalize subject text
---
 drivers/pci/setup-bus.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 23082bc0ca37..ab7510ce6917 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1594,6 +1594,23 @@ static void __pci_bridge_assign_resources(const struct pci_dev *bridge,
 	}
 }
 
+static void restore_child_resource_alignment(struct pci_bus *bus)
+{
+	struct pci_dev *dev;
+
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		struct pci_bus *b;
+
+		pci_reassigndev_resource_alignment(dev);
+
+		b = dev->subordinate;
+		if (!b)
+			continue;
+
+		restore_child_resource_alignment(b);
+	}
+}
+
 #define PCI_RES_TYPE_MASK \
 	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
 	 IORESOURCE_MEM_64)
@@ -1648,6 +1665,8 @@ static void pci_bridge_release_resources(struct pci_bus *bus,
 		r->start = 0;
 		r->flags = 0;
 
+		restore_child_resource_alignment(bus);
+
 		/* Avoiding touch the one without PREF */
 		if (type & IORESOURCE_PREFETCH)
 			type = IORESOURCE_PREFETCH;
-- 
2.45.2

[PATCH v2 4/8] PCI: Restore memory decoding after reallocation

[Stewart Hildebrand]

Currently, the PCI subsystem unconditionally clears the memory decoding
bit of devices with resource alignment specified. While drivers should
call pci_enable_device() to enable memory decoding, some platforms have
devices that expect memory decoding to be enabled even when no driver is
bound to the device. It is assumed firmware enables memory decoding in
these cases. For example, the vgacon driver uses the 0xb8000 buffer to
display a console without any PCI involvement, yet, on some platforms,
memory decoding must be enabled on the PCI VGA device in order for the
console to be displayed properly.

Restore the memory decoding bit after the resource has been reallocated.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v2:
* capitalize subject text
* reword commit message

Testing notes / how to check if your platform needs memory decoding
enabled in order to use vgacon:
1. Boot your system with a monitor attached, without any driver bound to
   your PCI VGA device. You should see a console on your display.
2. Find the SBDF of your VGA device with lspci -v (00:01.0 in this
   example).
3. Disable memory decoding (replace 00:01.0 with your SBDF):
  $ sudo setpci -v -s 00:01.0 0x04.W=0x05
4. Type something to see if it appears on the console display
5. Re-enable memory decoding:
  $ sudo setpci -v -s 00:01.0 0x04.W=0x07

Relevant prior discussion at [1]

[1] https://lore.kernel.org/linux-pci/20160906165652.GE1214@localhost/
---
 drivers/pci/pci.c       |  1 +
 drivers/pci/setup-bus.c | 25 +++++++++++++++++++++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 28 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 53345dc596b5..1ac1435ad91e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6677,6 +6677,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 
 	pci_read_config_word(dev, PCI_COMMAND, &command);
 	if (command & PCI_COMMAND_MEMORY) {
+		dev->dev_flags |= PCI_DEV_FLAGS_MEMORY_ENABLE;
 		command &= ~PCI_COMMAND_MEMORY;
 		pci_write_config_word(dev, PCI_COMMAND, command);
 	}
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ab7510ce6917..6847b251e19a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -2131,6 +2131,29 @@ pci_root_bus_distribute_available_resources(struct pci_bus *bus,
 	}
 }
 
+static void restore_memory_decoding(struct pci_bus *bus)
+{
+	struct pci_dev *dev;
+
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		struct pci_bus *b;
+
+		if (dev->dev_flags & PCI_DEV_FLAGS_MEMORY_ENABLE) {
+			u16 command;
+
+			pci_read_config_word(dev, PCI_COMMAND, &command);
+			command |= PCI_COMMAND_MEMORY;
+			pci_write_config_word(dev, PCI_COMMAND, command);
+		}
+
+		b = dev->subordinate;
+		if (!b)
+			continue;
+
+		restore_memory_decoding(b);
+	}
+}
+
 /*
  * First try will not touch PCI bridge res.
  * Second and later try will clear small leaf bridge res.
@@ -2229,6 +2252,8 @@ void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus)
 	goto again;
 
 dump:
+	restore_memory_decoding(bus);
+
 	/* Dump the resource on buses */
 	pci_bus_dump_resources(bus);
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e83ac93a4dcb..cb5df4c6a999 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -245,6 +245,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
 	/* Device does honor MSI masking despite saying otherwise */
 	PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
+	/* Firmware enabled memory decoding, to be restored if BAR is updated */
+	PCI_DEV_FLAGS_MEMORY_ENABLE = (__force pci_dev_flags_t) (1 << 13),
 };
 
 enum pci_irq_reroute_variant {
-- 
2.45.2

[PATCH v2 5/8] x86/PCI: Preserve IORESOURCE_STARTALIGN alignment

[Stewart Hildebrand]

There is a corner case in pcibios_allocate_dev_resources() where the
IORESOURCE_STARTALIGN alignment of memory BAR resources gets
overwritten. This does not affect bridge resources. The corner case is
not yet possible to trigger on x86, but it will be possible once the
default resource alignment changes, and memory BAR resources will start
to use IORESOURCE_STARTALIGN. Account for IORESOURCE_STARTALIGN in
preparation for changing the default resource alignment.

Skip the pcibios_save_fw_addr() call since the resource doesn't contain
a valid address when alignment has been requested. The impact of this is
that we won't be able to restore the firmware allocated BAR, which does
not meet alignment requirements anyway.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v2:
* capitalize subject text
* clarify commit message
* skip pcibios_save_fw_addr() call
---
 arch/x86/pci/i386.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 3abd55902dbc..13d7f7ac3bde 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -256,7 +256,7 @@ static void alloc_resource(struct pci_dev *dev, int idx, int pass)
 		if (r->flags & IORESOURCE_PCI_FIXED) {
 			dev_info(&dev->dev, "BAR %d %pR is immovable\n",
 				 idx, r);
-		} else {
+		} else if (!(r->flags & IORESOURCE_STARTALIGN)) {
 			/* We'll assign a new address later */
 			pcibios_save_fw_addr(dev, idx, r->start);
 			r->end -= r->start;
-- 
2.45.2

[PATCH v2 6/8] powerpc/pci: Preserve IORESOURCE_STARTALIGN alignment

[Stewart Hildebrand]

There is a corner case in pcibios_allocate_resources()/alloc_resource()
where the IORESOURCE_STARTALIGN alignment of memory BAR resources gets
overwritten. This does not affect bridge resources. Account for
IORESOURCE_STARTALIGN.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v2:
* new patch
---
 arch/powerpc/kernel/pci-common.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index eac84d687b53..3f346ad641e0 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1303,8 +1303,10 @@ static inline void alloc_resource(struct pci_dev *dev, int idx)
 			pr_debug("PCI:  parent is %p: %pR\n", pr, pr);
 		/* We'll assign a new address later */
 		r->flags |= IORESOURCE_UNSET;
-		r->end -= r->start;
-		r->start = 0;
+		if (!(r->flags & IORESOURCE_STARTALIGN)) {
+			r->end -= r->start;
+			r->start = 0;
+		}
 	}
 }
 
-- 
2.45.2

[PATCH v2 7/8] PCI: Don't reassign resources that are already aligned

[Stewart Hildebrand]

As a follow-up to commit 0dde1c08d1b9 ("PCI: Don't reassign resources
that are already aligned"), don't reassign already aligned resources
that have requested alignment without resizing.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v2:
* capitalize subject text
---
 drivers/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1ac1435ad91e..6df318beff37 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6585,6 +6585,9 @@ static bool pci_request_resource_alignment(struct pci_dev *dev, int bar,
 	if (size >= align)
 		return false;
 
+	if (!resize && (r->start > align) && !(r->start & (align - 1)))
+		return false;
+
 	/*
 	 * Increase the alignment of the resource.  There are two ways we
 	 * can do this:
-- 
2.45.2

[PATCH v2 8/8] PCI: Align small (<4k) BARs

[Stewart Hildebrand]

Issues observed when small (<4k) BARs are not 4k aligned are:

1. Devices to be passed through (to e.g. a Xen HVM guest) with small
(<4k) BARs require each memory BAR to be page aligned. Currently, the
only way to guarantee this alignment from a user perspective is to fake
the size of the BARs using the pci=resource_alignment= option. This is a
bad user experience, and faking the BAR size is not always desirable.
For example, pcitest is a tool that is useful for PCI passthrough
validation with Xen, but pcitest fails with a fake BAR size.

2. Devices with multiple small (<4k) BARs could have the MSI-X tables
located in one of its small (<4k) BARs. This may lead to the MSI-X
tables being mapped in the same 4k region as other data. The PCIe 6.1
specification (section 7.7.2 MSI-X Capability and Table Structure) says
we probably shouldn't do that.

To improve the user experience (i.e. don't require the user to specify
pci=resource_alignment=), and increase conformance to PCIe spec, set the
default minimum resource alignment of memory BARs to the greater of 4k
or PAGE_SIZE.

Quoting the comment in
drivers/pci/pci.c:pci_request_resource_alignment(), there are two ways
we can increase the resource alignment:

1) Increase the size of the resource.  BARs are aligned on their
   size, so when we reallocate space for this resource, we'll
   allocate it with the larger alignment.  This also prevents
   assignment of any other BARs inside the alignment region, so
   if we're requesting page alignment, this means no other BARs
   will share the page.

   The disadvantage is that this makes the resource larger than
   the hardware BAR, which may break drivers that compute things
   based on the resource size, e.g., to find registers at a
   fixed offset before the end of the BAR.

2) Retain the resource size, but use IORESOURCE_STARTALIGN and
   set r->start to the desired alignment.  By itself this
   doesn't prevent other BARs being put inside the alignment
   region, but if we realign *every* resource of every device in
   the system, none of them will share an alignment region.

   When the user has requested alignment for only some devices via
   the "pci=resource_alignment" argument, "resize" is true and we
   use the first method.  Otherwise we assume we're aligning all
   devices and we use the second.

Changing pcibios_default_alignment() results in the second method of
alignment with IORESOURCE_STARTALIGN.

The new default alignment may be overridden by arches by implementing
pcibios_default_alignment(), or by the user on a per-device basis with
the pci=resource_alignment= option (although this reverts to using
IORESOURCE_SIZEALIGN).

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Preparatory patches in this series are prerequisites to this patch.

v1->v2:
* capitalize subject text
* s/4 * 1024/SZ_4K/
* #include <linux/sizes.h>
* update commit message
* use max(SZ_4K, PAGE_SIZE) for alignment value
---
 drivers/pci/pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6df318beff37..6b85a204ec4e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -31,6 +31,7 @@
 #include <asm/dma.h>
 #include <linux/aer.h>
 #include <linux/bitfield.h>
+#include <linux/sizes.h>
 #include "pci.h"
 
 DEFINE_MUTEX(pci_slot_mutex);
@@ -6485,7 +6486,12 @@ struct pci_dev __weak *pci_real_dma_dev(struct pci_dev *dev)
 
 resource_size_t __weak pcibios_default_alignment(void)
 {
-	return 0;
+	/*
+	 * Avoid MSI-X tables being mapped in the same 4k region as other data
+	 * according to PCIe 6.1 specification section 7.7.2 MSI-X Capability
+	 * and Table Structure.
+	 */
+	return max(SZ_4K, PAGE_SIZE);
 }
 
 /*
-- 
2.45.2

RE: [PATCH v2 0/8] PCI: Align small (<4k) BARs

[David Laight]

From: Stewart Hildebrand
> Sent: 16 July 2024 20:33
> 
> This series sets the default minimum resource alignment to 4k for memory
> BARs. In preparation, it makes an optimization and addresses some corner
> cases observed when reallocating BARs. I consider the prepapatory
> patches to be prerequisites to changing the default BAR alignment.

Should the BARs be page aligned on systems with large pages?
At least as an option for hypervisor pass-through and any than can be mmap()ed
into userspace.

Does any hardware actually have 'silly numbers' of small memory BARs?

I have a vague memory of some ethernet controllers having lots of (?)
virtual devices that might have separate registers than can be mapped
out to a hypervisor.
Expanding those to a large page might be problematic - but needed for security.

For more normal hardware just ensuring that two separate targets don't share
a page while allowing (eg) two 1k BAR to reside in the same 64k page would
give some security.

Aligning a small MSIX BAR is unlikely to have any effect on the address
space utilisation (for PCIe) since the bridge will assign a power of two
sized block - with a big pad (useful for generating pcie errors!)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2 0/8] PCI: Align small (<4k) BARs

[David Laight]

From: David Laight
> Sent: 17 July 2024 14:15
> 
> From: Stewart Hildebrand
> > Sent: 16 July 2024 20:33
> >
> > This series sets the default minimum resource alignment to 4k for memory
> > BARs. In preparation, it makes an optimization and addresses some corner
> > cases observed when reallocating BARs. I consider the prepapatory
> > patches to be prerequisites to changing the default BAR alignment.
> 
> Should the BARs be page aligned on systems with large pages?
> At least as an option for hypervisor pass-through and any than can be mmap()ed
> into userspace.

The actual patch says PAGE_SIZE ..

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 0/8] PCI: Align small (<4k) BARs

[Stewart Hildebrand]

On 7/17/24 09:15, David Laight wrote:
> From: Stewart Hildebrand
>> Sent: 16 July 2024 20:33
>>
>> This series sets the default minimum resource alignment to 4k for memory
>> BARs. In preparation, it makes an optimization and addresses some corner
>> cases observed when reallocating BARs. I consider the prepapatory
>> patches to be prerequisites to changing the default BAR alignment.
> 
> Should the BARs be page aligned on systems with large pages?
> At least as an option for hypervisor pass-through and any than can be mmap()ed
> into userspace.

It is sort of an option already using the pci=resource_alignment=
option, but you'd need to spell out every device and manually set the
alignment value, and you'd still end up with fake BAR sizes. I had
actually prepared locally a patch to make this less painful to do and
preserve the BAR size (introduce "pci=resource_alignment=all" option),
but I'd like Bjorn's opinion before sending since there has been some
previous reluctance to making changes to the pci=resource_alignment=
option [2].

[2] https://lore.kernel.org/linux-pci/20160929115422.GA31048@localhost/

Anyway, 4k is more defensible because that is what the PCIe 6.1 spec
calls out, and that is better than the current situation of no default
minimum alignment.

I feel PAGE_SIZE is also justified, and that is why the actual patch now
says max(SZ_4K, PAGE_SIZE) as you pointed out elsewhere. This is a
change from v1 that simply had 4k (sorry, I forgot to update the cover
letter). PowerNV has been using PAGE_SIZE since commits 0a701aa63784 and
382746376993, I think with 64k page size. I don't have a strong opinion
whether the common default should be max(SZ_4K, PAGE_SIZE) or simply
SZ_4K.

> Does any hardware actually have 'silly numbers' of small memory BARs?
> 
> I have a vague memory of some ethernet controllers having lots of (?)
> virtual devices that might have separate registers than can be mapped
> out to a hypervisor.
> Expanding those to a large page might be problematic - but needed for security.

This series does not change alignment of SRIOV / VF BARs. See commits
62d9a78f32d9, ea629d873f3e, and PCIe 6.1 spec section 9.2.1.1.1.

> For more normal hardware just ensuring that two separate targets don't share
> a page while allowing (eg) two 1k BAR to reside in the same 64k page would
> give some security.

Allow me to understand this better, with an example:

PCI Device A
    BAR 1 (1k)
    BAR 2 (1k)

PCI Device B
    BAR 1 (1k)
    BAR 2 (1k)

We align all BARs to 4k. Additionally, are you saying it would be ok to
let both device A BARs to reside in the same 64k page, while device B
BARs would need to reside in a separate 64k page? I.e. having two levels
of alignment: PAGE_SIZE on a per-device basis, and 4k on a per-BAR
basis?

If I understand you correctly, there's currently no logic in the PCI
subsystem to easily support this, so that is a rather large ask. I'm
also not sure that it's necessary.

> Aligning a small MSIX BAR is unlikely to have any effect on the address
> space utilisation (for PCIe) since the bridge will assign a power of two
> sized block - with a big pad (useful for generating pcie errors!)
> 
> 	David

Re: [PATCH v2 1/8] x86/PCI: Move some logic to new function

[Philipp Stanner]

On Tue, 2024-07-16 at 15:32 -0400, Stewart Hildebrand wrote:
> ... to reduce indentation level. Take the opportunity to remove
> redundant info from debug print string.

Is that intended to be the final commit message or is it still a draft?

I'd call the commit "x86/PCI: Improve code readability" (or sth like
that) since that is what the commit is about. It's not so much about
the code move per se.

and have a short message such as:

"
The indentation in pcibios_allocate_dev_resources() is unusally deep.

Improve that by moving some of its code to the new function
alloc_resource().

As we're at it, remove redundant information from dev_dbg().
"


Regards,
P.

> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> v1->v2:
> * new patch
> ---
>  arch/x86/pci/i386.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
> index f2f4a5d50b27..3abd55902dbc 100644
> --- a/arch/x86/pci/i386.c
> +++ b/arch/x86/pci/i386.c
> @@ -246,6 +246,25 @@ struct pci_check_idx_range {
>         int end;
>  };
>  
> +static void alloc_resource(struct pci_dev *dev, int idx, int pass)
> +{
> +       struct resource *r = &dev->resource[idx];
> +
> +       dev_dbg(&dev->dev, "BAR %d: reserving %pr (p=%d)\n", idx, r,
> pass);
> +
> +       if (pci_claim_resource(dev, idx) < 0) {
> +               if (r->flags & IORESOURCE_PCI_FIXED) {
> +                       dev_info(&dev->dev, "BAR %d %pR is
> immovable\n",
> +                                idx, r);
> +               } else {
> +                       /* We'll assign a new address later */
> +                       pcibios_save_fw_addr(dev, idx, r->start);
> +                       r->end -= r->start;
> +                       r->start = 0;
> +               }
> +       }
> +}
> +
>  static void pcibios_allocate_dev_resources(struct pci_dev *dev, int
> pass)
>  {
>         int idx, disabled, i;
> @@ -271,23 +290,8 @@ static void
> pcibios_allocate_dev_resources(struct pci_dev *dev, int pass)
>                                 disabled = !(command &
> PCI_COMMAND_IO);
>                         else
>                                 disabled = !(command &
> PCI_COMMAND_MEMORY);
> -                       if (pass == disabled) {
> -                               dev_dbg(&dev->dev,
> -                                       "BAR %d: reserving %pr (d=%d,
> p=%d)\n",
> -                                       idx, r, disabled, pass);
> -                               if (pci_claim_resource(dev, idx) < 0)
> {
> -                                       if (r->flags &
> IORESOURCE_PCI_FIXED) {
> -                                               dev_info(&dev->dev,
> "BAR %d %pR is immovable\n",
> -                                                        idx, r);
> -                                       } else {
> -                                               /* We'll assign a new
> address later */
> -
>                                                pcibios_save_fw_addr(de
> v,
> -                                                               idx,
> r->start);
> -                                               r->end -= r->start;
> -                                               r->start = 0;
> -                                       }
> -                               }
> -                       }
> +                       if (pass == disabled)
> +                               alloc_resource(dev, idx, pass);
>                 }
>         if (!pass) {
>                 r = &dev->resource[PCI_ROM_RESOURCE];

RE: [PATCH v2 0/8] PCI: Align small (<4k) BARs

[David Laight]

From: Stewart Hildebrand
> Sent: 17 July 2024 19:31
...
> > For more normal hardware just ensuring that two separate targets don't share
> > a page while allowing (eg) two 1k BAR to reside in the same 64k page would
> > give some security.
> 
> Allow me to understand this better, with an example:
> 
> PCI Device A
>     BAR 1 (1k)
>     BAR 2 (1k)
> 
> PCI Device B
>     BAR 1 (1k)
>     BAR 2 (1k)
> 
> We align all BARs to 4k. Additionally, are you saying it would be ok to
> let both device A BARs to reside in the same 64k page, while device B
> BARs would need to reside in a separate 64k page? I.e. having two levels
> of alignment: PAGE_SIZE on a per-device basis, and 4k on a per-BAR
> basis?
> 
> If I understand you correctly, there's currently no logic in the PCI
> subsystem to easily support this, so that is a rather large ask. I'm
> also not sure that it's necessary.

That is what I was thinking, but it probably doesn't matter.
It would only be necessary if the system would otherwise run out
of PCI(e) address space.

Even after I reduced our FPGAs BARs from 32MB to 'only' 4MB (1MB + 1MB + 8k)
we still get issues with some PC bios failing to allocate the resources
in some slots - but these are old x86-64 systems that might have been expected
to run 32bit windows.
The requirement to use a separate BAR for MSIX pretty much doubles the
required address space.

As an aside, if a PCIe device asks for:
	BAR-0 (4k)
	BAR-1 (8k)
	BAR-2 (4k)
(which is a bit silly)
does it get packed into 16k with no padding by assigning BAR-2 between
BAR-0 and BAR-1, or is it all padded out to 32k.
I'd probably add a comment to say it isn't done :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 0/8] PCI: Align small (<4k) BARs

[Stewart Hildebrand]

On 7/18/24 06:01, David Laight wrote:
> From: Stewart Hildebrand
>> Sent: 17 July 2024 19:31
> ...
>>> For more normal hardware just ensuring that two separate targets don't share
>>> a page while allowing (eg) two 1k BAR to reside in the same 64k page would
>>> give some security.
>>
>> Allow me to understand this better, with an example:
>>
>> PCI Device A
>>     BAR 1 (1k)
>>     BAR 2 (1k)
>>
>> PCI Device B
>>     BAR 1 (1k)
>>     BAR 2 (1k)
>>
>> We align all BARs to 4k. Additionally, are you saying it would be ok to
>> let both device A BARs to reside in the same 64k page, while device B
>> BARs would need to reside in a separate 64k page? I.e. having two levels
>> of alignment: PAGE_SIZE on a per-device basis, and 4k on a per-BAR
>> basis?
>>
>> If I understand you correctly, there's currently no logic in the PCI
>> subsystem to easily support this, so that is a rather large ask. I'm
>> also not sure that it's necessary.
> 
> That is what I was thinking, but it probably doesn't matter.
> It would only be necessary if the system would otherwise run out
> of PCI(e) address space.
> 
> Even after I reduced our FPGAs BARs from 32MB to 'only' 4MB (1MB + 1MB + 8k)
> we still get issues with some PC bios failing to allocate the resources
> in some slots - but these are old x86-64 systems that might have been expected
> to run 32bit windows.

I expect this series will not make any difference with that particular
scenario since the BARs are >4k (and PAGE_SIZE == 4k on x86).

> The requirement to use a separate BAR for MSIX pretty much doubles the
> required address space.

4k region, not BAR.

> As an aside, if a PCIe device asks for:
> 	BAR-0 (4k)
> 	BAR-1 (8k)
> 	BAR-2 (4k)
> (which is a bit silly)
> does it get packed into 16k with no padding by assigning BAR-2 between
> BAR-0 and BAR-1, or is it all padded out to 32k.
> I'd probably add a comment to say it isn't done :-)

On a system with 4k page size, this series should not affect the example
you've provided since those BARs are all 4k or larger.

If you are testing with this series applied to your kernel and notice
any regression, please let me know.

Re: [PATCH v2 1/8] x86/PCI: Move some logic to new function

[Stewart Hildebrand]

On 7/17/24 15:28, Philipp Stanner wrote:
> On Tue, 2024-07-16 at 15:32 -0400, Stewart Hildebrand wrote:
>> ... to reduce indentation level. Take the opportunity to remove
>> redundant info from debug print string.
> 
> Is that intended to be the final commit message or is it still a draft?
> 
> I'd call the commit "x86/PCI: Improve code readability" (or sth like
> that) since that is what the commit is about. It's not so much about
> the code move per se.
> 
> and have a short message such as:
> 
> "
> The indentation in pcibios_allocate_dev_resources() is unusally deep.
> 
> Improve that by moving some of its code to the new function
> alloc_resource().
> 
> As we're at it, remove redundant information from dev_dbg().
> "
> 
> 
> Regards,
> P.

Thanks for the suggestion. I'll rework it.