[PATCH v3 0/7] PCI: Add support for enforcing all MMIO BARs not to share PAGE_SIZE

[PATCH v3 0/7] PCI: Add support for enforcing all MMIO BARs not to share PAGE_SIZE

[Yongji Xie]

This series aims to add an option for PCI resource allocator to  
force BARs not to share PAGE_SIZE. This would make sense to VFIO 
driver. Because current VFIO implementation disallows to mmap 
sub-page(size < PAGE_SIZE) MMIO BARs which may share the same page 
with other BARs for security reasons. Thus, we have to handle mmio 
access to these BARs in QEMU emulation rather than in guest which 
will cause some performance loss.

To achieve that, we would like to make use of the existing 
resource_alignment kernel parameter and force a minimum alignment 
of PAGE_SIZE. It's flexible. And we can enable it by default on
some archs which may easily hit the performance issue because of  
their 64K page.

In this series, patch 1,2,3 fixed bugs of using resource_alignment;
patch 4,5 tried to add a new option for resource_alignment to use 
IORESOURCE_STARTALIGN to specify the alignment of PCI BARs; patch 6
modified resource_alignment to support syntax which can be used to
enforce the alignment of all MMIO BARs to be at least PAGE_SIZE;
patch 7 enabled this feature by default on PowerNV platform.

Changelog v3:
- Ignore enforced alignment to fixed BARs
- Fix issue that disabling memory decoding when reassigning the alignment
- Only enable default alignment on PowerNV platform

Changelog v2:
- Ignore enforced alignment to VF BARs on pci_reassigndev_resource_alignment()

Yongji Xie (7):
  PCI: Ignore enforced alignment when kernel uses existing firmware setup
  PCI: Ignore enforced alignment to VF BARs
  PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()
  PCI: Add a new option for resource_alignment to reassign alignment
  PCI: Do not use IORESOURCE_STARTALIGN to identify bridge resources
  PCI: Add support for enforcing all MMIO BARs to be page aligned
  PCI: Add a macro to set default alignment for all PCI devices

 Documentation/kernel-parameters.txt |    7 +-
 arch/powerpc/include/asm/pci.h      |    4 ++
 drivers/pci/pci.c                   |  123 +++++++++++++++++++++++++++--------
 drivers/pci/setup-bus.c             |    9 ++-
 4 files changed, 111 insertions(+), 32 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/7] PCI: Ignore enforced alignment when kernel uses existing firmware setup

[Yongji Xie]

PCI resources allocator will use firmware setup and not try to
reassign resource when PCI_PROBE_ONLY or IORESOURCE_PCI_FIXED
is set.

The enforced alignment in pci_reassigndev_resource_alignment()
should be ignored in this case. Otherwise, some PCI devices'
resources would be released here and not re-allocated.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/pci/pci.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c8b4dbd..be8f72c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4760,6 +4760,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
 
 	spin_lock(&resource_alignment_lock);
 	p = resource_alignment_param;
+	if (pci_has_flag(PCI_PROBE_ONLY)) {
+		if (*p)
+			printk_once(KERN_INFO "PCI: Ignore resource_alignment parameter: %s with PCI_PROBE_ONLY set\n",
+						p);
+		spin_unlock(&resource_alignment_lock);
+		return 0;
+	}
 	while (*p) {
 		count = 0;
 		if (sscanf(p, "%d%n", &align_order, &count) == 1 &&
@@ -4837,6 +4844,11 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		r = &dev->resource[i];
 		if (!(r->flags & IORESOURCE_MEM))
 			continue;
+		if (r->flags & IORESOURCE_PCI_FIXED) {
+			dev_info(&dev->dev, "No alignment for fixed BAR%d: %pR\n",
+				i, r);
+			continue;
+		}
 		size = resource_size(r);
 		if (size < align) {
 			size = align;
-- 
1.7.9.5

[PATCH v3 2/7] PCI: Ignore enforced alignment to VF BARs

[Yongji Xie]

VF BARs are read-only zeroes according to SRIOV spec,
the normal way(writing BARs) of allocating resources wouldn't
be applied to VFs. The VFs' resources would be allocated
when we enable SR-IOV capability. So we should not try to
reassign alignment after we enable VFs. It's meaningless
and will release the allocated resources which leads to a bug.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/pci/pci.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index be8f72c..6ae02de 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4822,6 +4822,10 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	resource_size_t align, size;
 	u16 command;
 
+	/* We should never try to reassign VF's alignment */
+	if (dev->is_virtfn)
+		return;
+
 	/* check if specified PCI is target device to reassign */
 	align = pci_specified_resource_alignment(dev);
 	if (!align)
-- 
1.7.9.5

[PATCH v3 3/7] PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()

[Yongji Xie]

We should not disable memory decoding when we reassign alignment
in pci_reassigndev_resource_alignment(). It's meaningless and
have some side effect. For example, some fixup functions such as
quirk_e100_interrupt() read PCI_COMMAND_MEMORY to know whether
the devices has been initialized by the firmware or not. If we
disable memory decoding here, these functions will get a wrong
information that the devices was not initialized by the firmware
which may cause a wrong fixup. Besides, disabling memory decoding
may also break some devices that need to have memory decoding
always-on during probing.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/pci/pci.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6ae02de..6241cfc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4820,7 +4820,6 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	int i;
 	struct resource *r;
 	resource_size_t align, size;
-	u16 command;
 
 	/* We should never try to reassign VF's alignment */
 	if (dev->is_virtfn)
@@ -4838,12 +4837,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		return;
 	}
 
-	dev_info(&dev->dev,
-		"Disabling memory decoding and releasing memory resources.\n");
-	pci_read_config_word(dev, PCI_COMMAND, &command);
-	command &= ~PCI_COMMAND_MEMORY;
-	pci_write_config_word(dev, PCI_COMMAND, command);
-
+	dev_info(&dev->dev, "Releasing memory resources.\n");
 	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
 		r = &dev->resource[i];
 		if (!(r->flags & IORESOURCE_MEM))
-- 
1.7.9.5

[PATCH v3 4/7] PCI: Add a new option for resource_alignment to reassign alignment

[Yongji Xie]

When using resource_alignment kernel parameter, the current
implement reassigns the alignment by changing resources' size
which can potentially break some drivers. For example, the driver
uses the size to locate some register whose length is related
to the size.

This patch adds a new option "noresize" for the parameter to
solve this problem.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 Documentation/kernel-parameters.txt |    5 ++++-
 drivers/pci/pci.c                   |   35 +++++++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c9..c4802f5 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2997,13 +2997,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				window. The default value is 64 megabytes.
 		resource_alignment=
 				Format:
-				[<order of align>@][<domain>:]<bus>:<slot>.<func>[; ...]
+				[<order of align>@][<domain>:]<bus>:<slot>.<func>
+				[:noresize][; ...]
 				Specifies alignment and device to reassign
 				aligned memory resources.
 				If <order of align> is not specified,
 				PAGE_SIZE is used as alignment.
 				PCI-PCI bridge can be specified, if resource
 				windows need to be expanded.
+				noresize: Don't change the resources' sizes when
+				reassigning alignment.
 		ecrc=		Enable/disable PCIe ECRC (transaction layer
 				end-to-end CRC checking).
 				bios: Use BIOS/firmware settings. This is the
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6241cfc..1d80a94 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4748,11 +4748,13 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
 /**
  * pci_specified_resource_alignment - get resource alignment specified by user.
  * @dev: the PCI device to get
+ * @resize: whether or not to change resources' size when reassigning alignment
  *
  * RETURNS: Resource alignment if it is specified.
  *          Zero if it is not specified.
  */
-static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
+static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
+		bool *resize)
 {
 	int seg, bus, slot, func, align_order, count;
 	resource_size_t align = 0;
@@ -4787,6 +4789,11 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
 			}
 		}
 		p += count;
+		if (!strncmp(p, ":noresize", 9)) {
+			*resize = false;
+			p += 9;
+		} else
+			*resize = true;
 		if (seg == pci_domain_nr(dev->bus) &&
 			bus == dev->bus->number &&
 			slot == PCI_SLOT(dev->devfn) &&
@@ -4819,6 +4826,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 {
 	int i;
 	struct resource *r;
+	bool resize = true;
 	resource_size_t align, size;
 
 	/* We should never try to reassign VF's alignment */
@@ -4826,7 +4834,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		return;
 
 	/* check if specified PCI is target device to reassign */
-	align = pci_specified_resource_alignment(dev);
+	align = pci_specified_resource_alignment(dev, &resize);
 	if (!align)
 		return;
 
@@ -4848,15 +4856,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 			continue;
 		}
 		size = resource_size(r);
-		if (size < align) {
-			size = align;
-			dev_info(&dev->dev,
-				"Rounding up size of resource #%d to %#llx.\n",
-				i, (unsigned long long)size);
+		if (resize) {
+			if (size < align) {
+				size = align;
+				dev_info(&dev->dev,
+					"Rounding up size of resource #%d to %#llx.\n",
+					i, (unsigned long long)size);
+			}
+			r->flags |= IORESOURCE_UNSET;
+			r->end = size - 1;
+			r->start = 0;
+		} else {
+			r->flags &= ~IORESOURCE_SIZEALIGN;
+			r->flags |= IORESOURCE_STARTALIGN | IORESOURCE_UNSET;
+			r->start = max(align, size);
+			r->end = r->start + size - 1;
 		}
-		r->flags |= IORESOURCE_UNSET;
-		r->end = size - 1;
-		r->start = 0;
 	}
 	/* Need to disable bridge's resource window,
 	 * to enable the kernel to reassign new resource
-- 
1.7.9.5

[PATCH v3 5/7] PCI: Do not use IORESOURCE_STARTALIGN to identify bridge resources

[Yongji Xie]

Now we use the IORESOURCE_STARTALIGN to identify bridge resources
in __assign_resources_sorted(). That's quite fragile. We may also
set flag IORESOURCE_STARTALIGN for SR-IOV resources in some cases,
for example, using the option "noresize" of parameter
"pci=resource_alignment".

In this patch, we try to use a more robust way to identify
bridge resources.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 55641a3..216ddbc 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -390,6 +390,7 @@ static void __assign_resources_sorted(struct list_head *head,
 	struct pci_dev_resource *dev_res, *tmp_res, *dev_res2;
 	unsigned long fail_type;
 	resource_size_t add_align, align;
+	int index;
 
 	/* Check if optional add_size is there */
 	if (!realloc_head || list_empty(realloc_head))
@@ -410,11 +411,13 @@ static void __assign_resources_sorted(struct list_head *head,
 
 		/*
 		 * There are two kinds of additional resources in the list:
-		 * 1. bridge resource  -- IORESOURCE_STARTALIGN
-		 * 2. SR-IOV resource   -- IORESOURCE_SIZEALIGN
+		 * 1. bridge resource
+		 * 2. SR-IOV resource
 		 * Here just fix the additional alignment for bridge
 		 */
-		if (!(dev_res->res->flags & IORESOURCE_STARTALIGN))
+		index = dev_res->res - dev_res->dev->resource;
+		if (index < PCI_BRIDGE_RESOURCES ||
+			index > PCI_BRIDGE_RESOURCE_END)
 			continue;
 
 		add_align = get_res_add_align(realloc_head, dev_res->res);
-- 
1.7.9.5

[PATCH v3 6/7] PCI: Add support for enforcing all MMIO BARs to be page aligned

[Yongji Xie]

When vfio passthrough a PCI device of which MMIO BARs are
smaller than PAGE_SIZE, guest will not handle the mmio
accesses to the BARs which leads to mmio emulations in host.

This is because vfio will not allow to passthrough one BAR's
mmio page which may be shared with other BARs. Otherwise,
there will be a backdoor that guest can use to access BARs
of other guest.

To solve this issue, this patch modifies resource_alignment
to support syntax where multiple devices get the same
alignment. So we can use something like
"pci=resource_alignment=*:*:*.*:noresize" to enforce the
alignment of all MMIO BARs to be at least PAGE_SIZE so that
one BAR's mmio page would not be shared with other BARs.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 Documentation/kernel-parameters.txt |    2 ++
 drivers/pci/pci.c                   |   60 ++++++++++++++++++++++++++++-------
 2 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c4802f5..cb09503 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3003,6 +3003,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				aligned memory resources.
 				If <order of align> is not specified,
 				PAGE_SIZE is used as alignment.
+				<domain>, <bus>, <slot> and <func> can be set to
+				"*" which means match all values.
 				PCI-PCI bridge can be specified, if resource
 				windows need to be expanded.
 				noresize: Don't change the resources' sizes when
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1d80a94..2e15ac8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4759,6 +4759,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 	int seg, bus, slot, func, align_order, count;
 	resource_size_t align = 0;
 	char *p;
+	bool invalid = false;
 
 	spin_lock(&resource_alignment_lock);
 	p = resource_alignment_param;
@@ -4777,16 +4778,49 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 		} else {
 			align_order = -1;
 		}
-		if (sscanf(p, "%x:%x:%x.%x%n",
-			&seg, &bus, &slot, &func, &count) != 4) {
+		if (p[0] == '*' && p[1] == ':') {
+			seg = -1;
+			count = 1;
+		} else if (sscanf(p, "%x%n", &seg, &count) != 1 ||
+				p[count] != ':') {
+			invalid = true;
+			break;
+		}
+		p += count + 1;
+		if (*p == '*') {
+			bus = -1;
+			count = 1;
+		} else if (sscanf(p, "%x%n", &bus, &count) != 1) {
+			invalid = true;
+			break;
+		}
+		p += count;
+		if (*p == '.') {
+			slot = bus;
+			bus = seg;
 			seg = 0;
-			if (sscanf(p, "%x:%x.%x%n",
-					&bus, &slot, &func, &count) != 3) {
-				/* Invalid format */
-				printk(KERN_ERR "PCI: Can't parse resource_alignment parameter: %s\n",
-					p);
+			p++;
+		} else if (*p == ':') {
+			p++;
+			if (p[0] == '*' && p[1] == '.') {
+				slot = -1;
+				count = 1;
+			} else if (sscanf(p, "%x%n", &slot, &count) != 1 ||
+					p[count] != '.') {
+				invalid = true;
 				break;
 			}
+			p += count + 1;
+		} else {
+			invalid = true;
+			break;
+		}
+		if (*p == '*') {
+			func = -1;
+			count = 1;
+		} else if (sscanf(p, "%x%n", &func, &count) != 1) {
+			invalid = true;
+			break;
 		}
 		p += count;
 		if (!strncmp(p, ":noresize", 9)) {
@@ -4794,10 +4828,10 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 			p += 9;
 		} else
 			*resize = true;
-		if (seg == pci_domain_nr(dev->bus) &&
-			bus == dev->bus->number &&
-			slot == PCI_SLOT(dev->devfn) &&
-			func == PCI_FUNC(dev->devfn)) {
+		if ((seg == pci_domain_nr(dev->bus) || seg == -1) &&
+			(bus == dev->bus->number || bus == -1) &&
+			(slot == PCI_SLOT(dev->devfn) || slot == -1) &&
+			(func == PCI_FUNC(dev->devfn) || func == -1)) {
 			if (align_order == -1)
 				align = PAGE_SIZE;
 			else
@@ -4807,10 +4841,14 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 		}
 		if (*p != ';' && *p != ',') {
 			/* End of param or invalid format */
+			invalid = true;
 			break;
 		}
 		p++;
 	}
+	if (invalid)
+		printk(KERN_ERR "PCI: Can't parse resource_alignment parameter:%s\n",
+				p);
 	spin_unlock(&resource_alignment_lock);
 	return align;
 }
-- 
1.7.9.5

[PATCH v3 7/7] PCI: Add a macro to set default alignment for all PCI devices

[Yongji Xie]

Now we can use something like "pci=resource_alignment=*:*:*.*:noresize"
to enforce the alignment of all MMIO BARs to be at least PAGE_SIZE so
that we can passthrough sub-page(size < PAGE_SIZE) BARs to guest in
VFIO module.

But sometimes we may want to enable this feature by default on
some platforms such as PowerNV platform which would easily see those
sub-page BARs because of its 64K page. To achieve that, we add a
macro PCIBIOS_DEFAULT_ALIGNMENT to set default alignment for all
PCI devices and define it on PowerNV platform.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/pci.h |    4 ++++
 drivers/pci/pci.c              |    4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index a6f3ac0..b0d76b8 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -28,6 +28,10 @@
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		0x10000000
 
+#ifdef CONFIG_PPC_POWERNV
+#define PCIBIOS_DEFAULT_ALIGNMENT	PAGE_SIZE
+#endif
+
 struct pci_dev;
 
 /* Values for the `which' argument to sys_pciconfig_iobase syscall.  */
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 2e15ac8..dde0cce 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4761,6 +4761,10 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 	char *p;
 	bool invalid = false;
 
+#ifdef PCIBIOS_DEFAULT_ALIGNMENT
+	align = PCIBIOS_DEFAULT_ALIGNMENT;
+	*resize = false;
+#endif
 	spin_lock(&resource_alignment_lock);
 	p = resource_alignment_param;
 	if (pci_has_flag(PCI_PROBE_ONLY)) {
-- 
1.7.9.5

Re: [PATCH v3 1/7] PCI: Ignore enforced alignment when kernel uses existing firmware setup

[Gavin Shan]

On Thu, Jun 30, 2016 at 06:53:07PM +0800, Yongji Xie wrote:
>PCI resources allocator will use firmware setup and not try to
>reassign resource when PCI_PROBE_ONLY or IORESOURCE_PCI_FIXED
>is set.
>
>The enforced alignment in pci_reassigndev_resource_alignment()
>should be ignored in this case. Otherwise, some PCI devices'
>resources would be released here and not re-allocated.
>
>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>---
> drivers/pci/pci.c |   12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index c8b4dbd..be8f72c 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -4760,6 +4760,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
>
> 	spin_lock(&resource_alignment_lock);
> 	p = resource_alignment_param;
>+	if (pci_has_flag(PCI_PROBE_ONLY)) {
>+		if (*p)
>+			printk_once(KERN_INFO "PCI: Ignore resource_alignment parameter: %s with PCI_PROBE_ONLY set\n",
>+						p);

We don't have to print @resource_alignment as it's ignored completely.
The content included in it doesn't mean anything:

			pr_info_once("PCI: resource_alignment ignored with PCI_PROBE_ONLY\n");

>+		spin_unlock(&resource_alignment_lock);
>+		return 0;
>+	}

A empty line is needed here.

> 	while (*p) {
> 		count = 0;
> 		if (sscanf(p, "%d%n", &align_order, &count) == 1 &&
>@@ -4837,6 +4844,11 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> 		r = &dev->resource[i];
> 		if (!(r->flags & IORESOURCE_MEM))
> 			continue;
>+		if (r->flags & IORESOURCE_PCI_FIXED) {
>+			dev_info(&dev->dev, "No alignment for fixed BAR%d: %pR\n",
>+				i, r);

The message would be like below to match PCI code style. I'm thinking it probably
uses dev_dbg() instead dev_info(), but not sure for 100%.

			dev_info(&dev->dev, "BAR %d: fixed %pR, no alignment\n", i, r);

>+			continue;
>+		}

A empty line is needed here.

> 		size = resource_size(r);
> 		if (size < align) {
> 			size = align;

Thanks,
Gavin

Re: [PATCH v3 2/7] PCI: Ignore enforced alignment to VF BARs

[Gavin Shan]

On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
>VF BARs are read-only zeroes according to SRIOV spec,
>the normal way(writing BARs) of allocating resources wouldn't
>be applied to VFs. The VFs' resources would be allocated
>when we enable SR-IOV capability. So we should not try to
>reassign alignment after we enable VFs. It's meaningless
>and will release the allocated resources which leads to a bug.
>
>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>---
> drivers/pci/pci.c |    4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index be8f72c..6ae02de 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -4822,6 +4822,10 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> 	resource_size_t align, size;
> 	u16 command;
>
>+	/* We should never try to reassign VF's alignment */
>+	if (dev->is_virtfn)
>+		return;
>+

Yongji, I think it's correct to ignore VF's BARs. Another concern is:
it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
example here: one PF has 16 VFs; each VF has only one 1KB. It means
the only PF IOV BAR is 16KB. I don't see how it works after expanding
it to 64KB which is the page size. It might be not a problem on PowerNV
platform, but potentially a issue on x86?

> 	/* check if specified PCI is target device to reassign */
> 	align = pci_specified_resource_alignment(dev);
> 	if (!align)

Thanks,
Gavin

Re: [PATCH v3 3/7] PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()

[Gavin Shan]

On Thu, Jun 30, 2016 at 06:53:09PM +0800, Yongji Xie wrote:
>We should not disable memory decoding when we reassign alignment
>in pci_reassigndev_resource_alignment(). It's meaningless and
>have some side effect. For example, some fixup functions such as
>quirk_e100_interrupt() read PCI_COMMAND_MEMORY to know whether
>the devices has been initialized by the firmware or not. If we
>disable memory decoding here, these functions will get a wrong
>information that the devices was not initialized by the firmware
>which may cause a wrong fixup. Besides, disabling memory decoding
>may also break some devices that need to have memory decoding
>always-on during probing.
>

It seems the changelog isn't correct enough if it's talking about
below check in code:

if (!(command & PCI_COMMAND_MEMORY) || !pci_resource_start(dev, 0))
	return;

>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>---
> drivers/pci/pci.c |    8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index 6ae02de..6241cfc 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -4820,7 +4820,6 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> 	int i;
> 	struct resource *r;
> 	resource_size_t align, size;
>-	u16 command;
>
> 	/* We should never try to reassign VF's alignment */
> 	if (dev->is_virtfn)
>@@ -4838,12 +4837,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> 		return;
> 	}
>
>-	dev_info(&dev->dev,
>-		"Disabling memory decoding and releasing memory resources.\n");
>-	pci_read_config_word(dev, PCI_COMMAND, &command);
>-	command &= ~PCI_COMMAND_MEMORY;
>-	pci_write_config_word(dev, PCI_COMMAND, command);
>-
>+	dev_info(&dev->dev, "Releasing memory resources.\n");

Is there a problem you found with PCI_COMMAND removed? If so, could you
please share more details, thanks.

> 	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
> 		r = &dev->resource[i];
> 		if (!(r->flags & IORESOURCE_MEM))

Thanks,
Gavin

Re: [PATCH v3 4/7] PCI: Add a new option for resource_alignment to reassign alignment

[Gavin Shan]

On Thu, Jun 30, 2016 at 06:53:10PM +0800, Yongji Xie wrote:
>When using resource_alignment kernel parameter, the current
>implement reassigns the alignment by changing resources' size
>which can potentially break some drivers. For example, the driver
>uses the size to locate some register whose length is related
>to the size.
>
>This patch adds a new option "noresize" for the parameter to
>solve this problem.
>
>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> Documentation/kernel-parameters.txt |    5 ++++-
> drivers/pci/pci.c                   |   35 +++++++++++++++++++++++++----------
> 2 files changed, 29 insertions(+), 11 deletions(-)
>
>diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>index 82b42c9..c4802f5 100644
>--- a/Documentation/kernel-parameters.txt
>+++ b/Documentation/kernel-parameters.txt
>@@ -2997,13 +2997,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> 				window. The default value is 64 megabytes.
> 		resource_alignment=
> 				Format:
>-				[<order of align>@][<domain>:]<bus>:<slot>.<func>[; ...]
>+				[<order of align>@][<domain>:]<bus>:<slot>.<func>
>+				[:noresize][; ...]
> 				Specifies alignment and device to reassign
> 				aligned memory resources.
> 				If <order of align> is not specified,
> 				PAGE_SIZE is used as alignment.
> 				PCI-PCI bridge can be specified, if resource
> 				windows need to be expanded.
>+				noresize: Don't change the resources' sizes when
>+				reassigning alignment.
> 		ecrc=		Enable/disable PCIe ECRC (transaction layer
> 				end-to-end CRC checking).
> 				bios: Use BIOS/firmware settings. This is the
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index 6241cfc..1d80a94 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -4748,11 +4748,13 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
> /**
>  * pci_specified_resource_alignment - get resource alignment specified by user.
>  * @dev: the PCI device to get
>+ * @resize: whether or not to change resources' size when reassigning alignment
>  *
>  * RETURNS: Resource alignment if it is specified.
>  *          Zero if it is not specified.
>  */
>-static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
>+static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
>+		bool *resize)
> {
> 	int seg, bus, slot, func, align_order, count;
> 	resource_size_t align = 0;
>@@ -4787,6 +4789,11 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
> 			}
> 		}
> 		p += count;
>+		if (!strncmp(p, ":noresize", 9)) {
>+			*resize = false;
>+			p += 9;
>+		} else
>+			*resize = true;
> 		if (seg == pci_domain_nr(dev->bus) &&
> 			bus == dev->bus->number &&
> 			slot == PCI_SLOT(dev->devfn) &&
>@@ -4819,6 +4826,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> {
> 	int i;
> 	struct resource *r;
>+	bool resize = true;
> 	resource_size_t align, size;
>
> 	/* We should never try to reassign VF's alignment */
>@@ -4826,7 +4834,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> 		return;
>
> 	/* check if specified PCI is target device to reassign */
>-	align = pci_specified_resource_alignment(dev);
>+	align = pci_specified_resource_alignment(dev, &resize);
> 	if (!align)
> 		return;
>
>@@ -4848,15 +4856,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> 			continue;
> 		}
> 		size = resource_size(r);
>-		if (size < align) {
>-			size = align;
>-			dev_info(&dev->dev,
>-				"Rounding up size of resource #%d to %#llx.\n",
>-				i, (unsigned long long)size);
>+		if (resize) {
>+			if (size < align) {
>+				size = align;
>+				dev_info(&dev->dev,
>+					"Rounding up size of resource #%d to %#llx.\n",
>+					i, (unsigned long long)size);
>+			}
>+			r->flags |= IORESOURCE_UNSET;
>+			r->end = size - 1;
>+			r->start = 0;
>+		} else {
>+			r->flags &= ~IORESOURCE_SIZEALIGN;
>+			r->flags |= IORESOURCE_STARTALIGN | IORESOURCE_UNSET;

			f->flags |= (IORESOURCE_STARTALIGN | IORESOURCE_UNSET);

Yongji, one quick question - when IORESOURCE_UNSET is cleared? If it's not
cleared, is there any unexpected side effects? Thanks.

>+			r->start = max(align, size);
>+			r->end = r->start + size - 1;
> 		}
>-		r->flags |= IORESOURCE_UNSET;
>-		r->end = size - 1;
>-		r->start = 0;
> 	}
> 	/* Need to disable bridge's resource window,
> 	 * to enable the kernel to reassign new resource

Thanks,
Gavin

Re: [PATCH v3 5/7] PCI: Do not use IORESOURCE_STARTALIGN to identify bridge resources

[Gavin Shan]

On Thu, Jun 30, 2016 at 06:53:11PM +0800, Yongji Xie wrote:
>Now we use the IORESOURCE_STARTALIGN to identify bridge resources
>in __assign_resources_sorted(). That's quite fragile. We may also
>set flag IORESOURCE_STARTALIGN for SR-IOV resources in some cases,
>for example, using the option "noresize" of parameter
>"pci=resource_alignment".
>
>In this patch, we try to use a more robust way to identify
>bridge resources.
>
>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

Yongji, I think this doesn't have to be part of this series, meaning
it can be sent or merged separately.

>---
> drivers/pci/setup-bus.c |    9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>index 55641a3..216ddbc 100644
>--- a/drivers/pci/setup-bus.c
>+++ b/drivers/pci/setup-bus.c
>@@ -390,6 +390,7 @@ static void __assign_resources_sorted(struct list_head *head,
> 	struct pci_dev_resource *dev_res, *tmp_res, *dev_res2;
> 	unsigned long fail_type;
> 	resource_size_t add_align, align;
>+	int index;
>
> 	/* Check if optional add_size is there */
> 	if (!realloc_head || list_empty(realloc_head))
>@@ -410,11 +411,13 @@ static void __assign_resources_sorted(struct list_head *head,
>
> 		/*
> 		 * There are two kinds of additional resources in the list:
>-		 * 1. bridge resource  -- IORESOURCE_STARTALIGN
>-		 * 2. SR-IOV resource   -- IORESOURCE_SIZEALIGN
>+		 * 1. bridge resource
>+		 * 2. SR-IOV resource
> 		 * Here just fix the additional alignment for bridge
> 		 */
>-		if (!(dev_res->res->flags & IORESOURCE_STARTALIGN))
>+		index = dev_res->res - dev_res->dev->resource;
>+		if (index < PCI_BRIDGE_RESOURCES ||
>+			index > PCI_BRIDGE_RESOURCE_END)
> 			continue;
>
> 		add_align = get_res_add_align(realloc_head, dev_res->res);

Thanks,
Gavin

Re: [PATCH v3 1/7] PCI: Ignore enforced alignment when kernel uses existing firmware setup

[Yongji Xie]

Hi Gavin,

On 2016/7/1 8:28, Gavin Shan wrote:

> On Thu, Jun 30, 2016 at 06:53:07PM +0800, Yongji Xie wrote:
>> PCI resources allocator will use firmware setup and not try to
>> reassign resource when PCI_PROBE_ONLY or IORESOURCE_PCI_FIXED
>> is set.
>>
>> The enforced alignment in pci_reassigndev_resource_alignment()
>> should be ignored in this case. Otherwise, some PCI devices'
>> resources would be released here and not re-allocated.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>> drivers/pci/pci.c |   12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index c8b4dbd..be8f72c 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4760,6 +4760,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
>>
>> 	spin_lock(&resource_alignment_lock);
>> 	p = resource_alignment_param;
>> +	if (pci_has_flag(PCI_PROBE_ONLY)) {
>> +		if (*p)
>> +			printk_once(KERN_INFO "PCI: Ignore resource_alignment parameter: %s with PCI_PROBE_ONLY set\n",
>> +						p);
> We don't have to print @resource_alignment as it's ignored completely.
> The content included in it doesn't mean anything:
>
> 			pr_info_once("PCI: resource_alignment ignored with PCI_PROBE_ONLY\n");

OK. It looks simpler now.

>> +		spin_unlock(&resource_alignment_lock);
>> +		return 0;
>> +	}
> A empty line is needed here.
>
>> 	while (*p) {
>> 		count = 0;
>> 		if (sscanf(p, "%d%n", &align_order, &count) == 1 &&
>> @@ -4837,6 +4844,11 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> 		r = &dev->resource[i];
>> 		if (!(r->flags & IORESOURCE_MEM))
>> 			continue;
>> +		if (r->flags & IORESOURCE_PCI_FIXED) {
>> +			dev_info(&dev->dev, "No alignment for fixed BAR%d: %pR\n",
>> +				i, r);
> The message would be like below to match PCI code style. I'm thinking it probably
> uses dev_dbg() instead dev_info(), but not sure for 100%.
>
> 			dev_info(&dev->dev, "BAR %d: fixed %pR, no alignment\n", i, r);

Maybe dev_info() is OK.:-)

>> +			continue;
>> +		}
> A empty line is needed here.
>
>> 		size = resource_size(r);
>> 		if (size < align) {
>> 			size = align;
> Thanks,
> Gavin
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 2/7] PCI: Ignore enforced alignment to VF BARs

[Yongji Xie]

Hi Gavin,

On 2016/7/1 8:39, Gavin Shan wrote:

> On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
>> VF BARs are read-only zeroes according to SRIOV spec,
>> the normal way(writing BARs) of allocating resources wouldn't
>> be applied to VFs. The VFs' resources would be allocated
>> when we enable SR-IOV capability. So we should not try to
>> reassign alignment after we enable VFs. It's meaningless
>> and will release the allocated resources which leads to a bug.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>> drivers/pci/pci.c |    4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index be8f72c..6ae02de 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4822,6 +4822,10 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> 	resource_size_t align, size;
>> 	u16 command;
>>
>> +	/* We should never try to reassign VF's alignment */
>> +	if (dev->is_virtfn)
>> +		return;
>> +
> Yongji, I think it's correct to ignore VF's BARs. Another concern is:
> it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
> example here: one PF has 16 VFs; each VF has only one 1KB. It means
> the only PF IOV BAR is 16KB. I don't see how it works after expanding
> it to 64KB which is the page size. It might be not a problem on PowerNV
> platform, but potentially a issue on x86?

Seems like the alignment would not be applied to IOV BARs because
pci_reassigndev_resource_alignment() will be called before
sriov_init().

Thanks,
Yongji

Re: [PATCH v3 2/7] PCI: Ignore enforced alignment to VF BARs

[Gavin Shan]

On Fri, Jul 01, 2016 at 01:27:17PM +0800, Yongji Xie wrote:
>>On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
>>>VF BARs are read-only zeroes according to SRIOV spec,
>>>the normal way(writing BARs) of allocating resources wouldn't
>>>be applied to VFs. The VFs' resources would be allocated
>>>when we enable SR-IOV capability. So we should not try to
>>>reassign alignment after we enable VFs. It's meaningless
>>>and will release the allocated resources which leads to a bug.
>>>
>>>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>>---
>>>drivers/pci/pci.c |    4 ++++
>>>1 file changed, 4 insertions(+)
>>>
>>>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>index be8f72c..6ae02de 100644
>>>--- a/drivers/pci/pci.c
>>>+++ b/drivers/pci/pci.c
>>>@@ -4822,6 +4822,10 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>>>	resource_size_t align, size;
>>>	u16 command;
>>>
>>>+	/* We should never try to reassign VF's alignment */
>>>+	if (dev->is_virtfn)
>>>+		return;
>>>+
>>Yongji, I think it's correct to ignore VF's BARs. Another concern is:
>>it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
>>example here: one PF has 16 VFs; each VF has only one 1KB. It means
>>the only PF IOV BAR is 16KB. I don't see how it works after expanding
>>it to 64KB which is the page size. It might be not a problem on PowerNV
>>platform, but potentially a issue on x86?
>
>Seems like the alignment would not be applied to IOV BARs because
>pci_reassigndev_resource_alignment() will be called before
>sriov_init().
>

Correct, thanks for the claim. I guess the alignment applied to PF IOV
BARs should be ignored as well? Anyway, the IOV BARs are retireved from
SRIOV capability. It deserves a comment if you plan to take the change.
Actually, the comment here (for ignoring alignment to VF BARs) can be
improved a bit as well, it'd better why the alignment cannot be applied.

Thanks,
Gavin

Re: [PATCH v3 3/7] PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()

[Yongji Xie]

Hi Gavin,

On 2016/7/1 8:50, Gavin Shan wrote:

> On Thu, Jun 30, 2016 at 06:53:09PM +0800, Yongji Xie wrote:
>> We should not disable memory decoding when we reassign alignment
>> in pci_reassigndev_resource_alignment(). It's meaningless and
>> have some side effect. For example, some fixup functions such as
>> quirk_e100_interrupt() read PCI_COMMAND_MEMORY to know whether
>> the devices has been initialized by the firmware or not. If we
>> disable memory decoding here, these functions will get a wrong
>> information that the devices was not initialized by the firmware
>> which may cause a wrong fixup. Besides, disabling memory decoding
>> may also break some devices that need to have memory decoding
>> always-on during probing.
>>
> It seems the changelog isn't correct enough if it's talking about
> below check in code:
>
> if (!(command & PCI_COMMAND_MEMORY) || !pci_resource_start(dev, 0))
> 	return;

Could you give me more details?  It seems that we couldn't disable
e100 interrupts which enabled by some firmware if we disabling
memory decoding here.

>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>> drivers/pci/pci.c |    8 +-------
>> 1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 6ae02de..6241cfc 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4820,7 +4820,6 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> 	int i;
>> 	struct resource *r;
>> 	resource_size_t align, size;
>> -	u16 command;
>>
>> 	/* We should never try to reassign VF's alignment */
>> 	if (dev->is_virtfn)
>> @@ -4838,12 +4837,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> 		return;
>> 	}
>>
>> -	dev_info(&dev->dev,
>> -		"Disabling memory decoding and releasing memory resources.\n");
>> -	pci_read_config_word(dev, PCI_COMMAND, &command);
>> -	command &= ~PCI_COMMAND_MEMORY;
>> -	pci_write_config_word(dev, PCI_COMMAND, command);
>> -
>> +	dev_info(&dev->dev, "Releasing memory resources.\n");
> Is there a problem you found with PCI_COMMAND removed? If so, could you
> please share more details, thanks.

Disabling memory decoding may break some devices which
have flag mmio_always_on. Besides the mmio_always_on case,
I also found this would break some P2P bridge such as
PEX 8718 controller.

Thanks,
Yongji

Re: [PATCH v3 2/7] PCI: Ignore enforced alignment to VF BARs

[Yongji Xie]

Hi Gavin,

On 2016/7/1 14:05, Gavin Shan wrote:

> On Fri, Jul 01, 2016 at 01:27:17PM +0800, Yongji Xie wrote:
>>> On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
>>>> VF BARs are read-only zeroes according to SRIOV spec,
>>>> the normal way(writing BARs) of allocating resources wouldn't
>>>> be applied to VFs. The VFs' resources would be allocated
>>>> when we enable SR-IOV capability. So we should not try to
>>>> reassign alignment after we enable VFs. It's meaningless
>>>> and will release the allocated resources which leads to a bug.
>>>>
>>>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>>> ---
>>>> drivers/pci/pci.c |    4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index be8f72c..6ae02de 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -4822,6 +4822,10 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>>>> 	resource_size_t align, size;
>>>> 	u16 command;
>>>>
>>>> +	/* We should never try to reassign VF's alignment */
>>>> +	if (dev->is_virtfn)
>>>> +		return;
>>>> +
>>> Yongji, I think it's correct to ignore VF's BARs. Another concern is:
>>> it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
>>> example here: one PF has 16 VFs; each VF has only one 1KB. It means
>>> the only PF IOV BAR is 16KB. I don't see how it works after expanding
>>> it to 64KB which is the page size. It might be not a problem on PowerNV
>>> platform, but potentially a issue on x86?
>> Seems like the alignment would not be applied to IOV BARs because
>> pci_reassigndev_resource_alignment() will be called before
>> sriov_init().
>>
> Correct, thanks for the claim. I guess the alignment applied to PF IOV
> BARs should be ignored as well? Anyway, the IOV BARs are retireved from
> SRIOV capability. It deserves a comment if you plan to take the change.
> Actually, the comment here (for ignoring alignment to VF BARs) can be
> improved a bit as well, it'd better why the alignment cannot be applied.
>

Do you mean we should ignore PF IOV BARs like this:

--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4833,7 +4833,7 @@ void pci_reassigndev_resource_alignment(struct 
pci_dev *dev)
         command &= ~PCI_COMMAND_MEMORY;
         pci_write_config_word(dev, PCI_COMMAND, command);

-       for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
+       for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
                 r = &dev->resource[i];
                 if (!(r->flags & IORESOURCE_MEM))
                         continue;

Thanks,
Yongji

Re: [PATCH v3 4/7] PCI: Add a new option for resource_alignment to reassign alignment

[Yongji Xie]

Hi Gavin,

On 2016/7/1 10:25, Gavin Shan wrote:

> On Thu, Jun 30, 2016 at 06:53:10PM +0800, Yongji Xie wrote:
>> When using resource_alignment kernel parameter, the current
>> implement reassigns the alignment by changing resources' size
>> which can potentially break some drivers. For example, the driver
>> uses the size to locate some register whose length is related
>> to the size.
>>
>> This patch adds a new option "noresize" for the parameter to
>> solve this problem.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>> ---
>> Documentation/kernel-parameters.txt |    5 ++++-
>> drivers/pci/pci.c                   |   35 +++++++++++++++++++++++++----------
>> 2 files changed, 29 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 82b42c9..c4802f5 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -2997,13 +2997,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>> 				window. The default value is 64 megabytes.
>> 		resource_alignment=
>> 				Format:
>> -				[<order of align>@][<domain>:]<bus>:<slot>.<func>[; ...]
>> +				[<order of align>@][<domain>:]<bus>:<slot>.<func>
>> +				[:noresize][; ...]
>> 				Specifies alignment and device to reassign
>> 				aligned memory resources.
>> 				If <order of align> is not specified,
>> 				PAGE_SIZE is used as alignment.
>> 				PCI-PCI bridge can be specified, if resource
>> 				windows need to be expanded.
>> +				noresize: Don't change the resources' sizes when
>> +				reassigning alignment.
>> 		ecrc=		Enable/disable PCIe ECRC (transaction layer
>> 				end-to-end CRC checking).
>> 				bios: Use BIOS/firmware settings. This is the
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 6241cfc..1d80a94 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4748,11 +4748,13 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
>> /**
>>   * pci_specified_resource_alignment - get resource alignment specified by user.
>>   * @dev: the PCI device to get
>> + * @resize: whether or not to change resources' size when reassigning alignment
>>   *
>>   * RETURNS: Resource alignment if it is specified.
>>   *          Zero if it is not specified.
>>   */
>> -static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
>> +static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
>> +		bool *resize)
>> {
>> 	int seg, bus, slot, func, align_order, count;
>> 	resource_size_t align = 0;
>> @@ -4787,6 +4789,11 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
>> 			}
>> 		}
>> 		p += count;
>> +		if (!strncmp(p, ":noresize", 9)) {
>> +			*resize = false;
>> +			p += 9;
>> +		} else
>> +			*resize = true;
>> 		if (seg == pci_domain_nr(dev->bus) &&
>> 			bus == dev->bus->number &&
>> 			slot == PCI_SLOT(dev->devfn) &&
>> @@ -4819,6 +4826,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> {
>> 	int i;
>> 	struct resource *r;
>> +	bool resize = true;
>> 	resource_size_t align, size;
>>
>> 	/* We should never try to reassign VF's alignment */
>> @@ -4826,7 +4834,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> 		return;
>>
>> 	/* check if specified PCI is target device to reassign */
>> -	align = pci_specified_resource_alignment(dev);
>> +	align = pci_specified_resource_alignment(dev, &resize);
>> 	if (!align)
>> 		return;
>>
>> @@ -4848,15 +4856,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>> 			continue;
>> 		}
>> 		size = resource_size(r);
>> -		if (size < align) {
>> -			size = align;
>> -			dev_info(&dev->dev,
>> -				"Rounding up size of resource #%d to %#llx.\n",
>> -				i, (unsigned long long)size);
>> +		if (resize) {
>> +			if (size < align) {
>> +				size = align;
>> +				dev_info(&dev->dev,
>> +					"Rounding up size of resource #%d to %#llx.\n",
>> +					i, (unsigned long long)size);
>> +			}
>> +			r->flags |= IORESOURCE_UNSET;
>> +			r->end = size - 1;
>> +			r->start = 0;
>> +		} else {
>> +			r->flags &= ~IORESOURCE_SIZEALIGN;
>> +			r->flags |= IORESOURCE_STARTALIGN | IORESOURCE_UNSET;
> 			f->flags |= (IORESOURCE_STARTALIGN | IORESOURCE_UNSET);
>
> Yongji, one quick question - when IORESOURCE_UNSET is cleared? If it's not
> cleared, is there any unexpected side effects? Thanks.

PCI core will clear this flag in pci_assign_resource() when the BAR
has been assigned properly. If it's not cleared, the driver will fail in
binding the device because pci_enable_resources() will check this flag.

Thanks,
Yongji

Re: [PATCH v3 5/7] PCI: Do not use IORESOURCE_STARTALIGN to identify bridge resources

[Yongji Xie]

Hi Gavin,

On 2016/7/1 10:34, Gavin Shan wrote:

> On Thu, Jun 30, 2016 at 06:53:11PM +0800, Yongji Xie wrote:
>> Now we use the IORESOURCE_STARTALIGN to identify bridge resources
>> in __assign_resources_sorted(). That's quite fragile. We may also
>> set flag IORESOURCE_STARTALIGN for SR-IOV resources in some cases,
>> for example, using the option "noresize" of parameter
>> "pci=resource_alignment".
>>
>> In this patch, we try to use a more robust way to identify
>> bridge resources.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
> Yongji, I think this doesn't have to be part of this series, meaning
> it can be sent or merged separately.
>

Seems like I give a wrong example in my changelog.  The
parameter "pci=resource_alignment" would not set flag
IORESOURCE_STARTALIGN for SR-IOV resources as I replied
to you in previous patch. So this patch has nothing to do
with this series now, I will remove it as you suggested.

Thanks,
Yongji

Re: [PATCH v3 2/7] PCI: Ignore enforced alignment to VF BARs

[Gavin Shan]

On Fri, Jul 01, 2016 at 02:40:16PM +0800, Yongji Xie wrote:
>Hi Gavin,
>
>On 2016/7/1 14:05, Gavin Shan wrote:
>
>>On Fri, Jul 01, 2016 at 01:27:17PM +0800, Yongji Xie wrote:
>>>>On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
>>>>>VF BARs are read-only zeroes according to SRIOV spec,
>>>>>the normal way(writing BARs) of allocating resources wouldn't
>>>>>be applied to VFs. The VFs' resources would be allocated
>>>>>when we enable SR-IOV capability. So we should not try to
>>>>>reassign alignment after we enable VFs. It's meaningless
>>>>>and will release the allocated resources which leads to a bug.
>>>>>
>>>>>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>>>>---
>>>>>drivers/pci/pci.c |    4 ++++
>>>>>1 file changed, 4 insertions(+)
>>>>>
>>>>>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>>index be8f72c..6ae02de 100644
>>>>>--- a/drivers/pci/pci.c
>>>>>+++ b/drivers/pci/pci.c
>>>>>@@ -4822,6 +4822,10 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>>>>>	resource_size_t align, size;
>>>>>	u16 command;
>>>>>
>>>>>+	/* We should never try to reassign VF's alignment */
>>>>>+	if (dev->is_virtfn)
>>>>>+		return;
>>>>>+
>>>>Yongji, I think it's correct to ignore VF's BARs. Another concern is:
>>>>it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
>>>>example here: one PF has 16 VFs; each VF has only one 1KB. It means
>>>>the only PF IOV BAR is 16KB. I don't see how it works after expanding
>>>>it to 64KB which is the page size. It might be not a problem on PowerNV
>>>>platform, but potentially a issue on x86?
>>>Seems like the alignment would not be applied to IOV BARs because
>>>pci_reassigndev_resource_alignment() will be called before
>>>sriov_init().
>>>
>>Correct, thanks for the claim. I guess the alignment applied to PF IOV
>>BARs should be ignored as well? Anyway, the IOV BARs are retireved from
>>SRIOV capability. It deserves a comment if you plan to take the change.
>>Actually, the comment here (for ignoring alignment to VF BARs) can be
>>improved a bit as well, it'd better why the alignment cannot be applied.
>>
>
>Do you mean we should ignore PF IOV BARs like this:
>
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -4833,7 +4833,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev
>*dev)
>        command &= ~PCI_COMMAND_MEMORY;
>        pci_write_config_word(dev, PCI_COMMAND, command);
>
>-       for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
>+       for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>                r = &dev->resource[i];
>                if (!(r->flags & IORESOURCE_MEM))
>                        continue;
>

Yeah, I think it's what I expected. Please add a comment to explain
why PCI bridge windows and PF's IOV BARs are not involed.

Thanks,
Gavin

Re: [PATCH v3 5/7] PCI: Do not use IORESOURCE_STARTALIGN to identify bridge resources

[Gavin Shan]

On Fri, Jul 01, 2016 at 03:04:10PM +0800, Yongji Xie wrote:
>Hi Gavin,
>
>On 2016/7/1 10:34, Gavin Shan wrote:
>
>>On Thu, Jun 30, 2016 at 06:53:11PM +0800, Yongji Xie wrote:
>>>Now we use the IORESOURCE_STARTALIGN to identify bridge resources
>>>in __assign_resources_sorted(). That's quite fragile. We may also
>>>set flag IORESOURCE_STARTALIGN for SR-IOV resources in some cases,
>>>for example, using the option "noresize" of parameter
>>>"pci=resource_alignment".
>>>
>>>In this patch, we try to use a more robust way to identify
>>>bridge resources.
>>>
>>>Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>>Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>
>>Yongji, I think this doesn't have to be part of this series, meaning
>>it can be sent or merged separately.
>>
>
>Seems like I give a wrong example in my changelog.  The
>parameter "pci=resource_alignment" would not set flag
>IORESOURCE_STARTALIGN for SR-IOV resources as I replied
>to you in previous patch. So this patch has nothing to do
>with this series now, I will remove it as you suggested.
>

Thanks, please refine the changelog as it's not related to the
enforced alignment as you claimed when resending it separately.

Thanks,
Gavin

Re: [PATCH v3 0/7] PCI: Add support for enforcing all MMIO BARs not to share PAGE_SIZE

[Yongji Xie]

Hi Bjorn,

Any comment on V3?

Thanks,

Yongji


On 2016/6/30 18:53, Yongji Xie wrote:
> This series aims to add an option for PCI resource allocator to
> force BARs not to share PAGE_SIZE. This would make sense to VFIO
> driver. Because current VFIO implementation disallows to mmap
> sub-page(size < PAGE_SIZE) MMIO BARs which may share the same page
> with other BARs for security reasons. Thus, we have to handle mmio
> access to these BARs in QEMU emulation rather than in guest which
> will cause some performance loss.
>
> To achieve that, we would like to make use of the existing
> resource_alignment kernel parameter and force a minimum alignment
> of PAGE_SIZE. It's flexible. And we can enable it by default on
> some archs which may easily hit the performance issue because of
> their 64K page.
>
> In this series, patch 1,2,3 fixed bugs of using resource_alignment;
> patch 4,5 tried to add a new option for resource_alignment to use
> IORESOURCE_STARTALIGN to specify the alignment of PCI BARs; patch 6
> modified resource_alignment to support syntax which can be used to
> enforce the alignment of all MMIO BARs to be at least PAGE_SIZE;
> patch 7 enabled this feature by default on PowerNV platform.
>
> Changelog v3:
> - Ignore enforced alignment to fixed BARs
> - Fix issue that disabling memory decoding when reassigning the alignment
> - Only enable default alignment on PowerNV platform
>
> Changelog v2:
> - Ignore enforced alignment to VF BARs on pci_reassigndev_resource_alignment()
>
> Yongji Xie (7):
>    PCI: Ignore enforced alignment when kernel uses existing firmware setup
>    PCI: Ignore enforced alignment to VF BARs
>    PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()
>    PCI: Add a new option for resource_alignment to reassign alignment
>    PCI: Do not use IORESOURCE_STARTALIGN to identify bridge resources
>    PCI: Add support for enforcing all MMIO BARs to be page aligned
>    PCI: Add a macro to set default alignment for all PCI devices
>
>   Documentation/kernel-parameters.txt |    7 +-
>   arch/powerpc/include/asm/pci.h      |    4 ++
>   drivers/pci/pci.c                   |  123 +++++++++++++++++++++++++++--------
>   drivers/pci/setup-bus.c             |    9 ++-
>   4 files changed, 111 insertions(+), 32 deletions(-)
>