[PATCH v5 0/4] PCI: Introduce a way to enforce all MMIO BARs not to share PAGE_SIZE

This series introduces a way for PCI resource allocator to force
MMIO 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.

In our solution, we try to make use of the existing code path of
resource_alignment kernel parameter and add a macro to set default
alignment for it. Thus we can define this macro by default on some
archs which may easily hit the performance issue because of their
64K page.

In this series, patch 1,2 fixed bugs of using resource_alignment;
patch 3 tried to add a new option for resource_alignment to use
IORESOURCE_STARTALIGN to specify the alignment of PCI BARs; patch 4
adds a macro to set the default alignment of all MMIO BARs.

Changelog v5:
- Rebased against v4.8-rc6
- Drop the patch that forbidding disable memory decoding in
  pci_reassigndev_resource_alignment()

Changelog v4:
- Rebased against v4.8-rc1
- Drop one irrelevant patch
- Drop the patch that adding wildcard to resource_alignment to enforce
  the alignment of all MMIO BARs to be at least PAGE_SIZE
- Change the format of option "noresize" of resource_alignment
- Code style improvements

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 (4):
  PCI: Ignore enforced alignment when kernel uses existing firmware setup
  PCI: Ignore enforced alignment to VF BARs
  PCI: Add a new option for resource_alignment to reassign alignment
  PCI: Add a macro to set default alignment for all PCI devices

 Documentation/kernel-parameters.txt |    9 +++--
 arch/powerpc/include/asm/pci.h      |    4 +++
 drivers/pci/pci.c                   |   63 +++++++++++++++++++++++++++++------
 3 files changed, 63 insertions(+), 13 deletions(-)

-- 
1.7.9.5

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 <redacted>
---
 drivers/pci/pci.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d51..2d85a96 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -4959,6 +4959,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)
+			pr_info_once("PCI: resource_alignment ignored with PCI_PROBE_ONLY\n");
+		spin_unlock(&resource_alignment_lock);
+		return 0;
+	}
+
 	while (*p) {
 		count = 0;
 		if (sscanf(p, "%d%n", &align_order, &count) == 1 &&

@@ -5063,6 +5070,12 @@ 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

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 <redacted>
---
 drivers/pci/pci.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 2d85a96..b8357d7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -5048,6 +5048,15 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	resource_size_t align, size;
 	u16 command;
 
+	/*
+	 * VF BARs are RO zero according to SR-IOV spec 3.4.1.11. Their
+	 * resources would be allocated when we enable them and not be
+	 * re-allocated any more. So we should never try to reassign
+	 * VF's alignment here.
+	 */
+	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

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 <redacted>
---
 Documentation/kernel-parameters.txt |    9 ++++++---
 drivers/pci/pci.c                   |   37 +++++++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a4f4d69..d6a340d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt

@@ -3023,9 +3023,10 @@ 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>@]pci:<vendor>:<device>\
-						[:<subvendor>:<subdevice>][; ...]
+				[<order of align>@][noresize@][<domain>:]
+				<bus>:<slot>.<func>[; ...]
+				[<order of align>@][noresize@]pci:<vendor>:<device>
+				[:<subvendor>:<subdevice>][; ...]
 				Specifies alignment and device to reassign
 				aligned memory resources.
 				If <order of align> is not specified,

@@ -3036,6 +3037,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				instances of a device, the PCI vendor,
 				device, subvendor, and subdevice may be
 				specified, e.g., 4096@pci:8086:9c22:103c:198f
+				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 b8357d7..37f8062 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -4946,11 +4946,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;
 	unsigned short vendor, device, subsystem_vendor, subsystem_device;

@@ -4974,6 +4976,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
 		} else {
 			align_order = -1;
 		}
+
+		if (!strncmp(p, "noresize@", 9)) {
+			*resize = false;
+			p += 9;
+		} else
+			*resize = true;
+
 		if (strncmp(p, "pci:", 4) == 0) {
 			/* PCI vendor/device (subvendor/subdevice) ids are specified */
 			p += 4;

@@ -5045,6 +5054,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 {
 	int i;
 	struct resource *r;
+	bool resize = true;
 	resource_size_t align, size;
 	u16 command;
 

@@ -5058,7 +5068,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;
 

@@ -5086,15 +5096,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		}
 
 		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

On Tue, Sep 13, 2016 at 05:00:33PM +0800, Yongji Xie wrote:

Why do you want to keep this code that changes the size of the resource?
Can't we just *always* use IORESOURCE_STARTALIGN as below?  It seems like
that would potentially fix bugs, as you mention.

I think it'd be better if we didn't change the size, even if the user
didn't specify "noresize@".  We wouldn't even need the "noresize@" option
then.

Or is there some reason to keep the resize?  If it's just a question of
being afraid to change the existing behavior because of the risk, I'm
willing to take that risk.

+		} 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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2016/9/29 6:42, Bjorn Helgaas wrote:

On Tue, Sep 13, 2016 at 05:00:33PM +0800, Yongji Xie wrote:

quoted

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 <redacted>
---
  Documentation/kernel-parameters.txt |    9 ++++++---
  drivers/pci/pci.c                   |   37 +++++++++++++++++++++++++----------
  2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a4f4d69..d6a340d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt

@@ -3023,9 +3023,10 @@ 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>@]pci:<vendor>:<device>\
-						[:<subvendor>:<subdevice>][; ...]
+				[<order of align>@][noresize@][<domain>:]
+				<bus>:<slot>.<func>[; ...]
+				[<order of align>@][noresize@]pci:<vendor>:<device>
+				[:<subvendor>:<subdevice>][; ...]
  				Specifies alignment and device to reassign
  				aligned memory resources.
  				If <order of align> is not specified,

@@ -3036,6 +3037,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
  				instances of a device, the PCI vendor,
  				device, subvendor, and subdevice may be
  				specified, e.g., 4096@pci:8086:9c22:103c:198f
+				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 b8357d7..37f8062 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -4946,11 +4946,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;
  	unsigned short vendor, device, subsystem_vendor, subsystem_device;

@@ -4974,6 +4976,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
  		} else {
  			align_order = -1;
  		}
+
+		if (!strncmp(p, "noresize@", 9)) {
+			*resize = false;
+			p += 9;
+		} else
+			*resize = true;
+
  		if (strncmp(p, "pci:", 4) == 0) {
  			/* PCI vendor/device (subvendor/subdevice) ids are specified */
  			p += 4;

@@ -5045,6 +5054,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
  {
  	int i;
  	struct resource *r;
+	bool resize = true;
  	resource_size_t align, size;
  	u16 command;
  

@@ -5058,7 +5068,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;
  

@@ -5086,15 +5096,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
  		}
  
  		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;

Why do you want to keep this code that changes the size of the resource?
Can't we just *always* use IORESOURCE_STARTALIGN as below?  It seems like
that would potentially fix bugs, as you mention.

I think it'd be better if we didn't change the size, even if the user
didn't specify "noresize@".  We wouldn't even need the "noresize@" option
then.

Or is there some reason to keep the resize?  If it's just a question of
being afraid to change the existing behavior because of the risk, I'm
willing to take that risk.

Hi Bjorn,

Thanks for your time. The reason is just like what you said.  I'm worried
that this may break the existing behavior.  I'll updated this patch as
you suggested.

Regards,
Yongji

quoted

+		} 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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Sep 29, 2016 at 10:38:01AM +0800, Yongji Xie wrote:

On 2016/9/29 6:42, Bjorn Helgaas wrote:

quoted

On Tue, Sep 13, 2016 at 05:00:33PM +0800, Yongji Xie wrote:

quoted

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 <redacted>
---
 Documentation/kernel-parameters.txt |    9 ++++++---
 drivers/pci/pci.c                   |   37 +++++++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a4f4d69..d6a340d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt

@@ -3023,9 +3023,10 @@ 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>@]pci:<vendor>:<device>\
-						[:<subvendor>:<subdevice>][; ...]
+				[<order of align>@][noresize@][<domain>:]
+				<bus>:<slot>.<func>[; ...]
+				[<order of align>@][noresize@]pci:<vendor>:<device>
+				[:<subvendor>:<subdevice>][; ...]
 				Specifies alignment and device to reassign
 				aligned memory resources.
 				If <order of align> is not specified,

@@ -3036,6 +3037,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				instances of a device, the PCI vendor,
 				device, subvendor, and subdevice may be
 				specified, e.g., 4096@pci:8086:9c22:103c:198f
+				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 b8357d7..37f8062 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -4946,11 +4946,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;
 	unsigned short vendor, device, subsystem_vendor, subsystem_device;

@@ -4974,6 +4976,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
 		} else {
 			align_order = -1;
 		}
+
+		if (!strncmp(p, "noresize@", 9)) {
+			*resize = false;
+			p += 9;
+		} else
+			*resize = true;
+
 		if (strncmp(p, "pci:", 4) == 0) {
 			/* PCI vendor/device (subvendor/subdevice) ids are specified */
 			p += 4;

@@ -5045,6 +5054,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 {
 	int i;
 	struct resource *r;
+	bool resize = true;
 	resource_size_t align, size;
 	u16 command;

@@ -5058,7 +5068,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;

@@ -5086,15 +5096,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		}
 		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;

Why do you want to keep this code that changes the size of the resource?
Can't we just *always* use IORESOURCE_STARTALIGN as below?  It seems like
that would potentially fix bugs, as you mention.

I think it'd be better if we didn't change the size, even if the user
didn't specify "noresize@".  We wouldn't even need the "noresize@" option
then.

Or is there some reason to keep the resize?  If it's just a question of
being afraid to change the existing behavior because of the risk, I'm
willing to take that risk.

Hi Bjorn,

Thanks for your time. The reason is just like what you said.  I'm worried
that this may break the existing behavior.  I'll updated this patch as
you suggested.

I guess one problem is that if we *don't* change the size, there's
nothing to prevent something else from being allocated right after the
resource.  For example, if we have a 1K BAR, and the objective is to
make sure it's on a 4K page by itself, not only do we have to align
the BAR on 4K, we *also* have to keep anything else from using the
remaining 3K of that page.

Maybe it's easier to fix the drivers so they don't rely on the size.
Do you have examples of such drivers?

I'm hesitant to add a new option to the "pci=resource_alignment"
parameter because it's already very complicated and it exposes more
kernel internals than I'm really comfortable with.  If we can figure
out a way to make the existing parameter work for everybody, I would
prefer that.

Bjorn

quoted

quoted

+		} 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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2016/9/29 19:54, Bjorn Helgaas wrote:

On Thu, Sep 29, 2016 at 10:38:01AM +0800, Yongji Xie wrote:

quoted

On 2016/9/29 6:42, Bjorn Helgaas wrote:

quoted

On Tue, Sep 13, 2016 at 05:00:33PM +0800, Yongji Xie wrote:

quoted

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 <redacted>
---
  Documentation/kernel-parameters.txt |    9 ++++++---
  drivers/pci/pci.c                   |   37 +++++++++++++++++++++++++----------
  2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a4f4d69..d6a340d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt

@@ -3023,9 +3023,10 @@ 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>@]pci:<vendor>:<device>\
-						[:<subvendor>:<subdevice>][; ...]
+				[<order of align>@][noresize@][<domain>:]
+				<bus>:<slot>.<func>[; ...]
+				[<order of align>@][noresize@]pci:<vendor>:<device>
+				[:<subvendor>:<subdevice>][; ...]
  				Specifies alignment and device to reassign
  				aligned memory resources.
  				If <order of align> is not specified,

@@ -3036,6 +3037,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
  				instances of a device, the PCI vendor,
  				device, subvendor, and subdevice may be
  				specified, e.g., 4096@pci:8086:9c22:103c:198f
+				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 b8357d7..37f8062 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -4946,11 +4946,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;
  	unsigned short vendor, device, subsystem_vendor, subsystem_device;

@@ -4974,6 +4976,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
  		} else {
  			align_order = -1;
  		}
+
+		if (!strncmp(p, "noresize@", 9)) {
+			*resize = false;
+			p += 9;
+		} else
+			*resize = true;
+
  		if (strncmp(p, "pci:", 4) == 0) {
  			/* PCI vendor/device (subvendor/subdevice) ids are specified */
  			p += 4;

@@ -5045,6 +5054,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
  {
  	int i;
  	struct resource *r;
+	bool resize = true;
  	resource_size_t align, size;
  	u16 command;

@@ -5058,7 +5068,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;

@@ -5086,15 +5096,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
  		}
  		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;

Why do you want to keep this code that changes the size of the resource?
Can't we just *always* use IORESOURCE_STARTALIGN as below?  It seems like
that would potentially fix bugs, as you mention.

I think it'd be better if we didn't change the size, even if the user
didn't specify "noresize@".  We wouldn't even need the "noresize@" option
then.

Or is there some reason to keep the resize?  If it's just a question of
being afraid to change the existing behavior because of the risk, I'm
willing to take that risk.

Hi Bjorn,

Thanks for your time. The reason is just like what you said.  I'm worried
that this may break the existing behavior.  I'll updated this patch as
you suggested.

I guess one problem is that if we *don't* change the size, there's
nothing to prevent something else from being allocated right after the
resource.  For example, if we have a 1K BAR, and the objective is to
make sure it's on a 4K page by itself, not only do we have to align
the BAR on 4K, we *also* have to keep anything else from using the
remaining 3K of that page.

In VFIO module, a dummy resource would be allocated into the
right after the resource which can make sure the BAR is on an exclusive
page.

If the resource_alignment is used for other purpose, it's true that
some other resources may be allocated into the right after the resource.
But I'm not sure whether we need to care about this in those cases.
It may be a problem when there are some existing applications
need this guarantee.

Maybe it's easier to fix the drivers so they don't rely on the size.
Do you have examples of such drivers?

ata/sata_sis.c:    (pci_resource_len(pdev, SIS_SCR_PCI_BAR) < 128))) {

I'm hesitant to add a new option to the "pci=resource_alignment"
parameter because it's already very complicated and it exposes more
kernel internals than I'm really comfortable with.  If we can figure
out a way to make the existing parameter work for everybody, I would
prefer that.

If we can make sure the PCI driver only use pci_resource_len() to get
the BAR's length,  we can enhance this function to have an ability to
get the original length of the BAR so that we can still change resource's
size when using resource_alignment. For example, we can introduce an
addtional field to "struct resource::ori_lengh". In pci_resource_len(),
we do something like:

if (resource->ori_length)
     return resource->ori_length;

Thanks,
Yongji

When vfio passthroughs 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.

This patch adds a macro to set default alignment for all
PCI devices. Then we could solve this issue on some platforms
which would easily hit this issue because of their 64K page
such as PowerNV platform by defining this macro as PAGE_SIZE.

Signed-off-by: Yongji Xie <redacted>
---
 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 e9bd6cf..5e31bc2 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 37f8062..9c61cbe 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -4959,6 +4959,10 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 	resource_size_t align = 0;
 	char *p;
 
+#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

On Tue, Sep 13, 2016 at 05:00:34PM +0800, Yongji Xie wrote:

I'm a little confused about how this works.

I think this change only does something if the user specifies
"pci=resource_alignment=..." or writes to the /sys/.../resource_alignment
file, because those are the only ways to set resource_alignment_param.

If that's true, isn't the *default* to align to PAGE_SIZE?  So I don't
understand what PCIBIOS_DEFAULT_ALIGNMENT changes.

And I'm hoping we can get rid of the resize flag based on the
discussion of the previous patch.

 	spin_lock(&resource_alignment_lock);
 	p = resource_alignment_param;
 	if (pci_has_flag(PCI_PROBE_ONLY)) {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2016/9/29 22:00, Bjorn Helgaas wrote:

On Tue, Sep 13, 2016 at 05:00:34PM +0800, Yongji Xie wrote:

quoted

When vfio passthroughs 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.

This patch adds a macro to set default alignment for all
PCI devices. Then we could solve this issue on some platforms
which would easily hit this issue because of their 64K page
such as PowerNV platform by defining this macro as PAGE_SIZE.

Signed-off-by: Yongji Xie <redacted>
---
  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 e9bd6cf..5e31bc2 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 37f8062..9c61cbe 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -4959,6 +4959,10 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
  	resource_size_t align = 0;
  	char *p;
  
+#ifdef PCIBIOS_DEFAULT_ALIGNMENT
+	align = PCIBIOS_DEFAULT_ALIGNMENT;
+	*resize = false;
+#endif

I'm a little confused about how this works.

I think this change only does something if the user specifies
"pci=resource_alignment=..." or writes to the /sys/.../resource_alignment
file, because those are the only ways to set resource_alignment_param.

If that's true, isn't the *default* to align to PAGE_SIZE?  So I don't
understand what PCIBIOS_DEFAULT_ALIGNMENT changes.

In pci_reassigndev_resource_alignment(), we can see:

align = pci_specified_resource_alignment(dev);
     if (!align)
         return;

So we would still align the device's BAR to PAGE_SIZE without
set resource_alignment_param if we set @align to a default value
in pci_specified_resource_alignment().

And I'm hoping we can get rid of the resize flag based on the
discussion of the previous patch.

Will do.

Thanks,
Yongji

Hi Bjorn,

Kindly Ping... Any comment on V5?

Thanks,
Yongji

On 2016/9/13 17:00, Yongji Xie wrote:

This series introduces a way for PCI resource allocator to force
MMIO 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.

In our solution, we try to make use of the existing code path of
resource_alignment kernel parameter and add a macro to set default
alignment for it. Thus we can define this macro by default on some
archs which may easily hit the performance issue because of their
64K page.

In this series, patch 1,2 fixed bugs of using resource_alignment;
patch 3 tried to add a new option for resource_alignment to use
IORESOURCE_STARTALIGN to specify the alignment of PCI BARs; patch 4
adds a macro to set the default alignment of all MMIO BARs.

Changelog v5:
- Rebased against v4.8-rc6
- Drop the patch that forbidding disable memory decoding in
   pci_reassigndev_resource_alignment()

Changelog v4:
- Rebased against v4.8-rc1
- Drop one irrelevant patch
- Drop the patch that adding wildcard to resource_alignment to enforce
   the alignment of all MMIO BARs to be at least PAGE_SIZE
- Change the format of option "noresize" of resource_alignment
- Code style improvements

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 (4):
   PCI: Ignore enforced alignment when kernel uses existing firmware setup
   PCI: Ignore enforced alignment to VF BARs
   PCI: Add a new option for resource_alignment to reassign alignment
   PCI: Add a macro to set default alignment for all PCI devices

  Documentation/kernel-parameters.txt |    9 +++--
  arch/powerpc/include/asm/pci.h      |    4 +++
  drivers/pci/pci.c                   |   63 +++++++++++++++++++++++++++++------
  3 files changed, 63 insertions(+), 13 deletions(-)